You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jo...@apache.org on 2019/05/15 17:08:51 UTC
[impala] 03/06: IMPALA-966: Attribute type error to the right
expression in an insert statement
This is an automated email from the ASF dual-hosted git repository.
joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 30ea70a7ff00bad1c104b63083fdef06e406c595
Author: Alice Fan <fa...@gmail.com>
AuthorDate: Tue Apr 16 19:12:43 2019 -0700
IMPALA-966: Attribute type error to the right expression in an insert
statement
Currently, if an insert statement contains multiple expressions
that are incompatible with the column type, the error message
returned attributes the error to the wrong expression.
This patch makes sure the right expression is blamed. If there are
multiple incompatible type values for the target column, then the
error is attributed to the first widest (highest precision)
incompatible type expression.
Testing:
- Added tests to AnalyzeStmtsTest.java
Change-Id: I88718fc2cbe1a492165435a542fd2d91d8693a39
Reviewed-on: http://gerrit.cloudera.org:8080/13050
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
.../java/org/apache/impala/analysis/Analyzer.java | 21 +++++++++-----
.../org/apache/impala/analysis/InsertStmt.java | 22 +++++++++++----
.../org/apache/impala/analysis/ModifyStmt.java | 2 +-
.../org/apache/impala/analysis/StatementBase.java | 17 +++++++----
.../java/org/apache/impala/analysis/UnionStmt.java | 12 +++++++-
.../apache/impala/analysis/AnalyzeExprsTest.java | 2 +-
.../apache/impala/analysis/AnalyzeStmtsTest.java | 33 +++++++++++++++++++++-
7 files changed, 86 insertions(+), 23 deletions(-)
diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
index eded0d3..d54d02e 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
@@ -2323,25 +2323,31 @@ public class Analyzer {
/**
* Casts the exprs in the given lists position-by-position such that for every i,
* the i-th expr among all expr lists is compatible.
+ * Returns a list of exprs such that for every i-th expr in that list, it is the first
+ * widest compatible expression encountered among all i-th exprs in the expr lists.
+ * Returns null if an empty expression list or null is passed to it.
* Throw an AnalysisException if the types are incompatible.
*/
- public void castToUnionCompatibleTypes(List<List<Expr>> exprLists)
+ public List<Expr> castToUnionCompatibleTypes(List<List<Expr>> exprLists)
throws AnalysisException {
- if (exprLists == null || exprLists.size() < 2) return;
+ if (exprLists == null || exprLists.size() == 0) return null;
+ if (exprLists.size() == 1) return exprLists.get(0);
// Determine compatible types for exprs, position by position.
List<Expr> firstList = exprLists.get(0);
+ List<Expr> widestExprs = new ArrayList<>(firstList.size());
for (int i = 0; i < firstList.size(); ++i) {
// Type compatible with the i-th exprs of all expr lists.
// Initialize with type of i-th expr in first list.
Type compatibleType = firstList.get(i).getType();
- // Remember last compatible expr for error reporting.
- Expr lastCompatibleExpr = firstList.get(i);
+ widestExprs.add(firstList.get(i));
for (int j = 1; j < exprLists.size(); ++j) {
Preconditions.checkState(exprLists.get(j).size() == firstList.size());
- compatibleType = getCompatibleType(compatibleType,
- lastCompatibleExpr, exprLists.get(j).get(i));
- lastCompatibleExpr = exprLists.get(j).get(i);
+ Type preType = compatibleType;
+ compatibleType = getCompatibleType(
+ compatibleType, widestExprs.get(i), exprLists.get(j).get(i));
+ // compatibleType will be updated if a new wider type is encountered
+ if (preType != compatibleType) widestExprs.set(i, exprLists.get(j).get(i));
}
// Now that we've found a compatible type, add implicit casts if necessary.
for (int j = 0; j < exprLists.size(); ++j) {
@@ -2351,6 +2357,7 @@ public class Analyzer {
}
}
}
+ return widestExprs;
}
public String getDefaultDb() { return globalState_.queryCtx.session.database; }
diff --git a/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java b/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
index f3bf94c..1dcc99f 100644
--- a/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
@@ -684,12 +684,23 @@ public class InsertStmt extends StatementBase {
kuduPartitionColumnNames = getKuduPartitionColumnNames((FeKuduTable) table_);
}
+ UnionStmt unionStmt =
+ (queryStmt_ instanceof UnionStmt) ? (UnionStmt) queryStmt_ : null;
+ List<Expr> widestTypeExprList = null;
+ if (unionStmt != null && unionStmt.getWidestExprs() != null
+ && unionStmt.getWidestExprs().size() > 0) {
+ widestTypeExprList = unionStmt.getWidestExprs();
+ }
+
// Check dynamic partition columns for type compatibility.
for (int i = 0; i < selectListExprs.size(); ++i) {
Column targetColumn = selectExprTargetColumns.get(i);
- Expr compatibleExpr = checkTypeCompatibility(
- targetTableName_.toString(), targetColumn, selectListExprs.get(i),
- analyzer.getQueryOptions().isDecimal_v2());
+ // widestTypeExpr is widest type expression for column i
+ Expr widestTypeExpr =
+ (widestTypeExprList != null) ? widestTypeExprList.get(i) : null;
+ Expr compatibleExpr = checkTypeCompatibility(targetTableName_.toString(),
+ targetColumn, selectListExprs.get(i), analyzer.getQueryOptions().isDecimal_v2(),
+ widestTypeExpr);
if (targetColumn.getPosition() < numClusteringCols) {
// This is a dynamic clustering column
tmpPartitionKeyExprs.add(compatibleExpr);
@@ -710,9 +721,8 @@ public class InsertStmt extends StatementBase {
if (pkv.isStatic()) {
// tableColumns is guaranteed to exist after the earlier analysis checks
Column tableColumn = table_.getColumn(pkv.getColName());
- Expr compatibleExpr = checkTypeCompatibility(
- targetTableName_.toString(), tableColumn,
- pkv.getLiteralValue(), analyzer.isDecimalV2());
+ Expr compatibleExpr = checkTypeCompatibility(targetTableName_.toString(),
+ tableColumn, pkv.getLiteralValue(), analyzer.isDecimalV2(), null);
tmpPartitionKeyExprs.add(compatibleExpr);
tmpPartitionKeyNames.add(pkv.getColName());
}
diff --git a/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java b/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
index 5d34b97..bdacc88 100644
--- a/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
@@ -288,7 +288,7 @@ public abstract class ModifyStmt extends StatementBase {
}
rhsExpr = checkTypeCompatibility(targetTableRef_.getDesc().getTable().getFullName(),
- c, rhsExpr, analyzer.isDecimalV2());
+ c, rhsExpr, analyzer.isDecimalV2(), null /*widestTypeSrcExpr*/);
uniqueSlots.add(lhsSlotRef.getSlotId());
selectList.add(new SelectListItem(rhsExpr, null));
referencedColumns.add(colIndexMap.get(c.getName()));
diff --git a/fe/src/main/java/org/apache/impala/analysis/StatementBase.java b/fe/src/main/java/org/apache/impala/analysis/StatementBase.java
index 4599e1a..e0fb2de 100644
--- a/fe/src/main/java/org/apache/impala/analysis/StatementBase.java
+++ b/fe/src/main/java/org/apache/impala/analysis/StatementBase.java
@@ -193,14 +193,19 @@ public abstract class StatementBase extends StmtNode {
* are incompatible. 'dstTableName' is only used when constructing an AnalysisException
* message.
*
+ * 'widestTypeSrcExpr' is the first widest type expression of the source expressions.
+ * This is only used when constructing an AnalysisException message to make sure the
+ * right expression is blamed in the error message.
+ *
* If strictDecimal is true, only consider casts that result in no loss of information
* when casting between decimal types.
*/
- protected Expr checkTypeCompatibility(String dstTableName, Column dstCol,
- Expr srcExpr, boolean strictDecimal) throws AnalysisException {
+ protected Expr checkTypeCompatibility(String dstTableName, Column dstCol, Expr srcExpr,
+ boolean strictDecimal, Expr widestTypeSrcExpr) throws AnalysisException {
Type dstColType = dstCol.getType();
Type srcExprType = srcExpr.getType();
+ if (widestTypeSrcExpr == null) widestTypeSrcExpr = srcExpr;
// Trivially compatible, unless the type is complex.
if (dstColType.equals(srcExprType) && !dstColType.isComplexType()) return srcExpr;
@@ -215,10 +220,10 @@ public abstract class StatementBase extends StmtNode {
}
if (!compatType.equals(dstColType) && !compatType.isNull()) {
throw new AnalysisException(String.format(
- "Possible loss of precision for target table '%s'.\nExpression '%s' (type: " +
- "%s) would need to be cast to %s for column '%s'",
- dstTableName, srcExpr.toSql(), srcExprType.toSql(), dstColType.toSql(),
- dstCol.getName()));
+ "Possible loss of precision for target table '%s'.\nExpression '%s' (type: "
+ + "%s) would need to be cast to %s for column '%s'",
+ dstTableName, widestTypeSrcExpr.toSql(), srcExprType.toSql(),
+ dstColType.toSql(), dstCol.getName()));
}
return srcExpr.castTo(compatType);
}
diff --git a/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java b/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
index 8438c53..f32fa4b 100644
--- a/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
@@ -157,6 +157,12 @@ public class UnionStmt extends QueryStmt {
// (if any). Same as resultExprs_ if there is no ORDER BY.
private List<Expr> unionResultExprs_ = new ArrayList<>();
+ // List of expressions produced by analyzer.castToUnionCompatibleTypes().
+ // Contains a list of exprs such that for every i-th expr in that list, it is the first
+ // widest compatible expression encountered among all i-th exprs in every result expr
+ // list of the union operands.
+ protected List<Expr> widestExprs_ = new ArrayList<>();
+
// END: Members that need to be reset()
/////////////////////////////////////////
@@ -191,6 +197,7 @@ public class UnionStmt extends QueryStmt {
hasAnalyticExprs_ = other.hasAnalyticExprs_;
withClause_ = (other.withClause_ != null) ? other.withClause_.clone() : null;
unionResultExprs_ = Expr.cloneList(other.unionResultExprs_);
+ widestExprs_ = other.widestExprs_;
}
public List<UnionOperand> getOperands() { return operands_; }
@@ -241,7 +248,7 @@ public class UnionStmt extends QueryStmt {
for (UnionOperand op: operands_) {
resultExprLists.add(op.getQueryStmt().getResultExprs());
}
- analyzer.castToUnionCompatibleTypes(resultExprLists);
+ widestExprs_ = analyzer.castToUnionCompatibleTypes(resultExprLists);
// Create tuple descriptor materialized by this UnionStmt, its resultExprs, and
// its sortInfo if necessary.
@@ -616,6 +623,8 @@ public class UnionStmt extends QueryStmt {
public List<Expr> getUnionResultExprs() { return unionResultExprs_; }
+ public List<Expr> getWidestExprs() { return widestExprs_; }
+
@Override
public UnionStmt clone() { return new UnionStmt(this); }
@@ -637,5 +646,6 @@ public class UnionStmt extends QueryStmt {
toSqlString_ = null;
hasAnalyticExprs_ = false;
unionResultExprs_.clear();
+ widestExprs_ = null;
}
}
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
index d37b1ca..62cb502 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
@@ -396,7 +396,7 @@ public class AnalyzeExprsTest extends AnalyzerTest {
AnalysisError("insert into functional.alltypesinsert (tinyint_col, year, month) " +
"values(CAST(999 AS DECIMAL(3,0)), 1, 1)",
"Possible loss of precision for target table 'functional.alltypesinsert'.\n" +
- "Expression 'cast(999 as decimal(3,0))' (type: DECIMAL(3,0)) would need to be " +
+ "Expression 'CAST(999 AS DECIMAL(3,0))' (type: DECIMAL(3,0)) would need to be " +
"cast to TINYINT for column 'tinyint_col'");
}
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
index 8dca67d..9b42e3e 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
@@ -3399,6 +3399,37 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
AnalyzesOk("insert into d.flt select cast(1 as decimal(20, 10))", decimalV1Ctx);
AnalyzesOk("insert into d.flt select cast(1 as decimal(20, 10))", decimalV2Ctx);
+
+ // IMPALA-966: Test insertion of incompatible expressions. Error should blame the
+ // first widest (highest precision) incompatible type expression.
+ // Test insert multiple values with compatible and incompatible types into a column
+ String query = "insert into functional.testtbl (id) "
+ + "values (10), (cast(1 as float)), (cast(3 as double))";
+ AnalysisError(query,
+ "Possible loss of precision "
+ + "for target table 'functional.testtbl'.\n"
+ + "Expression 'CAST(3 AS DOUBLE)' (type: DOUBLE) "
+ + "would need to be cast to BIGINT for column 'id'");
+ // Test insert multiple values with the same incompatible type into a column
+ query = "insert into functional.testtbl (id) "
+ + "values (cast(1 as float)), (cast(2 as float)), (cast(3 as float))";
+ AnalysisError(query,
+ "Possible loss of precision "
+ + "for target table 'functional.testtbl'.\n"
+ + "Expression 'CAST(1 AS FLOAT)' (type: FLOAT) "
+ + "would need to be cast to BIGINT for column 'id'");
+ // Test insert unions of multiple compatible and incompatible types expressions
+ // into multiple columns
+ query = "insert into functional.alltypes (int_col, float_col) "
+ + "partition(year=2019, month=4) "
+ + "(select int_col, float_col from functional.alltypes union "
+ + "select float_col, double_col from functional.alltypes union "
+ + "select double_col, int_col from functional.alltypes)";
+ AnalysisError(query,
+ "Possible loss of precision "
+ + "for target table 'functional.alltypes'.\n"
+ + "Expression 'double_col' (type: DOUBLE) "
+ + "would need to be cast to INT for column 'int_col'");
}
/**
@@ -3896,7 +3927,7 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
@Test
public void TestClone() {
testNumberOfMembers(QueryStmt.class, 11);
- testNumberOfMembers(UnionStmt.class, 9);
+ testNumberOfMembers(UnionStmt.class, 10);
testNumberOfMembers(ValuesStmt.class, 0);
// Also check TableRefs.