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/12 05:23:55 UTC
impala git commit: IMPALA-7211: Fix the between predicate for decimals
Repository: impala
Updated Branches:
refs/heads/master e8a669bf9 -> 80c009631
IMPALA-7211: Fix the between predicate for decimals
Before this patch, some queries would fail where the inputs to the
between predicate were decimal types that are not compatible with each
other. We would needlessly cast them to the same type in the FE. This
was not necessary because we are able to handle decimals of different
types in the backend already for this predicate. This patch should even
result in a slight performance improvement.
Testing:
- Added some tests to AnalyzeExprsTest.
Change-Id: Iac89d62082052b41bfa698e4de09aec26df5c312
Reviewed-on: http://gerrit.cloudera.org:8080/10898
Reviewed-by: Vuk Ercegovac <ve...@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/80c00963
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/80c00963
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/80c00963
Branch: refs/heads/master
Commit: 80c009631e6018a64806233a31a327d33b4e4b49
Parents: e8a669b
Author: Taras Bobrovytsky <ta...@apache.org>
Authored: Thu Jul 5 19:38:59 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Thu Jul 12 04:04:39 2018 +0000
----------------------------------------------------------------------
.../org/apache/impala/analysis/Analyzer.java | 26 ++------
.../impala/analysis/BetweenPredicate.java | 31 ++++++++-
.../impala/analysis/AnalyzeExprsTest.java | 68 +++++++-------------
3 files changed, 62 insertions(+), 63 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/80c00963/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 0dd68e9..242df27 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
@@ -2261,24 +2261,13 @@ public class Analyzer {
/**
* Determines compatible type for given exprs, and casts them to compatible type.
* Calls analyze() on each of the exprs.
- * Throw an AnalysisException if the types are incompatible,
- * returns compatible type otherwise.
+ * Throws an AnalysisException if the types are incompatible,
*/
- public Type castAllToCompatibleType(List<Expr> exprs) throws AnalysisException {
- // 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
+ public void castAllToCompatibleType(List<Expr> exprs) throws AnalysisException {
+ // Group all the decimal types together at the end of the list to avoid comparing
+ // the decimals with each other first. For example, if we have the following list,
+ // [decimal, decimal, double], we will end up casting everything to a double anyways,
+ // so it does not matter if the decimals are not compatible with each other.
//
// 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.
@@ -2290,7 +2279,7 @@ public class Analyzer {
(!expr1.getType().isDecimal() && !expr2.getType().isDecimal())) {
return 0;
}
- return expr1.getType().isDecimal() ? -1 : 1;
+ return expr1.getType().isDecimal() ? 1 : -1;
}
});
Expr lastCompatibleExpr = sortedExprs.get(0);
@@ -2307,7 +2296,6 @@ public class Analyzer {
exprs.set(i, castExpr);
}
}
- return compatibleType;
}
/**
http://git-wip-us.apache.org/repos/asf/impala/blob/80c00963/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java b/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
index 269217c..f51aef3 100644
--- a/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
+++ b/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
@@ -17,6 +17,8 @@
package org.apache.impala.analysis;
+import org.apache.impala.catalog.ScalarType;
+import org.apache.impala.catalog.Type;
import org.apache.impala.common.AnalysisException;
import org.apache.impala.thrift.TExprNode;
@@ -46,6 +48,23 @@ public class BetweenPredicate extends Predicate {
isNotBetween_ = other.isNotBetween_;
}
+ /* Returns true if all the children should be cast to decimal. */
+ private boolean checkDecimalCast() {
+ boolean allScalar = true;
+ // If there is at least one float, then all the children need to be cast to
+ // float (instead of decimal).
+ boolean noFloats = true;
+ boolean atLeastOneDecimal = false;
+ for(int i = 0; i < children_.size(); ++i) {
+ if (!children_.get(i).getType().isScalarType()) allScalar = false;
+ // If at least one child is a float, then we want to cast all the children to float.
+ if (children_.get(i).getType().isFloatingPointType()) noFloats = false;
+ if (children_.get(i).getType().isDecimal()) atLeastOneDecimal = true;
+ }
+
+ return allScalar && noFloats && atLeastOneDecimal;
+ }
+
@Override
protected void analyzeImpl(Analyzer analyzer) throws AnalysisException {
super.analyzeImpl(analyzer);
@@ -54,7 +73,17 @@ public class BetweenPredicate extends Predicate {
throw new AnalysisException("Comparison between subqueries is not " +
"supported in a BETWEEN predicate: " + toSqlImpl());
}
- analyzer.castAllToCompatibleType(children_);
+
+ if (checkDecimalCast()) {
+ for(int i = 0; i < children_.size(); ++i) {
+ ScalarType t = (ScalarType) children_.get(i).getType();
+ // The backend function can handle decimals of different precision and scale, so
+ // it is ok if the children don't have the same decimal type.
+ children_.get(i).castTo(t.getMinResolutionDecimal());
+ }
+ } else {
+ analyzer.castAllToCompatibleType(children_);
+ }
}
@Override
http://git-wip-us.apache.org/repos/asf/impala/blob/80c00963/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 df0c4e7..791883b 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
@@ -729,6 +729,25 @@ public class AnalyzeExprsTest extends AnalyzerTest {
"where int_struct_col between 10 and 20",
"Incompatible return types 'STRUCT<f1:INT,f2:INT>' and 'TINYINT' " +
"of exprs 'int_struct_col' and '10'.");
+ // IMPALA-7211: Do not cast decimal types to other decimal types
+ AnalyzesOk("select cast(1 as decimal(38,2)) between " +
+ "0.9 * cast(1 as decimal(38,3)) and 3");
+ AnalyzesOk("select cast(2 as decimal(38,37)) between " +
+ "cast(1 as decimal(38,1)) and cast(3 as decimal(38,15))");
+ AnalyzesOk("select cast(2 as decimal(38,37)) between " +
+ "cast(1 as double) and cast(3 as decimal(38,1))");
+ AnalyzesOk("select cast(2 as decimal(38,37)) between " +
+ "cast(1 as decimal(38,1)) and cast(3 as double)");
+ AnalyzesOk("select cast(2 as decimal(38,37)) between " +
+ "cast(1 as decimal(38,1)) and cast(3 as bigint)");
+ AnalyzesOk("select cast(2 as decimal(38,37)) between " +
+ "cast(1 as bigint) and cast(3 as bigint)");
+ AnalyzesOk("select cast(2 as decimal(38,37)) between " +
+ "cast(1 as bigint) and cast(3 as decimal(38,1))");
+ AnalyzesOk("select cast(2 as decimal(38,37)) between " +
+ "cast(1 as decimal(38,1)) and cast(3 as int)");
+ AnalyzesOk("select cast(2 as decimal(38,37)) between " +
+ "cast(1 as int) and cast(3 as decimal(38,1))");
}
@Test
@@ -2473,49 +2492,12 @@ public class AnalyzeExprsTest extends AnalyzerTest {
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))'");
+ AnalyzesOk("select cast(2 as double) in " +
+ "(cast(2 as decimal(38, 37)), cast(3 as decimal(38, 1)))");
+ AnalyzesOk("select cast(2 as decimal(38, 37)) in " +
+ "(cast(2 as double), cast(3 as decimal(38, 1)))");
+ AnalyzesOk("select cast(2 as decimal(38, 37)) in " +
+ "(cast(2 as decimal(38, 1)), cast(3 as double))");
}
@Test