You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/07/11 05:54:00 UTC
[2/4] impala git commit: IMPALA-7254: Inconsistent decimal behavior
for IN/BETWEEN predicate
IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate
In decimal v2, performing a cast that can result in a loss of precision
is considered as an error. In the prior code when finding a compatible
type for performing a cast between expressions that have decimal and
floating types can cause the decimal expressions to never be compared to
other decimal expressions. This results in a cast to a type that may not
be truly compatible in all expressions, especially when decimal v2 is
enabled. This patch fixes it by grouping all the decimals together to make
sure they are compatible with each other.
Testing:
- Added new FE tests
- Ran all core tests
Change-Id: I44f6ea45f765be7201630541354c72fda0a8a98d
Reviewed-on: http://gerrit.cloudera.org:8080/10882
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/f1643662
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/f1643662
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/f1643662
Branch: refs/heads/master
Commit: f1643662882b73dcc9c32b49ada69f5bfda06770
Parents: 9abd1fe
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Fri Jul 6 00:00:34 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Wed Jul 11 02:32:09 2018 +0000
----------------------------------------------------------------------
.../org/apache/impala/analysis/Analyzer.java | 38 ++++++++++--
.../impala/analysis/AnalyzeExprsTest.java | 64 ++++++++++++++++++++
2 files changed, 97 insertions(+), 5 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/f1643662/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
----------------------------------------------------------------------
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 95eec08..0dd68e9 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
@@ -19,6 +19,7 @@ package org.apache.impala.analysis;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.Comparator;
import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.Iterator;
@@ -2264,13 +2265,40 @@ public class Analyzer {
* returns compatible type otherwise.
*/
public Type castAllToCompatibleType(List<Expr> exprs) throws AnalysisException {
- // Determine compatible type of exprs.
- Expr lastCompatibleExpr = exprs.get(0);
+ // Group all decimal types together so that when finding a compatible type all
+ // decimal types will be compared against each other first. For example:
+ //
+ // These expressions are not compatible because of strict decimal and should throw
+ // an AnalysisException.
+ // [decimal(38, 1), double, decimal(38, 37)]
+ //
+ // Without sorting:
+ // 1. Comparing decimal(38, 1) and double - OK
+ // 2. Comparing double and decimal(38, 37) - OK
+ //
+ // With sorting:
+ // [decimal(38, 1), decimal(38, 37), double]
+ // 1. Comparing decimal(38, 1) with decimal(38, 37) - ERROR
+ //
+ // We need to create a new sorted list instead of mutating it when sorting it because
+ // mutating the original exprs will change the order of the original exprs.
+ List<Expr> sortedExprs = new ArrayList<>(exprs);
+ Collections.sort(sortedExprs, new Comparator<Expr>() {
+ @Override
+ public int compare(Expr expr1, Expr expr2) {
+ if ((expr1.getType().isDecimal() && expr2.getType().isDecimal()) ||
+ (!expr1.getType().isDecimal() && !expr2.getType().isDecimal())) {
+ return 0;
+ }
+ return expr1.getType().isDecimal() ? -1 : 1;
+ }
+ });
+ Expr lastCompatibleExpr = sortedExprs.get(0);
Type compatibleType = null;
- for (int i = 0; i < exprs.size(); ++i) {
- exprs.get(i).analyze(this);
+ for (int i = 0; i < sortedExprs.size(); ++i) {
+ sortedExprs.get(i).analyze(this);
compatibleType = getCompatibleType(compatibleType, lastCompatibleExpr,
- exprs.get(i));
+ sortedExprs.get(i));
}
// Add implicit casts if necessary.
for (int i = 0; i < exprs.size(); ++i) {
http://git-wip-us.apache.org/repos/asf/impala/blob/f1643662/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
----------------------------------------------------------------------
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 a30d98e..76f979c 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
@@ -2443,6 +2443,70 @@ public class AnalyzeExprsTest extends AnalyzerTest {
AnalysisError("select " + decimal_5_5 + " > 'cast(1 as timestamp)'",
"operands of type DECIMAL(5,5) and STRING are not comparable: "
+ "CAST(1 AS DECIMAL(5,5)) > 'cast(1 as timestamp)'");
+
+ // IMPALA-7254: Inconsistent decimal behavior when finding a compatible type.
+ // for casting.
+ // In predicate that does not result in a new precision.
+ AnalyzesOk("select cast(2 as double) in " +
+ "(cast(2 as decimal(38, 37)), cast(3 as decimal(38, 37)))");
+ AnalyzesOk("select cast(2 as decimal(38, 37)) in " +
+ "(cast(2 as double), cast(3 as decimal(38, 37)))");
+ AnalyzesOk("select cast(2 as decimal(38, 37)) in " +
+ "(cast(2 as decimal(38, 37)), cast(3 as double))");
+ // In predicate that results in a precision.
+ AnalyzesOk("select cast(2 as double) in " +
+ "(cast(2 as decimal(5, 3)), cast(3 as decimal(10, 5)))");
+ AnalyzesOk("select cast(2 as decimal(5, 3)) in " +
+ "(cast(2 as double), cast(3 as decimal(10, 5)))");
+ AnalyzesOk("select cast(2 as decimal(5, 3)) in " +
+ "(cast(2 as decimal(10, 5)), cast(3 as double))");
+ // In predicate that results in a loss of precision.
+ AnalysisError("select cast(2 as decimal(38, 37)) in (cast(2 as decimal(38, 1)))",
+ "Incompatible return types 'DECIMAL(38,37)' and 'DECIMAL(38,1)' of exprs " +
+ "'CAST(2 AS DECIMAL(38,37))' and 'CAST(2 AS DECIMAL(38,1))'");
+ AnalysisError("select cast(2 as double) in " +
+ "(cast(2 as decimal(38, 37)), cast(3 as decimal(38, 1)))",
+ "Incompatible return types 'DECIMAL(38,37)' and 'DECIMAL(38,1)' of exprs " +
+ "'CAST(2 AS DECIMAL(38,37))' and 'CAST(3 AS DECIMAL(38,1))'");
+ AnalysisError("select cast(2 as decimal(38, 37)) in " +
+ "(cast(2 as double), cast(3 as decimal(38, 1)))",
+ "Incompatible return types 'DECIMAL(38,37)' and 'DECIMAL(38,1)' of exprs " +
+ "'CAST(2 AS DECIMAL(38,37))' and 'CAST(3 AS DECIMAL(38,1))'");
+ AnalysisError("select cast(2 as decimal(38, 37)) in " +
+ "(cast(2 as decimal(38, 1)), cast(3 as double))",
+ "Incompatible return types 'DECIMAL(38,37)' and 'DECIMAL(38,1)' of exprs " +
+ "'CAST(2 AS DECIMAL(38,37))' and 'CAST(2 AS DECIMAL(38,1))'");
+ // Between predicate that does not result in a new precision.
+ AnalyzesOk("select cast(2 as double) between " +
+ "cast(2 as decimal(38, 37)) and cast(3 as decimal(38, 37))");
+ AnalyzesOk("select cast(2 as decimal(38, 37)) between " +
+ "cast(2 as double) and cast(3 as decimal(38, 37))");
+ AnalyzesOk("select cast(2 as decimal(38, 37)) between " +
+ "cast(2 as decimal(38, 37)) and cast(3 as double)");
+ // Between predicate that results in a new precision.
+ AnalyzesOk("select cast(2 as double) between " +
+ "cast(2 as decimal(5, 3)) and cast(3 as decimal(10, 5))");
+ AnalyzesOk("select cast(2 as decimal(5, 3)) between " +
+ "cast(2 as double) and cast(3 as decimal(10, 5))");
+ AnalyzesOk("select cast(2 as decimal(5, 3)) between " +
+ "cast(2 as decimal(10, 5)) and cast(3 as double)");
+ // Between predicate that results in a loss of precision.
+ AnalysisError("select cast(2 as decimal(38, 37)) between " +
+ "cast(2 as decimal(38, 1)) and cast(3 as decimal(38, 3))",
+ "Incompatible return types 'DECIMAL(38,37)' and 'DECIMAL(38,1)' of exprs " +
+ "'CAST(2 AS DECIMAL(38,37))' and 'CAST(2 AS DECIMAL(38,1))'");
+ AnalysisError("select cast(2 as double) between " +
+ "cast(2 as decimal(38, 37)) and cast(3 as decimal(38, 1))",
+ "Incompatible return types 'DECIMAL(38,37)' and 'DECIMAL(38,1)' of exprs " +
+ "'CAST(2 AS DECIMAL(38,37))' and 'CAST(3 AS DECIMAL(38,1))'");
+ AnalysisError("select cast(2 as decimal(38, 37)) between " +
+ "cast(2 as double) and cast(3 as decimal(38, 1))",
+ "Incompatible return types 'DECIMAL(38,37)' and 'DECIMAL(38,1)' of exprs " +
+ "'CAST(2 AS DECIMAL(38,37))' and 'CAST(3 AS DECIMAL(38,1))'");
+ AnalysisError("select cast(2 as decimal(38, 37)) between " +
+ "cast(2 as decimal(38, 1)) and cast(3 as double)",
+ "Incompatible return types 'DECIMAL(38,37)' and 'DECIMAL(38,1)' of exprs " +
+ "'CAST(2 AS DECIMAL(38,37))' and 'CAST(2 AS DECIMAL(38,1))'");
}
@Test