From a0908d4331bf351a27132345475e4aef7d09e631 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Mon, 23 Oct 2023 11:48:26 +0000 Subject: [PATCH 1/9] fix-1/2 --- .../druid/sql/calcite/expression/Expressions.java | 8 +++++++- .../druid/sql/calcite/planner/CalcitePlanner.java | 3 ++- .../druid/sql/calcite/planner/DruidSqlValidator.java | 12 +++++++++++- .../druid/sql/calcite/planner/PlannerFactory.java | 7 +++++-- .../apache/druid/sql/calcite/CalciteQueryTest.java | 12 ++++++++++++ 5 files changed, 37 insertions(+), 5 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java index 7a8786e21ef5..5cfc62f67bd4 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java @@ -28,6 +28,7 @@ import org.apache.calcite.rex.RexInputRef; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; +import org.apache.calcite.rex.RexOver; import org.apache.calcite.rex.RexUtil; import org.apache.calcite.sql.SqlKind; import org.apache.calcite.sql.SqlOperator; @@ -66,6 +67,7 @@ import org.apache.druid.sql.calcite.planner.ExpressionParser; import org.apache.druid.sql.calcite.planner.PlannerContext; import org.apache.druid.sql.calcite.rel.VirtualColumnRegistry; +import org.apache.druid.sql.calcite.run.EngineFeature; import org.apache.druid.sql.calcite.table.RowSignatures; import org.joda.time.Interval; @@ -276,7 +278,11 @@ private static DruidExpression rexCallToDruidExpression( .lookupOperatorConversion(operator); if (conversion == null) { - plannerContext.setPlanningError("SQL query requires '%s' operator that is not supported.", operator.getName()); + if (rexNode instanceof RexOver && !plannerContext.featureAvailable(EngineFeature.WINDOW_FUNCTIONS)) { + plannerContext.setPlanningError("Query contains a window functions; consider enabling windowing with [%s].", plannerContext.CTX_ENABLE_WINDOW_FNS); + } else { + plannerContext.setPlanningError("SQL query requires '%s' operator that is not supported.", operator.getName()); + } return null; } else { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java index 1abec772e313..2cda011848b5 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java @@ -407,7 +407,8 @@ private SqlValidator createSqlValidator(CalciteCatalogReader catalogReader) opTab, catalogReader, getTypeFactory(), - validatorConfig + validatorConfig, + context.unwrapOrThrow(PlannerContext.class) ); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java index bbc4f99a1316..2ddee31b39f9 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java @@ -30,6 +30,7 @@ import org.apache.calcite.sql.SqlOperatorTable; import org.apache.calcite.sql.parser.SqlParserPos; import org.apache.calcite.sql.validate.SqlValidatorScope; +import org.apache.druid.sql.calcite.run.EngineFeature; /** * Druid extended SQL validator. (At present, it doesn't actually @@ -37,14 +38,18 @@ */ class DruidSqlValidator extends BaseDruidSqlValidator { + private final PlannerContext plannerContext; + protected DruidSqlValidator( SqlOperatorTable opTab, CalciteCatalogReader catalogReader, JavaTypeFactory typeFactory, - Config validatorConfig + Config validatorConfig, + PlannerContext plannerContext ) { super(opTab, catalogReader, typeFactory, validatorConfig); + this.plannerContext = plannerContext; } @Override @@ -62,6 +67,11 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) throw buildCalciteContextException("ASCENDING ordering with NULLS LAST is not supported!", call); } } + if (call.getKind() == SqlKind.OVER) { + if(!plannerContext.featureAvailable(EngineFeature.WINDOW_FUNCTIONS)) { + throw buildCalciteContextException("XASCENDING ordering with NULLS LAST is not supported!", call); + } + } super.validateCall(call, scope); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java index 8cd7151dfb75..a5cdc028e7fc 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerFactory.java @@ -182,9 +182,12 @@ public SqlConformance conformance() return DruidConformance.instance(); } }; - } else { - return null; } + if (aClass.equals(PlannerContext.class)) { + return (C) plannerContext; + } + + return null; } }); 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 811227162ac7..2894d0521092 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 @@ -14275,4 +14275,16 @@ public void testUnSupportedNullsLast() .run()); assertThat(e, invalidSqlIs("ASCENDING ordering with NULLS LAST is not supported! (line [1], column [41])")); } + + @Test + public void testWindowingHint() + { + DruidException e = assertThrows(DruidException.class, () -> testBuilder() + .queryContext(ImmutableMap.of(PlannerContext.CTX_ENABLE_WINDOW_FNS, false)) + .sql("SELECT dim1,ROW_NUMBER() OVER () from druid.foo") + .run()); + + assertThat(e, invalidSqlIs("DESCENDING ordering with NULLS FIRST is not supported! (line [1], column [41])")); + } + } From f20e4634acd8897e1cdf1a618bb8032dd933da20 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Mon, 23 Oct 2023 11:57:50 +0000 Subject: [PATCH 2/9] add message v1 --- .../org/apache/druid/query/groupby/GroupingEngine.java | 6 +++--- .../druid/sql/calcite/planner/DruidSqlValidator.java | 9 +++++++-- 2 files changed, 10 insertions(+), 5 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 b242ff98555a..cec8260f26fe 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 @@ -788,11 +788,11 @@ private static boolean summaryRowPreconditions(GroupByQuery query) private static Iterator summaryRowIterator(GroupByQuery q) { List aggSpec = q.getAggregatorSpecs(); - Object[] values = new Object[aggSpec.size()]; + ResultRow resultRow = ResultRow.create(q.getResultRowSizeWithPostAggregators()); for (int i = 0; i < aggSpec.size(); i++) { - values[i] = aggSpec.get(i).factorize(new AllNullColumnSelectorFactory()).get(); + resultRow.set(i, aggSpec.get(i).factorize(new AllNullColumnSelectorFactory()).get()); } - return Collections.singleton(ResultRow.of(values)).iterator(); + return Collections.singleton(resultRow).iterator(); } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java index 2ddee31b39f9..1d6f120b2158 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java @@ -30,6 +30,7 @@ import org.apache.calcite.sql.SqlOperatorTable; import org.apache.calcite.sql.parser.SqlParserPos; import org.apache.calcite.sql.validate.SqlValidatorScope; +import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.sql.calcite.run.EngineFeature; /** @@ -68,8 +69,12 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) } } if (call.getKind() == SqlKind.OVER) { - if(!plannerContext.featureAvailable(EngineFeature.WINDOW_FUNCTIONS)) { - throw buildCalciteContextException("XASCENDING ordering with NULLS LAST is not supported!", call); + if (!plannerContext.featureAvailable(EngineFeature.WINDOW_FUNCTIONS)) { + throw buildCalciteContextException( + StringUtils.format( + "The query contains window functions. Please enable [%s]", + EngineFeature.WINDOW_FUNCTIONS), + call); } } super.validateCall(call, scope); From 7f440fa9a0d5219cc8441feefa30f15a4300a6c9 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Mon, 23 Oct 2023 12:12:44 +0000 Subject: [PATCH 3/9] extend test to cover for IOB issue --- .../apache/druid/query/groupby/GroupByQueryRunnerTest.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java index c8f5aa7dfcac..e9cd4d0c85e6 100644 --- a/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java +++ b/processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java @@ -12962,6 +12962,9 @@ public void testSummaryrowForEmptyInput() new FloatSumAggregatorFactory("idxFloat", "indexFloat"), new DoubleSumAggregatorFactory("idxDouble", "index") ) + .setPostAggregatorSpecs( + ImmutableList.of( + new ExpressionPostAggregator("post", "idx * 2", null, TestExprMacroTable.INSTANCE))) .setGranularity(QueryRunnerTestHelper.ALL_GRAN) .build(); @@ -12976,7 +12979,9 @@ public void testSummaryrowForEmptyInput() "idxFloat", NullHandling.replaceWithDefault() ? 0.0 : null, "idxDouble", - NullHandling.replaceWithDefault() ? 0.0 : null + NullHandling.replaceWithDefault() ? 0.0 : null, + "post", + NullHandling.replaceWithDefault() ? 0L : null ) ); From 6dd94f019d706650674ea87076519ef4cdb65f36 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Mon, 23 Oct 2023 12:16:25 +0000 Subject: [PATCH 4/9] move stuff around --- .../sql/calcite/planner/DruidSqlValidator.java | 18 +++++++++--------- .../druid/sql/calcite/CalciteQueryTest.java | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java index 1d6f120b2158..687b3dc70bc5 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java @@ -56,6 +56,15 @@ protected DruidSqlValidator( @Override public void validateCall(SqlCall call, SqlValidatorScope scope) { + if (call.getKind() == SqlKind.OVER) { + if (!plannerContext.featureAvailable(EngineFeature.WINDOW_FUNCTIONS)) { + throw buildCalciteContextException( + StringUtils.format( + "The query contains window functions. Please enable [%s]", + EngineFeature.WINDOW_FUNCTIONS), + call); + } + } if (call.getKind() == SqlKind.NULLS_FIRST) { SqlNode op0 = call.getOperandList().get(0); if (op0.getKind() == SqlKind.DESCENDING) { @@ -68,15 +77,6 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) throw buildCalciteContextException("ASCENDING ordering with NULLS LAST is not supported!", call); } } - if (call.getKind() == SqlKind.OVER) { - if (!plannerContext.featureAvailable(EngineFeature.WINDOW_FUNCTIONS)) { - throw buildCalciteContextException( - StringUtils.format( - "The query contains window functions. Please enable [%s]", - EngineFeature.WINDOW_FUNCTIONS), - call); - } - } super.validateCall(call, scope); } 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 2894d0521092..eb10fccf807b 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 @@ -14277,14 +14277,14 @@ public void testUnSupportedNullsLast() } @Test - public void testWindowingHint() + public void testWindowingErrorWithoutFeatureFlag() { DruidException e = assertThrows(DruidException.class, () -> testBuilder() .queryContext(ImmutableMap.of(PlannerContext.CTX_ENABLE_WINDOW_FNS, false)) .sql("SELECT dim1,ROW_NUMBER() OVER () from druid.foo") .run()); - assertThat(e, invalidSqlIs("DESCENDING ordering with NULLS FIRST is not supported! (line [1], column [41])")); + assertThat(e, invalidSqlIs("The query contains window functions. Please enable [WINDOW_FUNCTIONS] (line [1], column [13])")); } } From f18d2bef8103981511b8ca1829e55358312b4610 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Mon, 23 Oct 2023 12:35:22 +0000 Subject: [PATCH 5/9] change message --- .../org/apache/druid/sql/calcite/planner/DruidSqlValidator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java index 687b3dc70bc5..2844350241b9 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java @@ -60,7 +60,7 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) if (!plannerContext.featureAvailable(EngineFeature.WINDOW_FUNCTIONS)) { throw buildCalciteContextException( StringUtils.format( - "The query contains window functions. Please enable [%s]", + "The query contains window functions; To run these window functions, enable [%s] in query context.", EngineFeature.WINDOW_FUNCTIONS), call); } From 3dcb6808bf571410e57b5af42f650647004a324a Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Mon, 23 Oct 2023 13:43:51 +0000 Subject: [PATCH 6/9] fix testcase string --- .../apache/druid/sql/calcite/expression/Expressions.java | 8 +------- .../org/apache/druid/sql/calcite/CalciteQueryTest.java | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java index 5cfc62f67bd4..7a8786e21ef5 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java @@ -28,7 +28,6 @@ import org.apache.calcite.rex.RexInputRef; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; -import org.apache.calcite.rex.RexOver; import org.apache.calcite.rex.RexUtil; import org.apache.calcite.sql.SqlKind; import org.apache.calcite.sql.SqlOperator; @@ -67,7 +66,6 @@ import org.apache.druid.sql.calcite.planner.ExpressionParser; import org.apache.druid.sql.calcite.planner.PlannerContext; import org.apache.druid.sql.calcite.rel.VirtualColumnRegistry; -import org.apache.druid.sql.calcite.run.EngineFeature; import org.apache.druid.sql.calcite.table.RowSignatures; import org.joda.time.Interval; @@ -278,11 +276,7 @@ private static DruidExpression rexCallToDruidExpression( .lookupOperatorConversion(operator); if (conversion == null) { - if (rexNode instanceof RexOver && !plannerContext.featureAvailable(EngineFeature.WINDOW_FUNCTIONS)) { - plannerContext.setPlanningError("Query contains a window functions; consider enabling windowing with [%s].", plannerContext.CTX_ENABLE_WINDOW_FNS); - } else { - plannerContext.setPlanningError("SQL query requires '%s' operator that is not supported.", operator.getName()); - } + plannerContext.setPlanningError("SQL query requires '%s' operator that is not supported.", operator.getName()); return null; } else { 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 eb10fccf807b..bb998c32f1b5 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 @@ -14284,7 +14284,7 @@ public void testWindowingErrorWithoutFeatureFlag() .sql("SELECT dim1,ROW_NUMBER() OVER () from druid.foo") .run()); - assertThat(e, invalidSqlIs("The query contains window functions. Please enable [WINDOW_FUNCTIONS] (line [1], column [13])")); + assertThat(e, invalidSqlIs("The query contains window functions; To run these window functions, enable [WINDOW_FUNCTIONS] in query context. (line [1], column [13])")); } } From 3bfaa2724ead109f8055f8aca6e264ead0321ee1 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Tue, 24 Oct 2023 07:56:33 +0000 Subject: [PATCH 7/9] compute postaggs (thank you Clint!) --- .../org/apache/druid/query/groupby/GroupingEngine.java | 8 ++++++++ 1 file changed, 8 insertions(+) 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 cec8260f26fe..40c336f7a71c 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 @@ -792,6 +792,14 @@ private static Iterator summaryRowIterator(GroupByQuery q) for (int i = 0; i < aggSpec.size(); i++) { resultRow.set(i, aggSpec.get(i).factorize(new AllNullColumnSelectorFactory()).get()); } + Map map = resultRow.toMap(q); + for (int i = 0; i < q.getPostAggregatorSpecs().size(); i++) { + final PostAggregator postAggregator = q.getPostAggregatorSpecs().get(i); + final Object value = postAggregator.compute(map); + + resultRow.set(q.getResultRowPostAggregatorStart() + i, value); + map.put(postAggregator.getName(), value); + } return Collections.singleton(resultRow).iterator(); } From 9a567b67d821bdffce2838c4c35b708d8f174b47 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Tue, 24 Oct 2023 13:40:20 +0000 Subject: [PATCH 8/9] enable feature for test --- .../java/org/apache/druid/sql/calcite/CalciteSysQueryTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSysQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSysQueryTest.java index 0ebd26e65539..5b0a5dd82e3b 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSysQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSysQueryTest.java @@ -20,8 +20,10 @@ package org.apache.druid.sql.calcite; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import org.apache.druid.sql.calcite.NotYetSupported.Modes; import org.apache.druid.sql.calcite.NotYetSupported.NotYetSupportedProcessor; +import org.apache.druid.sql.calcite.planner.PlannerContext; import org.junit.Rule; import org.junit.Test; @@ -53,6 +55,7 @@ public void testTasksSumOver() msqIncompatible(); testBuilder() + .queryContext(ImmutableMap.of(PlannerContext.CTX_ENABLE_WINDOW_FNS, true)) .sql("select datasource, sum(duration) over () from sys.tasks group by datasource") .expectedResults(ImmutableList.of( new Object[]{"foo", 11L}, From f61eadd0bd237f9e8bd7241636376afd73077b98 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Tue, 24 Oct 2023 20:30:05 +0000 Subject: [PATCH 9/9] ignore tests in msq --- .../druid/msq/test/CalciteSelectQueryMSQTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteSelectQueryMSQTest.java b/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteSelectQueryMSQTest.java index e9c54cfc54b4..c83ec91f454f 100644 --- a/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteSelectQueryMSQTest.java +++ b/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteSelectQueryMSQTest.java @@ -157,6 +157,18 @@ public void testQueryWithMoreThanMaxNumericInFilter() } + @Ignore + @Override + public void testUnSupportedNullsFirst() + { + } + + @Ignore + @Override + public void testUnSupportedNullsLast() + { + } + /** * Same query as {@link CalciteQueryTest#testArrayAggQueryOnComplexDatatypes}. ARRAY_AGG is not supported in MSQ currently. * Once support is added, this test can be removed and msqCompatible() can be added to the one in CalciteQueryTest.