From 32ee2557031adee9e8dd79ccc81ebfec49796d58 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 4 Jun 2020 00:17:35 -1000 Subject: [PATCH 1/5] fix groupBy with literal in subquery grouping --- .../druid/sql/calcite/rel/DruidQuery.java | 7 ++ .../druid/sql/calcite/CalciteQueryTest.java | 69 +++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java index b25bfb823307..408af130ddaa 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java @@ -424,6 +424,8 @@ private static List computeDimensions( /** * Builds a {@link Subtotals} object based on {@link Aggregate#getGroupSets()}. + * + * @throws CannotBuildQueryException if subtotals cannot be computed */ private static Subtotals computeSubtotals( final PartialDruidQuery partialQuery, @@ -432,10 +434,15 @@ private static Subtotals computeSubtotals( { final Aggregate aggregate = partialQuery.getAggregate(); + // dimBitMapping maps from input field position to group set position (dimension number). final int[] dimBitMapping = new int[rowSignature.size()]; int i = 0; for (int dimBit : aggregate.getGroupSet()) { + // subtotals cannot be computed since grouping field is not contained in rowSignature + if (dimBit >= rowSignature.size()) { + throw new CannotBuildQueryException(aggregate); + } dimBitMapping[dimBit] = i++; } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index 8068a0d9e38b..a932507fbec4 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -70,6 +70,7 @@ import org.apache.druid.query.aggregation.last.LongLastAggregatorFactory; import org.apache.druid.query.aggregation.last.StringLastAggregatorFactory; import org.apache.druid.query.aggregation.post.ArithmeticPostAggregator; +import org.apache.druid.query.aggregation.post.ExpressionPostAggregator; import org.apache.druid.query.aggregation.post.FieldAccessPostAggregator; import org.apache.druid.query.aggregation.post.FinalizingFieldAccessPostAggregator; import org.apache.druid.query.dimension.DefaultDimensionSpec; @@ -13419,6 +13420,74 @@ public void testNvlColumns() throws Exception ); } + @Test + public void testGroupByWithLiteralInSubqueryGrouping() throws Exception + { + testQuery( + "SELECT \n" + + " t1, t2\n" + + " FROM\n" + + " ( SELECT\n" + + " 'dummy' as t1,\n" + + " CASE\n" + + " WHEN \n" + + " dim4 = 'b'\n" + + " THEN dim4\n" + + " ELSE NULL\n" + + " END AS t2\n" + + " FROM\n" + + " numfoo\n" + + " GROUP BY\n" + + " dim4\n" + + " )\n" + + " GROUP BY\n" + + " t1,t2\n", + ImmutableList.of( + GroupByQuery.builder() + .setDataSource( + GroupByQuery.builder() + .setDataSource(CalciteTests.DATASOURCE3) + .setInterval(querySegmentSpec(Filtration.eternity())) + .setGranularity(Granularities.ALL) + .setDimensions(new DefaultDimensionSpec("dim4", "_d0", ValueType.STRING)) + .setPostAggregatorSpecs( + ImmutableList.of( + expressionPostAgg( + "p0", + "\'dummy\'" + ), + expressionPostAgg( + "p1", + "case_searched((\"_d0\" == 'b'),\"_d0\",null)" + ) + ) + ) + .setContext(QUERY_CONTEXT_DEFAULT) + .build() + ) + .setInterval(querySegmentSpec(Filtration.eternity())) + .setDimensions( + dimensions( + new DefaultDimensionSpec("p0", "d0", ValueType.STRING), + new DefaultDimensionSpec("p1", "d1", ValueType.STRING) + ) + ) + .setGranularity(Granularities.ALL) + .setContext(QUERY_CONTEXT_DEFAULT) + .build() + ), + NullHandling.replaceWithDefault() ? + ImmutableList.of( + new Object[]{"dummy", ""}, + new Object[]{"dummy", "b"} + ) : + ImmutableList.of( + new Object[]{"dummy", null}, + new Object[]{"dummy", "b"} + ) + ); + } + @Test public void testMultiValueStringWorksLikeStringGroupBy() throws Exception { From 777a92230df7e5ab3890df4cefe285792f571aba Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 4 Jun 2020 10:21:50 -1000 Subject: [PATCH 2/5] fix groupBy with literal in subquery grouping --- .../druid/sql/calcite/rel/DruidQuery.java | 15 +++++----- .../druid/sql/calcite/CalciteQueryTest.java | 29 +++++++++---------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java index 408af130ddaa..03d7c63c4032 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java @@ -191,7 +191,7 @@ public static DruidQuery fromPartialQuery( computeGrouping( partialQuery, plannerContext, - computeOutputRowSignature(sourceRowSignature, selectProjection, null, null), + computeOutputRowSignature(sourceRowSignature, null, null, null), virtualColumnRegistry, rexBuilder, finalizeAggregations @@ -434,15 +434,16 @@ private static Subtotals computeSubtotals( { final Aggregate aggregate = partialQuery.getAggregate(); - // dimBitMapping maps from input field position to group set position (dimension number). - final int[] dimBitMapping = new int[rowSignature.size()]; + final int[] dimBitMapping; + if (partialQuery.getSelectProject() != null) { + dimBitMapping = new int[partialQuery.getSelectProject().getRowType().getFieldCount()]; + } else { + dimBitMapping = new int[rowSignature.size()]; + } + int i = 0; for (int dimBit : aggregate.getGroupSet()) { - // subtotals cannot be computed since grouping field is not contained in rowSignature - if (dimBit >= rowSignature.size()) { - throw new CannotBuildQueryException(aggregate); - } dimBitMapping[dimBit] = i++; } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index a932507fbec4..c96724b2e249 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -13450,26 +13450,25 @@ public void testGroupByWithLiteralInSubqueryGrouping() throws Exception .setInterval(querySegmentSpec(Filtration.eternity())) .setGranularity(Granularities.ALL) .setDimensions(new DefaultDimensionSpec("dim4", "_d0", ValueType.STRING)) - .setPostAggregatorSpecs( - ImmutableList.of( - expressionPostAgg( - "p0", - "\'dummy\'" - ), - expressionPostAgg( - "p1", - "case_searched((\"_d0\" == 'b'),\"_d0\",null)" - ) - ) - ) .setContext(QUERY_CONTEXT_DEFAULT) .build() ) - .setInterval(querySegmentSpec(Filtration.eternity())) + .setVirtualColumns( + expressionVirtualColumn( + "v0", + "\'dummy\'", + ValueType.STRING + ), + expressionVirtualColumn( + "v1", + "case_searched((\"_d0\" == 'b'),\"_d0\",null)", + ValueType.STRING + ) + ) .setInterval(querySegmentSpec(Filtration.eternity())) .setDimensions( dimensions( - new DefaultDimensionSpec("p0", "d0", ValueType.STRING), - new DefaultDimensionSpec("p1", "d1", ValueType.STRING) + new DefaultDimensionSpec("v0", "d0", ValueType.STRING), + new DefaultDimensionSpec("v1", "d1", ValueType.STRING) ) ) .setGranularity(Granularities.ALL) From a3e15fd2c021bbe3fa0869c61b80cebad0338bae Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 4 Jun 2020 10:28:37 -1000 Subject: [PATCH 3/5] fix groupBy with literal in subquery grouping --- .../druid/sql/calcite/rel/DruidQuery.java | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java index 03d7c63c4032..03673671fe37 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java @@ -317,6 +317,7 @@ private static Grouping computeGrouping( final Subtotals subtotals = computeSubtotals( partialQuery, + plannerContext, rowSignature ); @@ -429,6 +430,7 @@ private static List computeDimensions( */ private static Subtotals computeSubtotals( final PartialDruidQuery partialQuery, + final PlannerContext plannerContext, final RowSignature rowSignature ) { @@ -437,7 +439,21 @@ private static Subtotals computeSubtotals( // dimBitMapping maps from input field position to group set position (dimension number). final int[] dimBitMapping; if (partialQuery.getSelectProject() != null) { - dimBitMapping = new int[partialQuery.getSelectProject().getRowType().getFieldCount()]; + int fieldCount = 0; + for (final RexNode rexNode : partialQuery.getSelectProject().getChildExps()) { + final DruidExpression expression = Expressions.toDruidExpression( + plannerContext, + rowSignature, + rexNode + ); + + if (expression == null) { + throw new CannotBuildQueryException(partialQuery.getSelectProject(), rexNode); + } else { + fieldCount++; + } + } + dimBitMapping = new int[fieldCount]; } else { dimBitMapping = new int[rowSignature.size()]; } From 19dd058230dd222d2f236ccd9e3fe386f6121b6a Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 4 Jun 2020 10:47:53 -1000 Subject: [PATCH 4/5] address comments --- .../druid/sql/calcite/rel/DruidQuery.java | 18 +----------------- .../druid/sql/calcite/CalciteQueryTest.java | 10 +++------- 2 files changed, 4 insertions(+), 24 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java index 03673671fe37..03d7c63c4032 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java @@ -317,7 +317,6 @@ private static Grouping computeGrouping( final Subtotals subtotals = computeSubtotals( partialQuery, - plannerContext, rowSignature ); @@ -430,7 +429,6 @@ private static List computeDimensions( */ private static Subtotals computeSubtotals( final PartialDruidQuery partialQuery, - final PlannerContext plannerContext, final RowSignature rowSignature ) { @@ -439,21 +437,7 @@ private static Subtotals computeSubtotals( // dimBitMapping maps from input field position to group set position (dimension number). final int[] dimBitMapping; if (partialQuery.getSelectProject() != null) { - int fieldCount = 0; - for (final RexNode rexNode : partialQuery.getSelectProject().getChildExps()) { - final DruidExpression expression = Expressions.toDruidExpression( - plannerContext, - rowSignature, - rexNode - ); - - if (expression == null) { - throw new CannotBuildQueryException(partialQuery.getSelectProject(), rexNode); - } else { - fieldCount++; - } - } - dimBitMapping = new int[fieldCount]; + dimBitMapping = new int[partialQuery.getSelectProject().getRowType().getFieldCount()]; } else { dimBitMapping = new int[rowSignature.size()]; } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index c96724b2e249..97e644d397cb 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -13464,7 +13464,8 @@ public void testGroupByWithLiteralInSubqueryGrouping() throws Exception "case_searched((\"_d0\" == 'b'),\"_d0\",null)", ValueType.STRING ) - ) .setInterval(querySegmentSpec(Filtration.eternity())) + ) + .setInterval(querySegmentSpec(Filtration.eternity())) .setDimensions( dimensions( new DefaultDimensionSpec("v0", "d0", ValueType.STRING), @@ -13475,13 +13476,8 @@ public void testGroupByWithLiteralInSubqueryGrouping() throws Exception .setContext(QUERY_CONTEXT_DEFAULT) .build() ), - NullHandling.replaceWithDefault() ? - ImmutableList.of( - new Object[]{"dummy", ""}, - new Object[]{"dummy", "b"} - ) : ImmutableList.of( - new Object[]{"dummy", null}, + new Object[]{"dummy", NULL_STRING}, new Object[]{"dummy", "b"} ) ); From 8efcb5b725207875b147b4ae62e149c7b09c5932 Mon Sep 17 00:00:00 2001 From: Maytas Monsereenusorn Date: Thu, 4 Jun 2020 12:02:30 -1000 Subject: [PATCH 5/5] update javadocs --- .../main/java/org/apache/druid/query/QueryToolChest.java | 9 ++++----- .../org/apache/druid/sql/calcite/rel/DruidQuery.java | 2 -- .../org/apache/druid/sql/calcite/CalciteQueryTest.java | 1 - 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/QueryToolChest.java b/processing/src/main/java/org/apache/druid/query/QueryToolChest.java index 509738ee6803..ee400f814fb8 100644 --- a/processing/src/main/java/org/apache/druid/query/QueryToolChest.java +++ b/processing/src/main/java/org/apache/druid/query/QueryToolChest.java @@ -179,11 +179,10 @@ public abstract Function makePreComputeManipulatorFn( ); /** - * Generally speaking this is the exact same thing as makePreComputeManipulatorFn. It is leveraged in - * order to compute PostAggregators on results after they have been completely merged together, which - * should actually be done in the mergeResults() call instead of here. - *

- * This should never actually be overridden and it should be removed as quickly as possible. + * Generally speaking this is the exact same thing as makePreComputeManipulatorFn. It is leveraged in order to + * compute PostAggregators on results after they have been completely merged together. To minimize walks of segments, + * it is recommended to use mergeResults() call instead of this method if possible. However, this may not always be + * possible as we don’t always want to run PostAggregators and other stuff that happens there when you mergeResults. * * @param query The Query that is currently being processed * @param fn The function that should be applied to all metrics in the results diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java index 03d7c63c4032..137f9b70bd38 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java @@ -424,8 +424,6 @@ private static List computeDimensions( /** * Builds a {@link Subtotals} object based on {@link Aggregate#getGroupSets()}. - * - * @throws CannotBuildQueryException if subtotals cannot be computed */ private static Subtotals computeSubtotals( final PartialDruidQuery partialQuery, diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java index 97e644d397cb..e3579aad6a94 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java @@ -70,7 +70,6 @@ import org.apache.druid.query.aggregation.last.LongLastAggregatorFactory; import org.apache.druid.query.aggregation.last.StringLastAggregatorFactory; import org.apache.druid.query.aggregation.post.ArithmeticPostAggregator; -import org.apache.druid.query.aggregation.post.ExpressionPostAggregator; import org.apache.druid.query.aggregation.post.FieldAccessPostAggregator; import org.apache.druid.query.aggregation.post.FinalizingFieldAccessPostAggregator; import org.apache.druid.query.dimension.DefaultDimensionSpec;