From 2b6c424f060d0dd618ae7ae5f2437cf42db4ac51 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Sat, 1 Jun 2024 13:25:40 +0530 Subject: [PATCH 1/7] update dims order in case limit spec with order by columns --- .../druid/query/groupby/GroupingEngine.java | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index 6451fb9b943d..2b2e82214896 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -686,13 +686,27 @@ public Sequence processSubtotalsSpec( processingConfig.intermediateComputeSizeBytes() ); - List queryDimNames = baseSubtotalQuery.getDimensions().stream().map(DimensionSpec::getOutputName) + List queryDimNamesInOrder = baseSubtotalQuery.getDimensions().stream().map(DimensionSpec::getOutputName) .collect(Collectors.toList()); // Only needed to make LimitSpec.filterColumns(..) call later in case base query has a non default LimitSpec. Set aggsAndPostAggs = null; if (!(baseSubtotalQuery.getLimitSpec() instanceof NoopLimitSpec)) { aggsAndPostAggs = getAggregatorAndPostAggregatorNames(baseSubtotalQuery); + + DefaultLimitSpec limitSpec = (DefaultLimitSpec) baseSubtotalQuery.getLimitSpec(); + if (!limitSpec.getColumns().isEmpty()) { + Map dimToOutputNames = baseSubtotalQuery.getDimensions() + .stream() + .collect(Collectors.toMap( + DimensionSpec::getDimension, + DimensionSpec::getOutputName + )); + queryDimNamesInOrder = limitSpec.getColumns() + .stream() + .map(spec -> dimToOutputNames.get(spec.getDimension())) + .collect(Collectors.toList()); + } } List> subtotals = query.getSubtotalsSpec(); @@ -724,7 +738,7 @@ public Sequence processSubtotalsSpec( .withLimitSpec(subtotalQueryLimitSpec); final GroupByRowProcessor.ResultSupplier resultSupplierOneFinal = resultSupplierOne; - if (Utils.isPrefix(subtotalSpec, queryDimNames)) { + if (Utils.isPrefix(subtotalSpec, queryDimNamesInOrder)) { // Since subtotalSpec is a prefix of base query dimensions, so results from base query are also sorted // by subtotalSpec as needed by stream merging. subtotalsResults.add( From d03bfb892502a1a6671469f8c738b53772060739 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Sat, 1 Jun 2024 14:06:06 +0530 Subject: [PATCH 2/7] test case --- .../druid/query/groupby/GroupingEngine.java | 2 +- .../druid/sql/calcite/CalciteQueryTest.java | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index 2b2e82214896..9d22d84c6d3a 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -695,7 +695,7 @@ public Sequence processSubtotalsSpec( aggsAndPostAggs = getAggregatorAndPostAggregatorNames(baseSubtotalQuery); DefaultLimitSpec limitSpec = (DefaultLimitSpec) baseSubtotalQuery.getLimitSpec(); - if (!limitSpec.getColumns().isEmpty()) { + if (!limitSpec.getColumns().isEmpty() && limitSpec.isLimited()) { Map dimToOutputNames = baseSubtotalQuery.getDimensions() .stream() .collect(Collectors.toMap( 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 f21fba9ab456..e514d22e3ec8 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 @@ -16278,4 +16278,27 @@ public void testGroupingSetsWithAggrgateCase() ) ).run(); } + + @SqlTestFrameworkConfig.NumMergeBuffers(3) + @Test + public void testGroupingSetsWithDifferentOrderLimitSpec() + { + testBuilder() + .sql( + "SELECT\n" + + " isNew, isRobot, COUNT(*) AS \"Cnt\"\n" + + "FROM \"wikipedia\"\n" + + "GROUP BY GROUPING SETS ((isRobot), (isNew))\n" + + "ORDER BY 2, 1\n" + + "limit 100" + ) + .expectedResults( + ImmutableList.of( + new Object[]{"false", null, 36966L}, + new Object[]{"true", null, 2278L}, + new Object[]{null, "false", 23824L}, + new Object[]{null, "true", 15420L} + ) + ).run(); + } } From 272d6673dec399656d306ce828a349eb56e339c0 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Sat, 1 Jun 2024 15:11:21 +0530 Subject: [PATCH 3/7] test fix --- .../org/apache/druid/sql/calcite/CalciteQueryTest.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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 e514d22e3ec8..ace2a5a4bf99 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 @@ -13795,10 +13795,8 @@ public void testGroupingSetsWithLimitOrderByGran() .build() ), ImmutableList.builder().add( - new Object[]{"", null, 2L}, - new Object[]{"a", null, 1L}, - new Object[]{"", null, 1L}, - new Object[]{"a", null, 1L}, + new Object[]{"", null, 3L}, + new Object[]{"a", null, 2L}, new Object[]{"abc", null, 1L}, new Object[]{NULL_STRING, null, 6L}, new Object[]{"", timestamp("2000-01-01"), 2L}, @@ -16293,6 +16291,7 @@ public void testGroupingSetsWithDifferentOrderLimitSpec() + "limit 100" ) .expectedResults( + ResultMatchMode.RELAX_NULLS, ImmutableList.of( new Object[]{"false", null, 36966L}, new Object[]{"true", null, 2278L}, From 95d798e6281854ad603b8add68542031bba65219 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Sat, 1 Jun 2024 17:59:40 +0530 Subject: [PATCH 4/7] test fix --- .../java/org/apache/druid/sql/calcite/CalciteQueryTest.java | 2 ++ 1 file changed, 2 insertions(+) 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 ace2a5a4bf99..862b2b748710 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 @@ -16281,6 +16281,8 @@ public void testGroupingSetsWithAggrgateCase() @Test public void testGroupingSetsWithDifferentOrderLimitSpec() { + cannotVectorize(); + msqIncompatible(); testBuilder() .sql( "SELECT\n" From b24163843ca3cbcb421feefb46ad9060d24a09dd Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Wed, 12 Jun 2024 06:51:27 +0530 Subject: [PATCH 5/7] refactor --- .../druid/query/groupby/GroupByQuery.java | 55 ++++++++++++++----- .../druid/query/groupby/GroupingEngine.java | 23 ++------ .../druid/query/groupby/GroupByQueryTest.java | 2 +- 3 files changed, 47 insertions(+), 33 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java index cdcf9e3daf40..d239a0e157bf 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java @@ -468,7 +468,7 @@ public boolean getApplyLimitPushDownFromContext() @Override public Ordering getResultOrdering() { - final Ordering rowOrdering = getRowOrdering(false); + final Ordering rowOrdering = getOrderingAndDimensions(false).getRowOrdering(); return Ordering.from( (lhs, rhs) -> { @@ -565,7 +565,7 @@ private boolean canDoLimitPushDown( * limit/order spec (unlike non-push down case where the results always use the default natural ascending order), * so when merging these partial result streams, the merge needs to use the same ordering to get correct results. */ - private Ordering getRowOrderingForPushDown( + private OrderingAndDimensions getRowOrderingAndDimensionsForPushDown( final boolean granular, final DefaultLimitSpec limitSpec ) @@ -577,6 +577,7 @@ private Ordering getRowOrderingForPushDown( final List needsReverseList = new ArrayList<>(); final List dimensionTypes = new ArrayList<>(); final List comparators = new ArrayList<>(); + final List dimensionsInOrder = new ArrayList<>(); for (OrderByColumnSpec orderSpec : limitSpec.getColumns()) { boolean needsReverse = orderSpec.getDirection() != OrderByColumnSpec.Direction.ASCENDING; @@ -589,6 +590,7 @@ private Ordering getRowOrderingForPushDown( final ColumnType type = dimensions.get(dimIndex).getOutputType(); dimensionTypes.add(type); comparators.add(orderSpec.getDimensionComparator()); + dimensionsInOrder.add(dim); } } @@ -599,13 +601,14 @@ private Ordering getRowOrderingForPushDown( final ColumnType type = dimensions.get(i).getOutputType(); dimensionTypes.add(type); comparators.add(StringComparators.NATURAL); + dimensionsInOrder.add(dimensions.get(i)); } } final Comparator timeComparator = getTimeComparator(granular); if (timeComparator == null) { - return Ordering.from( + return new OrderingAndDimensions(Ordering.from( (lhs, rhs) -> compareDimsForLimitPushDown( orderedFieldNumbers, needsReverseList, @@ -614,9 +617,9 @@ private Ordering getRowOrderingForPushDown( lhs, rhs ) - ); + ), dimensionsInOrder); } else if (sortByDimsFirst) { - return Ordering.from( + return new OrderingAndDimensions(Ordering.from( (lhs, rhs) -> { final int cmp = compareDimsForLimitPushDown( orderedFieldNumbers, @@ -632,9 +635,9 @@ private Ordering getRowOrderingForPushDown( return timeComparator.compare(lhs, rhs); } - ); + ), dimensionsInOrder); } else { - return Ordering.from( + return new OrderingAndDimensions(Ordering.from( (lhs, rhs) -> { final int timeCompare = timeComparator.compare(lhs, rhs); @@ -651,15 +654,15 @@ private Ordering getRowOrderingForPushDown( rhs ); } - ); + ), dimensionsInOrder); } } - public Ordering getRowOrdering(final boolean granular) + public OrderingAndDimensions getOrderingAndDimensions(final boolean granular) { if (isApplyLimitPushDown()) { if (!DefaultLimitSpec.sortingOrderHasNonGroupingFields((DefaultLimitSpec) limitSpec, dimensions)) { - return getRowOrderingForPushDown(granular, (DefaultLimitSpec) limitSpec); + return getRowOrderingAndDimensionsForPushDown(granular, (DefaultLimitSpec) limitSpec); } } @@ -667,9 +670,9 @@ public Ordering getRowOrdering(final boolean granular) final Comparator timeComparator = getTimeComparator(granular); if (timeComparator == null) { - return Ordering.from((lhs, rhs) -> compareDims(dimensions, lhs, rhs)); + return new OrderingAndDimensions(Ordering.from((lhs, rhs) -> compareDims(dimensions, lhs, rhs)), dimensions); } else if (sortByDimsFirst) { - return Ordering.from( + return new OrderingAndDimensions(Ordering.from( (lhs, rhs) -> { final int cmp = compareDims(dimensions, lhs, rhs); if (cmp != 0) { @@ -678,9 +681,9 @@ public Ordering getRowOrdering(final boolean granular) return timeComparator.compare(lhs, rhs); } - ); + ), dimensions); } else { - return Ordering.from( + return new OrderingAndDimensions(Ordering.from( (lhs, rhs) -> { final int timeCompare = timeComparator.compare(lhs, rhs); @@ -690,7 +693,7 @@ public Ordering getRowOrdering(final boolean granular) return compareDims(dimensions, lhs, rhs); } - ); + ), dimensions); } } @@ -924,6 +927,28 @@ private static void verifyOutputNames( } } + public static class OrderingAndDimensions + { + Ordering rowOrdering; + List dimensions; + + public OrderingAndDimensions(Ordering rowOrdering, List dimensions) + { + this.rowOrdering = rowOrdering; + this.dimensions = dimensions; + } + + public Ordering getRowOrdering() + { + return rowOrdering; + } + + public List getDimensions() + { + return dimensions; + } + } + public static class Builder { @Nullable diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index 9d22d84c6d3a..30ff20f5f604 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -201,7 +201,7 @@ public static GroupByQueryResources prepareResource( */ public Comparator createResultComparator(Query queryParam) { - return ((GroupByQuery) queryParam).getRowOrdering(true); + return ((GroupByQuery) queryParam).getOrderingAndDimensions(true).getRowOrdering(); } /** @@ -686,27 +686,16 @@ public Sequence processSubtotalsSpec( processingConfig.intermediateComputeSizeBytes() ); - List queryDimNamesInOrder = baseSubtotalQuery.getDimensions().stream().map(DimensionSpec::getOutputName) - .collect(Collectors.toList()); + List queryDimNamesInOrder = baseSubtotalQuery.getOrderingAndDimensions(false) + .getDimensions() + .stream() + .map(DimensionSpec::getOutputName) + .collect(Collectors.toList()); // Only needed to make LimitSpec.filterColumns(..) call later in case base query has a non default LimitSpec. Set aggsAndPostAggs = null; if (!(baseSubtotalQuery.getLimitSpec() instanceof NoopLimitSpec)) { aggsAndPostAggs = getAggregatorAndPostAggregatorNames(baseSubtotalQuery); - - DefaultLimitSpec limitSpec = (DefaultLimitSpec) baseSubtotalQuery.getLimitSpec(); - if (!limitSpec.getColumns().isEmpty() && limitSpec.isLimited()) { - Map dimToOutputNames = baseSubtotalQuery.getDimensions() - .stream() - .collect(Collectors.toMap( - DimensionSpec::getDimension, - DimensionSpec::getOutputName - )); - queryDimNamesInOrder = limitSpec.getColumns() - .stream() - .map(spec -> dimToOutputNames.get(spec.getDimension())) - .collect(Collectors.toList()); - } } List> subtotals = query.getSubtotalsSpec(); diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryTest.java index 2784d6b89181..6ed5a7760cb3 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryTest.java @@ -130,7 +130,7 @@ public void testRowOrderingMixTypes() .addDimension(new DefaultDimensionSpec("bat", "bat", ColumnType.STRING_ARRAY)) .build(); - final Ordering rowOrdering = query.getRowOrdering(false); + final Ordering rowOrdering = query.getOrderingAndDimensions(false).getRowOrdering(); final int compare = rowOrdering.compare( ResultRow.of(1, 1f, "a", new Object[]{"1", "2"}), ResultRow.of(1L, 1d, "b", new Object[]{"3"}) From df1b60e7a5360a2fbaa84a772650d9e142015aca Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Wed, 12 Jun 2024 09:56:47 +0530 Subject: [PATCH 6/7] test fix --- .../test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java | 1 - 1 file changed, 1 deletion(-) 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 86c1ec6156f6..5447a090db38 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 @@ -16145,7 +16145,6 @@ public void testGroupingSetsWithAggregateCase() @Test public void testGroupingSetsWithDifferentOrderLimitSpec() { - cannotVectorize(); msqIncompatible(); testBuilder() .sql( From 8e7380e1ed3bb35d48bdc1fcb44b7d7a621a86da Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 17 Jun 2024 21:08:48 +0530 Subject: [PATCH 7/7] refactor row ordering methods --- .../druid/query/groupby/GroupByQuery.java | 140 +++++++----------- .../druid/query/groupby/GroupingEngine.java | 8 +- .../druid/query/groupby/GroupByQueryTest.java | 2 +- 3 files changed, 53 insertions(+), 97 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java index d239a0e157bf..994705f55e30 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java @@ -468,7 +468,7 @@ public boolean getApplyLimitPushDownFromContext() @Override public Ordering getResultOrdering() { - final Ordering rowOrdering = getOrderingAndDimensions(false).getRowOrdering(); + final Ordering rowOrdering = getRowOrdering(false); return Ordering.from( (lhs, rhs) -> { @@ -560,15 +560,20 @@ private boolean canDoLimitPushDown( return false; } - /** - * When limit push down is applied, the partial results would be sorted by the ordering specified by the - * limit/order spec (unlike non-push down case where the results always use the default natural ascending order), - * so when merging these partial result streams, the merge needs to use the same ordering to get correct results. - */ - private OrderingAndDimensions getRowOrderingAndDimensionsForPushDown( - final boolean granular, - final DefaultLimitSpec limitSpec - ) + public Ordering getRowOrdering(final boolean granular) + { + return getOrderingAndDimensions(granular).getRowOrdering(); + } + + public List getDimensionNamesInOrder() + { + return getOrderingAndDimensions(false).getDimensions() + .stream() + .map(DimensionSpec::getOutputName) + .collect(Collectors.toList()); + } + + public OrderingAndDimensions getOrderingAndDimensions(final boolean granular) { final boolean sortByDimsFirst = getContextSortByDimsFirst(); @@ -579,18 +584,28 @@ private OrderingAndDimensions getRowOrderingAndDimensionsForPushDown( final List comparators = new ArrayList<>(); final List dimensionsInOrder = new ArrayList<>(); - for (OrderByColumnSpec orderSpec : limitSpec.getColumns()) { - boolean needsReverse = orderSpec.getDirection() != OrderByColumnSpec.Direction.ASCENDING; - int dimIndex = OrderByColumnSpec.getDimIndexForOrderBy(orderSpec, dimensions); - if (dimIndex >= 0) { - DimensionSpec dim = dimensions.get(dimIndex); - orderedFieldNumbers.add(resultRowSignature.indexOf(dim.getOutputName())); - dimsInOrderBy.add(dimIndex); - needsReverseList.add(needsReverse); - final ColumnType type = dimensions.get(dimIndex).getOutputType(); - dimensionTypes.add(type); - comparators.add(orderSpec.getDimensionComparator()); - dimensionsInOrder.add(dim); + /* + * When limit push down is applied, the partial results would be sorted by the ordering specified by the + * limit/order spec (unlike non-push down case where the results always use the default natural ascending order), + * so when merging these partial result streams, the merge needs to use the same ordering to get correct results. + */ + if (isApplyLimitPushDown()) { + DefaultLimitSpec limitSpec1 = (DefaultLimitSpec) limitSpec; + if (!DefaultLimitSpec.sortingOrderHasNonGroupingFields(limitSpec1, dimensions)) { + for (OrderByColumnSpec orderSpec : ((DefaultLimitSpec) limitSpec).getColumns()) { + boolean needsReverse = orderSpec.getDirection() != OrderByColumnSpec.Direction.ASCENDING; + int dimIndex = OrderByColumnSpec.getDimIndexForOrderBy(orderSpec, dimensions); + if (dimIndex >= 0) { + DimensionSpec dim = dimensions.get(dimIndex); + orderedFieldNumbers.add(resultRowSignature.indexOf(dim.getOutputName())); + dimsInOrderBy.add(dimIndex); + needsReverseList.add(needsReverse); + final ColumnType type = dimensions.get(dimIndex).getOutputType(); + dimensionTypes.add(type); + comparators.add(orderSpec.getDimensionComparator()); + dimensionsInOrder.add(dim); + } + } } } @@ -606,10 +621,11 @@ private OrderingAndDimensions getRowOrderingAndDimensionsForPushDown( } final Comparator timeComparator = getTimeComparator(granular); + Ordering ordering; if (timeComparator == null) { - return new OrderingAndDimensions(Ordering.from( - (lhs, rhs) -> compareDimsForLimitPushDown( + ordering = Ordering.from( + (lhs, rhs) -> compareDims( orderedFieldNumbers, needsReverseList, dimensionTypes, @@ -617,11 +633,11 @@ private OrderingAndDimensions getRowOrderingAndDimensionsForPushDown( lhs, rhs ) - ), dimensionsInOrder); + ); } else if (sortByDimsFirst) { - return new OrderingAndDimensions(Ordering.from( + ordering = Ordering.from( (lhs, rhs) -> { - final int cmp = compareDimsForLimitPushDown( + final int cmp = compareDims( orderedFieldNumbers, needsReverseList, dimensionTypes, @@ -635,9 +651,9 @@ private OrderingAndDimensions getRowOrderingAndDimensionsForPushDown( return timeComparator.compare(lhs, rhs); } - ), dimensionsInOrder); + ); } else { - return new OrderingAndDimensions(Ordering.from( + ordering = Ordering.from( (lhs, rhs) -> { final int timeCompare = timeComparator.compare(lhs, rhs); @@ -645,7 +661,7 @@ private OrderingAndDimensions getRowOrderingAndDimensionsForPushDown( return timeCompare; } - return compareDimsForLimitPushDown( + return compareDims( orderedFieldNumbers, needsReverseList, dimensionTypes, @@ -654,47 +670,10 @@ private OrderingAndDimensions getRowOrderingAndDimensionsForPushDown( rhs ); } - ), dimensionsInOrder); - } - } - - public OrderingAndDimensions getOrderingAndDimensions(final boolean granular) - { - if (isApplyLimitPushDown()) { - if (!DefaultLimitSpec.sortingOrderHasNonGroupingFields((DefaultLimitSpec) limitSpec, dimensions)) { - return getRowOrderingAndDimensionsForPushDown(granular, (DefaultLimitSpec) limitSpec); - } + ); } - final boolean sortByDimsFirst = getContextSortByDimsFirst(); - final Comparator timeComparator = getTimeComparator(granular); - - if (timeComparator == null) { - return new OrderingAndDimensions(Ordering.from((lhs, rhs) -> compareDims(dimensions, lhs, rhs)), dimensions); - } else if (sortByDimsFirst) { - return new OrderingAndDimensions(Ordering.from( - (lhs, rhs) -> { - final int cmp = compareDims(dimensions, lhs, rhs); - if (cmp != 0) { - return cmp; - } - - return timeComparator.compare(lhs, rhs); - } - ), dimensions); - } else { - return new OrderingAndDimensions(Ordering.from( - (lhs, rhs) -> { - final int timeCompare = timeComparator.compare(lhs, rhs); - - if (timeCompare != 0) { - return timeCompare; - } - - return compareDims(dimensions, lhs, rhs); - } - ), dimensions); - } + return new OrderingAndDimensions(ordering, dimensionsInOrder); } @Nullable @@ -719,25 +698,6 @@ private Comparator getTimeComparator(boolean granular) } } - private int compareDims(List dimensions, ResultRow lhs, ResultRow rhs) - { - final int dimensionStart = getResultRowDimensionStart(); - - for (int i = 0; i < dimensions.size(); i++) { - DimensionSpec dimension = dimensions.get(i); - final int dimCompare = DimensionHandlerUtils.compareObjectsAsType( - lhs.get(dimensionStart + i), - rhs.get(dimensionStart + i), - dimension.getOutputType() - ); - if (dimCompare != 0) { - return dimCompare; - } - } - - return 0; - } - /** * Computes the timestamp that will be returned by {@link #getUniversalTimestamp()}. */ @@ -763,12 +723,12 @@ private DateTime computeUniversalTimestamp() } /** - * Compares the dimensions for limit pushdown. + * Compares the dimensions. * * Due to legacy reason, the provided StringComparator for the arrays isn't applied and must be changed once we * get rid of the StringComparators for array types */ - private static int compareDimsForLimitPushDown( + private static int compareDims( final IntList fields, final List needsReverseList, final List dimensionTypes, diff --git a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java index 30ff20f5f604..ab1ee1052b4b 100644 --- a/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java +++ b/processing/src/main/java/org/apache/druid/query/groupby/GroupingEngine.java @@ -201,7 +201,7 @@ public static GroupByQueryResources prepareResource( */ public Comparator createResultComparator(Query queryParam) { - return ((GroupByQuery) queryParam).getOrderingAndDimensions(true).getRowOrdering(); + return ((GroupByQuery) queryParam).getRowOrdering(true); } /** @@ -686,11 +686,7 @@ public Sequence processSubtotalsSpec( processingConfig.intermediateComputeSizeBytes() ); - List queryDimNamesInOrder = baseSubtotalQuery.getOrderingAndDimensions(false) - .getDimensions() - .stream() - .map(DimensionSpec::getOutputName) - .collect(Collectors.toList()); + List queryDimNamesInOrder = baseSubtotalQuery.getDimensionNamesInOrder(); // Only needed to make LimitSpec.filterColumns(..) call later in case base query has a non default LimitSpec. Set aggsAndPostAggs = null; diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryTest.java index 6ed5a7760cb3..2784d6b89181 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryTest.java @@ -130,7 +130,7 @@ public void testRowOrderingMixTypes() .addDimension(new DefaultDimensionSpec("bat", "bat", ColumnType.STRING_ARRAY)) .build(); - final Ordering rowOrdering = query.getOrderingAndDimensions(false).getRowOrdering(); + final Ordering rowOrdering = query.getRowOrdering(false); final int compare = rowOrdering.compare( ResultRow.of(1, 1f, "a", new Object[]{"1", "2"}), ResultRow.of(1L, 1d, "b", new Object[]{"3"})