IMPALA-6323 Allow constant analytic window expressions. The constraint imposed by IMPALA-1354 was artificial. If there are constant "partition by" expressions, simply drop them, they are no-ops.
Constant "order by" expressions can be ignored as well, though in effect they should be accounted for as null expressions in the backend, with the effect that combine all rows in the same window (i.e. no window breaks). Change-Id: Idf129026c45120e9470df601268863634037908c Reviewed-on: http://gerrit.cloudera.org:8080/11556 Tested-by: Impala Public Jenkins <[email protected]> Reviewed-by: Michael Ho <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/0bc54468 Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/0bc54468 Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/0bc54468 Branch: refs/heads/branch-3.1.0 Commit: 0bc544685e6d0d13f8800552a62bc35e1ce8e565 Parents: 0a7da0f Author: Michal Ostrowski <[email protected]> Authored: Fri Sep 28 11:39:11 2018 -0700 Committer: Zoltan Borok-Nagy <[email protected]> Committed: Tue Nov 13 12:51:39 2018 +0100 ---------------------------------------------------------------------- be/src/exec/analytic-eval-node.cc | 3 +- .../apache/impala/analysis/AnalyticExpr.java | 12 ++------ .../apache/impala/planner/AnalyticPlanner.java | 23 ++++++++++++---- .../impala/analysis/AnalyzeExprsTest.java | 15 ++++------ .../queries/PlannerTest/analytic-fns.test | 29 ++++++++++++++++++++ .../queries/QueryTest/analytic-fns.test | 21 ++++++++++++++ 6 files changed, 77 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/0bc54468/be/src/exec/analytic-eval-node.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/analytic-eval-node.cc b/be/src/exec/analytic-eval-node.cc index 036cf93..c7bcbda 100644 --- a/be/src/exec/analytic-eval-node.cc +++ b/be/src/exec/analytic-eval-node.cc @@ -394,7 +394,8 @@ inline Status AnalyticEvalNode::TryAddResultTupleForPrevRow( << " idx=" << stream_idx; if (fn_scope_ != ROWS && (next_partition || (fn_scope_ == RANGE && window_.__isset.window_end - && !PrevRowCompare(order_by_eq_expr_eval_, child_tuple_cmp_row)))) { + && !(order_by_eq_expr_eval_ == nullptr || + PrevRowCompare(order_by_eq_expr_eval_, child_tuple_cmp_row))))) { RETURN_IF_ERROR(AddResultTuple(stream_idx - 1)); } return Status::OK(); http://git-wip-us.apache.org/repos/asf/impala/blob/0bc54468/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java b/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java index d033ee1..76fc81f 100644 --- a/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java +++ b/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java @@ -424,22 +424,14 @@ public class AnalyticExpr extends Expr { type_ = getFnCall().getType(); for (Expr e: partitionExprs_) { - if (e.isConstant()) { - throw new AnalysisException( - "Expressions in the PARTITION BY clause must not be constant: " - + e.toSql() + " (in " + toSql() + ")"); - } else if (e.getType().isComplexType()) { + if (e.getType().isComplexType()) { throw new AnalysisException(String.format("PARTITION BY expression '%s' with " + "complex type '%s' is not supported.", e.toSql(), e.getType().toSql())); } } for (OrderByElement e: orderByElements_) { - if (e.getExpr().isConstant()) { - throw new AnalysisException( - "Expressions in the ORDER BY clause must not be constant: " - + e.getExpr().toSql() + " (in " + toSql() + ")"); - } else if (e.getExpr().getType().isComplexType()) { + if (e.getExpr().getType().isComplexType()) { throw new AnalysisException(String.format("ORDER BY expression '%s' with " + "complex type '%s' is not supported.", e.getExpr().toSql(), e.getExpr().getType().toSql())); http://git-wip-us.apache.org/repos/asf/impala/blob/0bc54468/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java b/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java index 90d98fd..3685bf4 100644 --- a/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java +++ b/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java @@ -82,6 +82,18 @@ public class AnalyticPlanner { } /** + * Return true if and only if exprs is non-empty and contains non-constant + * expressions. + */ + private boolean activeExprs(List<Expr> exprs) { + if (exprs.isEmpty()) return false; + for (Expr p: exprs) { + if (!p.isConstant()) { return true; } + } + return false; + } + + /** * Return plan tree that augments 'root' with plan nodes that implement single-node * evaluation of the AnalyticExprs in analyticInfo. * This plan takes into account a possible hash partition of its input on @@ -224,7 +236,7 @@ public class AnalyticPlanner { // remove the non-partitioning group from partitionGroups PartitionGroup nonPartitioning = null; for (PartitionGroup pg: partitionGroups) { - if (pg.partitionByExprs.isEmpty()) { + if (!activeExprs(pg.partitionByExprs)) { nonPartitioning = pg; break; } @@ -308,9 +320,10 @@ public class AnalyticPlanner { TupleDescriptor bufferedTupleDesc = null; // map from input to buffered tuple ExprSubstitutionMap bufferedSmap = new ExprSubstitutionMap(); + boolean activePartition = activeExprs(partitionByExprs); // sort on partition by (pb) + order by (ob) exprs and create pb/ob predicates - if (!partitionByExprs.isEmpty() || !orderByElements.isEmpty()) { + if (activePartition || !orderByElements.isEmpty()) { // first sort on partitionExprs (direction doesn't matter) List<Expr> sortExprs = Lists.newArrayList(partitionByExprs); List<Boolean> isAsc = @@ -337,12 +350,12 @@ public class AnalyticPlanner { // if this sort group does not have partitioning exprs, we want the sort // to be executed like a regular distributed sort - if (!partitionByExprs.isEmpty()) sortNode.setIsAnalyticSort(true); + if (activePartition) sortNode.setIsAnalyticSort(true); if (partitionExprs != null) { // create required input partition DataPartition inputPartition = DataPartition.UNPARTITIONED; - if (!partitionExprs.isEmpty()) { + if (activePartition) { inputPartition = DataPartition.hashPartitioned(partitionExprs); } sortNode.setInputPartition(inputPartition); @@ -380,7 +393,7 @@ public class AnalyticPlanner { // we need to remap the pb/ob exprs to a) the sort output, b) our buffer of the // sort input Expr partitionByEq = null; - if (!windowGroup.partitionByExprs.isEmpty()) { + if (activeExprs(windowGroup.partitionByExprs)) { partitionByEq = createNullMatchingEquals( Expr.substituteList(windowGroup.partitionByExprs, sortSmap, analyzer_, false), sortTupleId, bufferedSmap); http://git-wip-us.apache.org/repos/asf/impala/blob/0bc54468/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 1908c21..b547526 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java @@ -1005,16 +1005,11 @@ public class AnalyzeExprsTest extends AnalyzerTest { "(first_value(cast(int_col AS DECIMAL)) " + " over (order by int_col rows between 2 preceding and 1 preceding)) " + "from functional.alltypestiny"); - // IMPALA-1354: Constant expressions in order by and partition by exprs - AnalysisError( - "select rank() over (order by 1) from functional.alltypestiny", - "Expressions in the ORDER BY clause must not be constant: 1"); - AnalysisError( - "select rank() over (partition by 2 order by id) from functional.alltypestiny", - "Expressions in the PARTITION BY clause must not be constant: 2"); - AnalysisError( - "select rank() over (partition by 2 order by 1) from functional.alltypestiny", - "Expressions in the PARTITION BY clause must not be constant: 2"); + // IMPALA-6323: Allow constant expressions in analytic window exprs. + AnalyzesOk("select rank() over (order by 1) from functional.alltypestiny"); + AnalyzesOk("select count() over (partition by 2) from functional.alltypestiny"); + AnalyzesOk( + "select rank() over (partition by 2 order by 1) from functional.alltypestiny"); // nested analytic exprs AnalysisError( http://git-wip-us.apache.org/repos/asf/impala/blob/0bc54468/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test b/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test index f78bfb2..5084d2a 100644 --- a/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test +++ b/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test @@ -2457,3 +2457,32 @@ PLAN-ROOT SINK 00:SCAN HDFS [functional.alltypestiny] partitions=4/4 files=4 size=460B ==== +# IMPALA-6323 Partition by a constant is equivalent to no partitioning. +select x, count() over(partition by 1) from (VALUES((1 x), (2), (3))) T; +---- DISTRIBUTEDPLAN +PLAN-ROOT SINK +| +01:ANALYTIC +| functions: count() +| partition by: 1 +| +00:UNION + constant-operands=3 +==== +# IMPALA-6323 Order by a constant is equivalent to no ordering. +select x, count() over(order by 1) from (VALUES((1 x), (2), (3))) T; +---- DISTRIBUTEDPLAN +PLAN-ROOT SINK +| +02:ANALYTIC +| functions: count() +| order by: 1 ASC +| window: RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW +| +01:SORT +| order by: 1 ASC +| +00:UNION + constant-operands=3 +==== + http://git-wip-us.apache.org/repos/asf/impala/blob/0bc54468/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test b/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test index 0c6207c..fbbd939 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test +++ b/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test @@ -2119,3 +2119,24 @@ true,6,2 true,4,2 true,2,2 true,NULL,2 +==== +---- QUERY +# IMPALA-6323 Partition by a constant is equivalent to no partitioning. +select x, count() over(partition by 1) from (VALUES((1 x), (2), (3))) T; +---- TYPES +TINYINT, BIGINT +---- RESULTS +1,3 +2,3 +3,3 +==== +---- QUERY +# IMPALA-6323 Order by a constant is equivalent to no ordering. +select x, count() over(order by 1) from (VALUES((1 x), (2), (3))) T; +---- TYPES +TINYINT, BIGINT +---- RESULTS +1,3 +2,3 +3,3 +====
