From fa365be519fde10e03f4c40110a5ea51c05ef8a0 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Thu, 21 Nov 2019 04:45:01 -0800 Subject: [PATCH 1/6] fix bug with sqlOuterLimit, use sqlOuterLimit instead of wrapping sql query for web console --- .../sql/calcite/planner/DruidPlanner.java | 10 ++++++ .../druid/sql/calcite/CalciteQueryTest.java | 33 +++++++++++++++++++ .../src/views/query-view/query-view.tsx | 23 +++++-------- 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java index 9349286b4696..fc4b7d3187fd 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java @@ -261,6 +261,16 @@ private RelNode possiblyWrapRootWithOuterLimitFromContext( return root.rel; } + if (root.rel instanceof LogicalSort) { + LogicalSort outerSort = (LogicalSort) root.rel; + return LogicalSort.create( + outerSort.getInput(), + outerSort.collation, + makeBigIntLiteral(0), + makeBigIntLiteral(outerLimit) + ); + } + return LogicalSort.create( root.rel, root.collation, 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 0c1b9f96fa11..ac903aedc2a6 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 @@ -809,6 +809,39 @@ public void testSelectStarFromSelectSingleColumnWithLimitDescending() throws Exc new Object[]{"def"} ) ); + + List expected; + if (NullHandling.replaceWithDefault()) { + expected = ImmutableList.of( + new Object[]{"", 1L}, + new Object[]{"def", 1L} + ); + } else { + expected = ImmutableList.of( + new Object[]{"def", 1L}, + new Object[]{"abc", 1L} + ); + } + testQuery( + "SELECT dim1, COUNT(*) FROM druid.foo GROUP BY dim1 ORDER BY dim1 DESC", + outerLimitContext, + ImmutableList.of( + new TopNQueryBuilder() + .dataSource(CalciteTests.DATASOURCE1) + .intervals(querySegmentSpec(Filtration.eternity())) + .dimension(new DefaultDimensionSpec("dim1", "d0", ValueType.STRING)) + .threshold(2) + .aggregators(aggregators(new CountAggregatorFactory("a0"))) + .metric( + new InvertedTopNMetricSpec( + new DimensionTopNMetricSpec(null, StringComparators.LEXICOGRAPHIC) + ) + ) + .context(outerLimitContext) + .build() + ), + expected + ); } @Test diff --git a/web-console/src/views/query-view/query-view.tsx b/web-console/src/views/query-view/query-view.tsx index 2f1d7499bec8..7d90795e47e7 100644 --- a/web-console/src/views/query-view/query-view.tsx +++ b/web-console/src/views/query-view/query-view.tsx @@ -131,13 +131,6 @@ export class QueryView extends React.PureComponent { const { queryString, queryContext, wrapQueryLimit } = queryWithContext; - const actualQuery = QueryView.wrapInLimitIfNeeded(queryString, wrapQueryLimit); - const explainPayload: Record = { - query: QueryView.wrapInExplainIfNeeded(actualQuery), + query: QueryView.wrapInExplainIfNeeded(queryString), resultFormat: 'object', }; - if (!isEmptyContext(queryContext)) explainPayload.context = queryContext; + if (!isEmptyContext(queryContext) || wrapQueryLimit) { + explainPayload.context = queryContext || {}; + explainPayload.context.sqlOuterLimit = wrapQueryLimit; + } const result = await queryDruidSql(explainPayload); return parseQueryPlan(result[0]['PLAN']); From cdf32a3de0166d7832eb695437ae72fac7bfb0e9 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 26 Nov 2019 14:40:48 -0800 Subject: [PATCH 2/6] fixes, refactors, tests --- .../druid/sql/calcite/planner/Calcites.java | 34 ++++++ .../sql/calcite/planner/DruidPlanner.java | 77 ++++++------ .../sql/calcite/rule/SortCollapseRule.java | 50 +++----- .../sql/calcite/BaseCalciteQueryTest.java | 4 + .../druid/sql/calcite/CalciteQueryTest.java | 114 ++++++++++++++++-- .../src/views/query-view/query-view.tsx | 4 +- 6 files changed, 205 insertions(+), 78 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java index 57067e50a50b..996fdf4e13f0 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java @@ -23,6 +23,7 @@ import com.google.common.io.BaseEncoding; import com.google.common.primitives.Chars; import org.apache.calcite.jdbc.CalciteSchema; +import org.apache.calcite.rel.core.Sort; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rex.RexLiteral; @@ -378,4 +379,37 @@ public static String makePrefixedName(final String prefix, final String suffix) { return StringUtils.format("%s:%s", prefix, suffix); } + + public static int getInt(RexNode rex, int defaultValue) + { + return rex == null ? defaultValue : RexLiteral.intValue(rex); + } + + public static int getOffset(Sort sort) + { + return Calcites.getInt(sort.offset, 0); + } + + public static int getFetch(Sort sort) + { + return Calcites.getInt(sort.fetch, -1); + } + + public static int combineFetch(int innerFetch, int outerFetch, int outerOffset) + { + final int fetch; + if (innerFetch < 0 && outerFetch < 0) { + // Neither has a limit => no limit overall. + fetch = -1; + } else if (innerFetch < 0) { + // Outer limit only. + fetch = outerFetch; + } else if (outerFetch < 0) { + // Inner limit only. + fetch = Math.max(0, innerFetch - outerOffset); + } else { + fetch = Math.max(0, Math.min(innerFetch - outerOffset, outerFetch)); + } + return fetch; + } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java index fc4b7d3187fd..5d84da2d797b 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java @@ -24,6 +24,7 @@ import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.primitives.Ints; import org.apache.calcite.DataContext; import org.apache.calcite.adapter.java.JavaTypeFactory; import org.apache.calcite.interpreter.BindableConvention; @@ -35,6 +36,7 @@ import org.apache.calcite.plan.RelOptUtil; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.RelRoot; +import org.apache.calcite.rel.core.Sort; import org.apache.calcite.rel.logical.LogicalSort; import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rex.RexBuilder; @@ -139,31 +141,26 @@ private PlannerResult planWithDruidConvention( if (explain != null) { return planExplanation(druidRel, explain, dataSourceNames); } else { - final Supplier> resultsSupplier = new Supplier>() - { - @Override - public Sequence get() - { - if (root.isRefTrivial()) { - return druidRel.runQuery(); - } else { - // Add a mapping on top to accommodate root.fields. - return Sequences.map( - druidRel.runQuery(), - new Function() + final Supplier> resultsSupplier = () -> { + if (root.isRefTrivial()) { + return druidRel.runQuery(); + } else { + // Add a mapping on top to accommodate root.fields. + return Sequences.map( + druidRel.runQuery(), + new Function() + { + @Override + public Object[] apply(final Object[] input) { - @Override - public Object[] apply(final Object[] input) - { - final Object[] retVal = new Object[root.fields.size()]; - for (int i = 0; i < root.fields.size(); i++) { - retVal[i] = input[root.fields.get(i).getKey()]; - } - return retVal; + final Object[] retVal = new Object[root.fields.size()]; + for (int i = 0; i < root.fields.size(); i++) { + retVal[i] = input[root.fields.get(i).getKey()]; } + return retVal; } - ); - } + } + ); } }; @@ -212,9 +209,9 @@ private PlannerResult planWithBindableConvention( new BaseSequence.IteratorMaker>() { @Override - public EnumeratorIterator make() + public EnumeratorIterator make() { - return new EnumeratorIterator(new Iterator() + return new EnumeratorIterator<>(new Iterator() { @Override public boolean hasNext() @@ -236,16 +233,19 @@ public void cleanup(EnumeratorIterator iterFromMake) } } - ), () -> enumerator.close()); + ), enumerator::close); }; return new PlannerResult(resultsSupplier, root.validatedRowType, ImmutableSet.of()); } } /** - * This method wraps the root with a logical sort that applies a limit (no ordering change). - * The CTX_SQL_OUTER_LIMIT flag that controls this wrapping is meant for internal use only by the - * web console, allowing it to apply a limit to queries without rewriting the original SQL. + * This method wraps the root with a {@link LogicalSort} that applies a limit (no ordering change). If the outer rel + * is already a {@link Sort}, we can merge our outerLimit into it, similar to what is going on in + * {@link org.apache.druid.sql.calcite.rule.SortCollapseRule}. + * + * The {@link PlannerContext#CTX_SQL_OUTER_LIMIT} flag that controls this wrapping is meant for internal use only by + * the web console, allowing it to apply a limit to queries without rewriting the original SQL. * * @param root root node * @return root node wrapped with a limiting logical sort if a limit is specified in the query context. @@ -261,13 +261,20 @@ private RelNode possiblyWrapRootWithOuterLimitFromContext( return root.rel; } - if (root.rel instanceof LogicalSort) { - LogicalSort outerSort = (LogicalSort) root.rel; + if (root.rel instanceof Sort) { + Sort innerSort = (Sort) root.rel; + final int offset = Calcites.getOffset(innerSort); + final int fetch = Calcites.combineFetch( + Calcites.getFetch(innerSort), + Ints.checkedCast(outerLimit), + 0 + ); + return LogicalSort.create( - outerSort.getInput(), - outerSort.collation, - makeBigIntLiteral(0), - makeBigIntLiteral(outerLimit) + innerSort.getInput(), + innerSort.collation, + makeBigIntLiteral(offset), + makeBigIntLiteral(fetch) ); } @@ -292,7 +299,7 @@ private static class EnumeratorIterator implements Iterator { private final Iterator it; - public EnumeratorIterator(Iterator it) + EnumeratorIterator(Iterator it) { this.it = it; } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java b/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java index c3e91233f990..1fd66e892445 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java @@ -22,7 +22,7 @@ import org.apache.calcite.plan.RelOptRule; import org.apache.calcite.plan.RelOptRuleCall; import org.apache.calcite.rel.core.Sort; -import org.apache.calcite.rex.RexLiteral; +import org.apache.druid.sql.calcite.planner.Calcites; /** * Collapses two adjacent Sort operations together. Useful for queries like @@ -45,48 +45,30 @@ public static SortCollapseRule instance() @Override public void onMatch(final RelOptRuleCall call) { - // First is the inner sort, second is the outer sort. - final Sort first = call.rel(1); - final Sort second = call.rel(0); + final Sort outerSort = call.rel(0); + final Sort innerSort = call.rel(1); - if (second.collation.getFieldCollations().isEmpty() - || second.collation.getFieldCollations().equals(first.collation.getFieldCollations())) { + if (outerSort.collation.getFieldCollations().isEmpty() + || outerSort.collation.getFieldCollations().equals(innerSort.collation.getFieldCollations())) { // Add up the offsets. - final int firstOffset = (first.offset != null ? RexLiteral.intValue(first.offset) : 0); - final int secondOffset = (second.offset != null ? RexLiteral.intValue(second.offset) : 0); + final int innerOffset = Calcites.getOffset(innerSort); + final int innerFetch = Calcites.getFetch(innerSort); + final int outerOffset = Calcites.getOffset(outerSort); + final int outerFetch = Calcites.getFetch(innerSort); - final int offset = firstOffset + secondOffset; - final int fetch; + final int offset = innerOffset + outerOffset; + final int fetch = Calcites.combineFetch(innerFetch, outerFetch, outerOffset); - if (first.fetch == null && second.fetch == null) { - // Neither has a limit => no limit overall. - fetch = -1; - } else if (first.fetch == null) { - // Outer limit only. - fetch = RexLiteral.intValue(second.fetch); - } else if (second.fetch == null) { - // Inner limit only. - fetch = Math.max(0, RexLiteral.intValue(first.fetch) - secondOffset); - } else { - fetch = Math.max( - 0, - Math.min( - RexLiteral.intValue(first.fetch) - secondOffset, - RexLiteral.intValue(second.fetch) - ) - ); - } - - final Sort combined = first.copy( - first.getTraitSet(), - first.getInput(), - first.getCollation(), + final Sort combined = innerSort.copy( + innerSort.getTraitSet(), + innerSort.getInput(), + innerSort.getCollation(), offset == 0 ? null : call.builder().literal(offset), fetch < 0 ? null : call.builder().literal(fetch) ); call.transformTo(combined); - call.getPlanner().setImportance(second, 0.0); + call.getPlanner().setImportance(outerSort, 0.0); } } } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java index 4d22779491c2..4ac6f92e8dbe 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/BaseCalciteQueryTest.java @@ -214,6 +214,8 @@ public int getMaxSemiJoinRowsInMemory() // Matches QUERY_CONTEXT_LOS_ANGELES public static final Map TIMESERIES_CONTEXT_LOS_ANGELES = new HashMap<>(); + public static final Map OUTER_LIMIT_CONTEXT = new HashMap<>(QUERY_CONTEXT_DEFAULT); + public static QueryRunnerFactoryConglomerate conglomerate; public static Closer resourceCloser; @@ -236,6 +238,8 @@ public int getMaxSemiJoinRowsInMemory() TIMESERIES_CONTEXT_LOS_ANGELES.put("skipEmptyBuckets", true); TIMESERIES_CONTEXT_LOS_ANGELES.put(QueryContexts.DEFAULT_TIMEOUT_KEY, QueryContexts.DEFAULT_TIMEOUT_MILLIS); TIMESERIES_CONTEXT_LOS_ANGELES.put(QueryContexts.MAX_SCATTER_GATHER_BYTES_KEY, Long.MAX_VALUE); + + OUTER_LIMIT_CONTEXT.put(PlannerContext.CTX_SQL_OUTER_LIMIT, 2); } // Generate timestamps for expected results 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 ac903aedc2a6..0f9ffb764e72 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 @@ -786,13 +786,14 @@ public void testSelectStarFromSelectSingleColumnWithLimitDescending() throws Exc new Object[]{"10.1"} ) ); + } - // The outer limit wrapping behavior that was used in the query above can be applied with a context flag instead - Map outerLimitContext = new HashMap<>(QUERY_CONTEXT_DEFAULT); - outerLimitContext.put(PlannerContext.CTX_SQL_OUTER_LIMIT, 2); + @Test + public void testSelectLimitWrapping() throws Exception + { testQuery( "SELECT dim1 FROM druid.foo ORDER BY __time DESC", - outerLimitContext, + OUTER_LIMIT_CONTEXT, ImmutableList.of( newScanQueryBuilder() .dataSource(CalciteTests.DATASOURCE1) @@ -801,7 +802,7 @@ public void testSelectStarFromSelectSingleColumnWithLimitDescending() throws Exc .limit(2) .order(ScanQuery.Order.DESCENDING) .resultFormat(ScanQuery.ResultFormat.RESULT_FORMAT_COMPACTED_LIST) - .context(outerLimitContext) + .context(OUTER_LIMIT_CONTEXT) .build() ), ImmutableList.of( @@ -809,7 +810,11 @@ public void testSelectStarFromSelectSingleColumnWithLimitDescending() throws Exc new Object[]{"def"} ) ); + } + @Test + public void testTopNLimitWrapping() throws Exception + { List expected; if (NullHandling.replaceWithDefault()) { expected = ImmutableList.of( @@ -824,7 +829,7 @@ public void testSelectStarFromSelectSingleColumnWithLimitDescending() throws Exc } testQuery( "SELECT dim1, COUNT(*) FROM druid.foo GROUP BY dim1 ORDER BY dim1 DESC", - outerLimitContext, + OUTER_LIMIT_CONTEXT, ImmutableList.of( new TopNQueryBuilder() .dataSource(CalciteTests.DATASOURCE1) @@ -837,13 +842,108 @@ public void testSelectStarFromSelectSingleColumnWithLimitDescending() throws Exc new DimensionTopNMetricSpec(null, StringComparators.LEXICOGRAPHIC) ) ) - .context(outerLimitContext) + .context(OUTER_LIMIT_CONTEXT) .build() ), expected ); } + + @Test + public void testTopNLimitWrappingOrderByAgg() throws Exception + { + testQuery( + "SELECT dim1, COUNT(*) FROM druid.foo GROUP BY 1 ORDER BY 2 DESC", + OUTER_LIMIT_CONTEXT, + ImmutableList.of( + new TopNQueryBuilder() + .dataSource(CalciteTests.DATASOURCE1) + .intervals(querySegmentSpec(Filtration.eternity())) + .dimension(new DefaultDimensionSpec("dim1", "d0", ValueType.STRING)) + .threshold(2) + .aggregators(aggregators(new CountAggregatorFactory("a0"))) + .metric("a0") + .context(OUTER_LIMIT_CONTEXT) + .build() + ), + ImmutableList.of(new Object[]{"", 1L}, new Object[]{"1", 1L}) + ); + } + + @Test + public void testGroupByLimitWrapping() throws Exception + { + List expected; + if (NullHandling.replaceWithDefault()) { + expected = ImmutableList.of( + new Object[]{"def", "abc", 1L}, + new Object[]{"abc", "", 1L} + ); + } else { + expected = ImmutableList.of( + new Object[]{"def", "abc", 1L}, + new Object[]{"abc", null, 1L} + ); + } + testQuery( + "SELECT dim1, dim2, COUNT(*) FROM druid.foo GROUP BY dim1, dim2 ORDER BY dim1 DESC", + OUTER_LIMIT_CONTEXT, + ImmutableList.of( + new GroupByQuery.Builder() + .setDataSource(CalciteTests.DATASOURCE1) + .setInterval(querySegmentSpec(Filtration.eternity())) + .setGranularity(Granularities.ALL) + .setDimensions( + new DefaultDimensionSpec("dim1", "d0", ValueType.STRING), + new DefaultDimensionSpec("dim2", "d1", ValueType.STRING) + ) + .setLimitSpec( + new DefaultLimitSpec( + ImmutableList.of( + new OrderByColumnSpec("d0", Direction.DESCENDING, StringComparators.LEXICOGRAPHIC)), + 2 + ) + ) + .setAggregatorSpecs(aggregators(new CountAggregatorFactory("a0"))) + .setContext(OUTER_LIMIT_CONTEXT) + .build() + ), + expected + ); + } + + @Test + public void testGroupByLimitWrappingOrderByAgg() throws Exception + { + testQuery( + "SELECT dim1, dim2, COUNT(*) FROM druid.foo GROUP BY 1, 2 ORDER BY 3 DESC", + OUTER_LIMIT_CONTEXT, + ImmutableList.of( + new GroupByQuery.Builder() + .setDataSource(CalciteTests.DATASOURCE1) + .setInterval(querySegmentSpec(Filtration.eternity())) + .setGranularity(Granularities.ALL) + .setDimensions( + new DefaultDimensionSpec("dim1", "d0", ValueType.STRING), + new DefaultDimensionSpec("dim2", "d1", ValueType.STRING) + ) + .setLimitSpec( + new DefaultLimitSpec( + ImmutableList.of( + new OrderByColumnSpec("a0", Direction.DESCENDING, StringComparators.NUMERIC) + ), + 2 + ) + ) + .setAggregatorSpecs(aggregators(new CountAggregatorFactory("a0"))) + .setContext(OUTER_LIMIT_CONTEXT) + .build() + ), + ImmutableList.of(new Object[]{"", "a", 1L}, new Object[]{"def", "abc", 1L}) + ); + } + @Test public void testSelectProjectionFromSelectSingleColumnWithInnerLimitDescending() throws Exception { diff --git a/web-console/src/views/query-view/query-view.tsx b/web-console/src/views/query-view/query-view.tsx index 7d90795e47e7..45729071c2a4 100644 --- a/web-console/src/views/query-view/query-view.tsx +++ b/web-console/src/views/query-view/query-view.tsx @@ -252,7 +252,7 @@ export class QueryView extends React.PureComponent Date: Tue, 26 Nov 2019 14:42:35 -0800 Subject: [PATCH 3/6] meh --- .../druid/sql/calcite/planner/DruidPlanner.java | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java index 5d84da2d797b..77c616106c1f 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java @@ -19,7 +19,6 @@ package org.apache.druid.sql.calcite.planner; -import com.google.common.base.Function; import com.google.common.base.Supplier; import com.google.common.base.Suppliers; import com.google.common.collect.ImmutableList; @@ -148,17 +147,12 @@ private PlannerResult planWithDruidConvention( // Add a mapping on top to accommodate root.fields. return Sequences.map( druidRel.runQuery(), - new Function() - { - @Override - public Object[] apply(final Object[] input) - { - final Object[] retVal = new Object[root.fields.size()]; - for (int i = 0; i < root.fields.size(); i++) { - retVal[i] = input[root.fields.get(i).getKey()]; - } - return retVal; + input -> { + final Object[] retVal = new Object[root.fields.size()]; + for (int i = 0; i < root.fields.size(); i++) { + retVal[i] = input[root.fields.get(i).getKey()]; } + return retVal; } ); } From 27feae36928f7b7b4be517f7647129f88cc70b73 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 26 Nov 2019 14:57:26 -0800 Subject: [PATCH 4/6] better name --- .../java/org/apache/druid/sql/calcite/planner/Calcites.java | 2 +- .../java/org/apache/druid/sql/calcite/planner/DruidPlanner.java | 2 +- .../org/apache/druid/sql/calcite/rule/SortCollapseRule.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java index 996fdf4e13f0..81f45f237b4f 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java @@ -395,7 +395,7 @@ public static int getFetch(Sort sort) return Calcites.getInt(sort.fetch, -1); } - public static int combineFetch(int innerFetch, int outerFetch, int outerOffset) + public static int collapseFetch(int innerFetch, int outerFetch, int outerOffset) { final int fetch; if (innerFetch < 0 && outerFetch < 0) { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java index 77c616106c1f..54a9ad807794 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java @@ -258,7 +258,7 @@ private RelNode possiblyWrapRootWithOuterLimitFromContext( if (root.rel instanceof Sort) { Sort innerSort = (Sort) root.rel; final int offset = Calcites.getOffset(innerSort); - final int fetch = Calcites.combineFetch( + final int fetch = Calcites.collapseFetch( Calcites.getFetch(innerSort), Ints.checkedCast(outerLimit), 0 diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java b/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java index 1fd66e892445..83aedef3aaa2 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java @@ -57,7 +57,7 @@ public void onMatch(final RelOptRuleCall call) final int outerFetch = Calcites.getFetch(innerSort); final int offset = innerOffset + outerOffset; - final int fetch = Calcites.combineFetch(innerFetch, outerFetch, outerOffset); + final int fetch = Calcites.collapseFetch(innerFetch, outerFetch, outerOffset); final Sort combined = innerSort.copy( innerSort.getTraitSet(), From c770b2799e443f48bfe944c8b6e2a4929ec55560 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 26 Nov 2019 15:04:32 -0800 Subject: [PATCH 5/6] fix comment location --- .../org/apache/druid/sql/calcite/rule/SortCollapseRule.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java b/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java index 83aedef3aaa2..a1832ba4ca99 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java @@ -50,12 +50,12 @@ public void onMatch(final RelOptRuleCall call) if (outerSort.collation.getFieldCollations().isEmpty() || outerSort.collation.getFieldCollations().equals(innerSort.collation.getFieldCollations())) { - // Add up the offsets. final int innerOffset = Calcites.getOffset(innerSort); final int innerFetch = Calcites.getFetch(innerSort); final int outerOffset = Calcites.getOffset(outerSort); final int outerFetch = Calcites.getFetch(innerSort); + // Add up the offsets. final int offset = innerOffset + outerOffset; final int fetch = Calcites.collapseFetch(innerFetch, outerFetch, outerOffset); From cdbd3d4a3896ae89a22ec3da995abadf54454d5f Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Wed, 27 Nov 2019 14:26:48 -0800 Subject: [PATCH 6/6] fix copy and paste --- .../org/apache/druid/sql/calcite/rule/SortCollapseRule.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java b/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java index a1832ba4ca99..bad8bdef7ed1 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rule/SortCollapseRule.java @@ -53,7 +53,7 @@ public void onMatch(final RelOptRuleCall call) final int innerOffset = Calcites.getOffset(innerSort); final int innerFetch = Calcites.getFetch(innerSort); final int outerOffset = Calcites.getOffset(outerSort); - final int outerFetch = Calcites.getFetch(innerSort); + final int outerFetch = Calcites.getFetch(outerSort); // Add up the offsets. final int offset = innerOffset + outerOffset;