From 8153a081bcc7cd3f31e94fb2a8a3a072e72d5f79 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Tue, 16 Jul 2024 10:49:13 +0530 Subject: [PATCH 1/8] better exceptions --- .../sql/VarianceSqlAggregatorTest.java | 21 +++++++++++++ .../calcite/planner/DruidSqlValidator.java | 21 +++++++++++++ .../druid/sql/calcite/rel/Windowing.java | 4 +-- .../druid/sql/calcite/CalciteQueryTest.java | 30 +++++++++++++++++++ 4 files changed, 73 insertions(+), 3 deletions(-) diff --git a/extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/sql/VarianceSqlAggregatorTest.java b/extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/sql/VarianceSqlAggregatorTest.java index 687e26e9ff27..9c839f88635c 100644 --- a/extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/sql/VarianceSqlAggregatorTest.java +++ b/extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/sql/VarianceSqlAggregatorTest.java @@ -29,6 +29,7 @@ import org.apache.druid.data.input.impl.DoubleDimensionSchema; import org.apache.druid.data.input.impl.FloatDimensionSchema; import org.apache.druid.data.input.impl.LongDimensionSchema; +import org.apache.druid.error.DruidException; import org.apache.druid.guice.DruidInjectorBuilder; import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.math.expr.ExprMacroTable; @@ -64,6 +65,7 @@ import org.apache.druid.sql.calcite.TempDirProducer; import org.apache.druid.sql.calcite.filtration.Filtration; import org.apache.druid.sql.calcite.planner.PlannerContext; +import org.apache.druid.sql.calcite.run.EngineFeature; import org.apache.druid.sql.calcite.util.CalciteTests; import org.apache.druid.sql.calcite.util.SqlTestFramework.StandardComponentSupplier; import org.apache.druid.sql.calcite.util.TestDataBuilder; @@ -73,6 +75,9 @@ import java.util.List; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + @SqlTestFrameworkConfig.ComponentSupplier(VarianceComponentSupplier.class) public class VarianceSqlAggregatorTest extends BaseCalciteQueryTest { @@ -724,4 +729,20 @@ public void testOverWindow() )) .run(); } + + @Test + public void testStddevNotSupportedOverWindow() + { + assumeFeatureAvailable(EngineFeature.WINDOW_FUNCTIONS); + + DruidException e = assertThrows( + DruidException.class, + () -> testBuilder() + .queryContext(ImmutableMap.of(PlannerContext.CTX_ENABLE_WINDOW_FNS, true)) + .sql("SELECT stddev(m1) OVER () from numfoo") + .run() + ); + + assertTrue(e.getMessage().contains("Aggregation [STDDEV] is currently not supported for window functions")); + } } 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 75778daf5593..c8221aabb8be 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 @@ -36,6 +36,7 @@ import org.apache.calcite.sql.SqlNodeList; import org.apache.calcite.sql.SqlOperatorTable; import org.apache.calcite.sql.SqlSelect; +import org.apache.calcite.sql.SqlSelectKeyword; import org.apache.calcite.sql.SqlUpdate; import org.apache.calcite.sql.SqlUtil; import org.apache.calcite.sql.SqlWindow; @@ -110,6 +111,10 @@ protected DruidSqlValidator( @Override public void validateWindow(SqlNode windowOrId, SqlValidatorScope scope, @Nullable SqlCall call) { + if (call.getFunctionQuantifier() != null && call.getFunctionQuantifier().getValue() == SqlSelectKeyword.DISTINCT) { + throw buildCalciteContextException("DISTINCT is not supported for window functions", windowOrId); + } + final SqlWindow targetWindow; switch (windowOrId.getKind()) { case IDENTIFIER: @@ -771,6 +776,15 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) throw buildCalciteContextException("ASCENDING ordering with NULLS LAST is not supported!", call); } } + + if (plannerContext.getPlannerConfig().isUseApproximateCountDistinct()) { + if (isSqlCallDistinct(call) && call.getOperator().getKind() != SqlKind.COUNT) { + throw buildCalciteContextException( + "Only COUNT with DISTINCT is supported when useApproximateCountDistinct is enabled. Run with disabling it.", + call + ); + } + } super.validateCall(call, scope); } @@ -857,4 +871,11 @@ private SqlNode getSqlNodeFor(SqlInsert insert, int idx) } return src; } + + private boolean isSqlCallDistinct(@Nullable SqlCall call) + { + return call != null + && call.getFunctionQuantifier() != null + && call.getFunctionQuantifier().getValue() == SqlSelectKeyword.DISTINCT; + } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java index afd775ef4eef..d4b91de7bbf4 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java @@ -166,9 +166,7 @@ public static Windowing fromCalciteStuff( if (aggregation == null || aggregation.getPostAggregator() != null || aggregation.getAggregatorFactories().size() != 1) { - if (null == plannerContext.getPlanningError()) { - plannerContext.setPlanningError("Aggregation [%s] is not supported", aggregateCall); - } + plannerContext.setPlanningError("Aggregation [%s] is currently not supported for window functions", aggregateCall.getAggregation().getName()); throw new CannotBuildQueryException(window, aggregateCall); } 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 2ae095d41c74..c829b5aefa35 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 @@ -15577,6 +15577,36 @@ public void testNtileNotSupportedWithFrame() assertThat(e, invalidSqlContains("Framing of NTILE is not supported")); } + @Test + public void testDistinctNotSupportedWithWindow() + { + assumeFeatureAvailable(EngineFeature.WINDOW_FUNCTIONS); + + DruidException e = assertThrows( + DruidException.class, + () -> testBuilder() + .queryContext(ImmutableMap.of(PlannerContext.CTX_ENABLE_WINDOW_FNS, true)) + .sql("SELECT count(distinct dim1) OVER () from druid.foo") + .run() + ); + + assertThat(e, invalidSqlContains("DISTINCT is not supported for window functions")); + } + + @Test + public void testDistinctSumNotSupportedWithApproximation() + { + DruidException e = assertThrows( + DruidException.class, + () -> testBuilder() + .queryContext(ImmutableMap.of(PlannerConfig.CTX_KEY_USE_APPROXIMATE_COUNT_DISTINCT, true)) + .sql("SELECT sum(distinct m1) from druid.foo") + .run() + ); + + assertThat(e, invalidSqlContains("Only COUNT with DISTINCT is supported")); + } + @Test public void testInGroupByLimitOutGroupByOrderBy() { From b573be5367c9f797df466f7b32c4e47bfdef94f9 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Tue, 16 Jul 2024 11:01:52 +0530 Subject: [PATCH 2/8] refactor --- .../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 c8221aabb8be..6b79509e8a5a 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 @@ -111,7 +111,7 @@ protected DruidSqlValidator( @Override public void validateWindow(SqlNode windowOrId, SqlValidatorScope scope, @Nullable SqlCall call) { - if (call.getFunctionQuantifier() != null && call.getFunctionQuantifier().getValue() == SqlSelectKeyword.DISTINCT) { + if (isSqlCallDistinct(call)) { throw buildCalciteContextException("DISTINCT is not supported for window functions", windowOrId); } From 708db8c725f289bc9e845ae6beb3652cffa3c98a Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Tue, 16 Jul 2024 15:40:04 +0530 Subject: [PATCH 3/8] post agg with over --- .../org/apache/druid/sql/calcite/expression/Expressions.java | 4 ++++ 1 file changed, 4 insertions(+) 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 ec518ee4522f..b119346de379 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; @@ -238,6 +239,9 @@ public static DruidExpression toDruidExpressionWithPostAggOperands( final SqlKind kind = rexNode.getKind(); if (kind == SqlKind.INPUT_REF) { return inputRefToDruidExpression(rowSignature, rexNode); + } else if (rexNode instanceof RexOver) { + plannerContext.setPlanningError("Encountered an OVER during Filter translation [%s].", rexNode); + return null; } else if (rexNode instanceof RexCall) { return rexCallToDruidExpression(plannerContext, rowSignature, rexNode, postAggregatorVisitor); } else if (kind == SqlKind.LITERAL) { From d35247a353f8d9c5b86e3b356e4e8af4dafe41ad Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Thu, 18 Jul 2024 08:14:07 +0530 Subject: [PATCH 4/8] refactor --- .../sql/VarianceSqlAggregatorTest.java | 7 +- .../sql/calcite/expression/Expressions.java | 4 - .../calcite/planner/DruidSqlValidator.java | 9 -- .../druid/sql/calcite/rel/Windowing.java | 2 +- .../sql/calcite/CalciteWindowQueryTest.java | 91 ------------------- .../tests/window/allBoundsCombination.sqlTest | 38 ++++++++ .../tests/window/duplicateAggregation.sqlTest | 10 ++ .../tests/window/windowInsideSubquery.sqlTest | 19 ++++ 8 files changed, 73 insertions(+), 107 deletions(-) create mode 100644 sql/src/test/resources/calcite/tests/window/allBoundsCombination.sqlTest create mode 100644 sql/src/test/resources/calcite/tests/window/duplicateAggregation.sqlTest create mode 100644 sql/src/test/resources/calcite/tests/window/windowInsideSubquery.sqlTest diff --git a/extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/sql/VarianceSqlAggregatorTest.java b/extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/sql/VarianceSqlAggregatorTest.java index 9c839f88635c..9c62e2969ca6 100644 --- a/extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/sql/VarianceSqlAggregatorTest.java +++ b/extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/sql/VarianceSqlAggregatorTest.java @@ -75,8 +75,8 @@ import java.util.List; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; @SqlTestFrameworkConfig.ComponentSupplier(VarianceComponentSupplier.class) public class VarianceSqlAggregatorTest extends BaseCalciteQueryTest @@ -743,6 +743,9 @@ public void testStddevNotSupportedOverWindow() .run() ); - assertTrue(e.getMessage().contains("Aggregation [STDDEV] is currently not supported for window functions")); + assertEquals( + "Query could not be planned. A possible reason is [Aggregation [STDDEV] is currently not supported for window functions]", + e.getMessage() + ); } } 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 b119346de379..ec518ee4522f 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; @@ -239,9 +238,6 @@ public static DruidExpression toDruidExpressionWithPostAggOperands( final SqlKind kind = rexNode.getKind(); if (kind == SqlKind.INPUT_REF) { return inputRefToDruidExpression(rowSignature, rexNode); - } else if (rexNode instanceof RexOver) { - plannerContext.setPlanningError("Encountered an OVER during Filter translation [%s].", rexNode); - return null; } else if (rexNode instanceof RexCall) { return rexCallToDruidExpression(plannerContext, rowSignature, rexNode, postAggregatorVisitor); } else if (kind == SqlKind.LITERAL) { 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 6b79509e8a5a..18638b32afd8 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 @@ -776,15 +776,6 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) throw buildCalciteContextException("ASCENDING ordering with NULLS LAST is not supported!", call); } } - - if (plannerContext.getPlannerConfig().isUseApproximateCountDistinct()) { - if (isSqlCallDistinct(call) && call.getOperator().getKind() != SqlKind.COUNT) { - throw buildCalciteContextException( - "Only COUNT with DISTINCT is supported when useApproximateCountDistinct is enabled. Run with disabling it.", - call - ); - } - } super.validateCall(call, scope); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java b/sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java index d4b91de7bbf4..39c180530f0c 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java @@ -160,7 +160,7 @@ public static Windowing fromCalciteStuff( Collections.emptyList(), aggName, aggregateCall, - false // Windowed aggregations don't currently finalize. This means that sketches won't work as expected. + false // Windowed aggregations finalize later when we write the computed value to result RAC ); if (aggregation == null diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java index b3d657b148f1..99a4c0f44488 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java @@ -22,7 +22,6 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import org.apache.druid.jackson.DefaultObjectMapper; import org.apache.druid.java.util.common.ISE; @@ -38,7 +37,6 @@ import org.apache.druid.sql.calcite.QueryVerification.QueryResultsVerifier; import org.apache.druid.sql.calcite.planner.PlannerContext; import org.junit.Assert; -import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; @@ -231,95 +229,6 @@ public void windowQueryTestWithCustomContextMaxSubqueryBytes(String filename) th } } - @Test - public void testEmptyWindowInSubquery() - { - testBuilder() - .sql( - "select c from (\n" - + " select channel, row_number() over () as c\n" - + " from wikipedia\n" - + " group by channel\n" - + ") LIMIT 5" - ) - .queryContext(ImmutableMap.of( - PlannerContext.CTX_ENABLE_WINDOW_FNS, true, - QueryContexts.ENABLE_DEBUG, true - )) - .expectedResults(ImmutableList.of( - new Object[]{1L}, - new Object[]{2L}, - new Object[]{3L}, - new Object[]{4L}, - new Object[]{5L} - )) - .run(); - } - - @Test - public void testWindow() - { - testBuilder() - .sql("SELECT\n" + - "(rank() over (order by count(*) desc)),\n" + - "(rank() over (order by count(*) desc))\n" + - "FROM \"wikipedia\"") - .queryContext(ImmutableMap.of( - PlannerContext.CTX_ENABLE_WINDOW_FNS, true, - QueryContexts.ENABLE_DEBUG, true - )) - .expectedResults(ImmutableList.of( - new Object[]{1L, 1L} - )) - .run(); - } - - @Test - public void testWindowAllBoundsCombination() - { - testBuilder() - .sql("select\n" - + "cityName,\n" - + "count(*) over (partition by cityName order by countryName rows between unbounded preceding and 1 preceding) c1,\n" - + "count(*) over (partition by cityName order by countryName rows between unbounded preceding and current row) c2,\n" - + "count(*) over (partition by cityName order by countryName rows between unbounded preceding and 1 following) c3,\n" - + "count(*) over (partition by cityName order by countryName rows between unbounded preceding and unbounded following) c4,\n" - + "count(*) over (partition by cityName order by countryName rows between 3 preceding and 1 preceding) c5,\n" - + "count(*) over (partition by cityName order by countryName rows between 1 preceding and current row) c6,\n" - + "count(*) over (partition by cityName order by countryName rows between 1 preceding and 1 FOLLOWING) c7,\n" - + "count(*) over (partition by cityName order by countryName rows between 1 preceding and unbounded FOLLOWING) c8,\n" - + "count(*) over (partition by cityName order by countryName rows between 1 FOLLOWING and unbounded FOLLOWING) c9,\n" - + "count(*) over (partition by cityName order by countryName rows between 1 FOLLOWING and 3 FOLLOWING) c10,\n" - + "count(*) over (partition by cityName order by countryName rows between current row and 1 following) c11,\n" - + "count(*) over (partition by cityName order by countryName rows between current row and unbounded following) c12\n" - + "from wikipedia\n" - + "where cityName in ('Vienna', 'Seoul')\n" - + "group by countryName, cityName, added") - .queryContext(ImmutableMap.of( - PlannerContext.CTX_ENABLE_WINDOW_FNS, true, - QueryContexts.ENABLE_DEBUG, true - )) - .expectedResults(ImmutableList.of( - new Object[]{"Seoul", 0L, 1L, 2L, 13L, 0L, 1L, 2L, 13L, 12L, 3L, 2L, 13L}, - new Object[]{"Seoul", 1L, 2L, 3L, 13L, 1L, 2L, 3L, 13L, 11L, 3L, 2L, 12L}, - new Object[]{"Seoul", 2L, 3L, 4L, 13L, 2L, 2L, 3L, 12L, 10L, 3L, 2L, 11L}, - new Object[]{"Seoul", 3L, 4L, 5L, 13L, 3L, 2L, 3L, 11L, 9L, 3L, 2L, 10L}, - new Object[]{"Seoul", 4L, 5L, 6L, 13L, 3L, 2L, 3L, 10L, 8L, 3L, 2L, 9L}, - new Object[]{"Seoul", 5L, 6L, 7L, 13L, 3L, 2L, 3L, 9L, 7L, 3L, 2L, 8L}, - new Object[]{"Seoul", 6L, 7L, 8L, 13L, 3L, 2L, 3L, 8L, 6L, 3L, 2L, 7L}, - new Object[]{"Seoul", 7L, 8L, 9L, 13L, 3L, 2L, 3L, 7L, 5L, 3L, 2L, 6L}, - new Object[]{"Seoul", 8L, 9L, 10L, 13L, 3L, 2L, 3L, 6L, 4L, 3L, 2L, 5L}, - new Object[]{"Seoul", 9L, 10L, 11L, 13L, 3L, 2L, 3L, 5L, 3L, 3L, 2L, 4L}, - new Object[]{"Seoul", 10L, 11L, 12L, 13L, 3L, 2L, 3L, 4L, 2L, 2L, 2L, 3L}, - new Object[]{"Seoul", 11L, 12L, 13L, 13L, 3L, 2L, 3L, 3L, 1L, 1L, 2L, 2L}, - new Object[]{"Seoul", 12L, 13L, 13L, 13L, 3L, 2L, 2L, 2L, 0L, 0L, 1L, 1L}, - new Object[]{"Vienna", 0L, 1L, 2L, 3L, 0L, 1L, 2L, 3L, 2L, 2L, 2L, 3L}, - new Object[]{"Vienna", 1L, 2L, 3L, 3L, 1L, 2L, 3L, 3L, 1L, 1L, 2L, 2L}, - new Object[]{"Vienna", 2L, 3L, 3L, 3L, 2L, 2L, 2L, 2L, 0L, 0L, 1L, 1L} - )) - .run(); - } - private WindowOperatorQuery getWindowOperatorQuery(List> queries) { assertEquals(1, queries.size()); diff --git a/sql/src/test/resources/calcite/tests/window/allBoundsCombination.sqlTest b/sql/src/test/resources/calcite/tests/window/allBoundsCombination.sqlTest new file mode 100644 index 000000000000..af6b0451761c --- /dev/null +++ b/sql/src/test/resources/calcite/tests/window/allBoundsCombination.sqlTest @@ -0,0 +1,38 @@ +type: "operatorValidation" + +sql: | + select + cityName, + count(*) over (partition by cityName order by countryName rows between unbounded preceding and 1 preceding) c1, + count(*) over (partition by cityName order by countryName rows between unbounded preceding and current row) c2, + count(*) over (partition by cityName order by countryName rows between unbounded preceding and 1 following) c3, + count(*) over (partition by cityName order by countryName rows between unbounded preceding and unbounded following) c4, + count(*) over (partition by cityName order by countryName rows between 3 preceding and 1 preceding) c5, + count(*) over (partition by cityName order by countryName rows between 1 preceding and current row) c6, + count(*) over (partition by cityName order by countryName rows between 1 preceding and 1 FOLLOWING) c7, + count(*) over (partition by cityName order by countryName rows between 1 preceding and unbounded FOLLOWING) c8, + count(*) over (partition by cityName order by countryName rows between 1 FOLLOWING and unbounded FOLLOWING) c9, + count(*) over (partition by cityName order by countryName rows between 1 FOLLOWING and 3 FOLLOWING) c10, + count(*) over (partition by cityName order by countryName rows between current row and 1 following) c11, + count(*) over (partition by cityName order by countryName rows between current row and unbounded following) c12 + from wikipedia + where cityName in ('Vienna', 'Seoul') + group by countryName, cityName, added + +expectedResults: + - ["Seoul",0,1,2,13,0,1,2,13,12,3,2,13] + - ["Seoul",1,2,3,13,1,2,3,13,11,3,2,12] + - ["Seoul",2,3,4,13,2,2,3,12,10,3,2,11] + - ["Seoul",3,4,5,13,3,2,3,11,9,3,2,10] + - ["Seoul",4,5,6,13,3,2,3,10,8,3,2,9] + - ["Seoul",5,6,7,13,3,2,3,9,7,3,2,8] + - ["Seoul",6,7,8,13,3,2,3,8,6,3,2,7] + - ["Seoul",7,8,9,13,3,2,3,7,5,3,2,6] + - ["Seoul",8,9,10,13,3,2,3,6,4,3,2,5] + - ["Seoul",9,10,11,13,3,2,3,5,3,3,2,4] + - ["Seoul",10,11,12,13,3,2,3,4,2,2,2,3] + - ["Seoul",11,12,13,13,3,2,3,3,1,1,2,2] + - ["Seoul",12,13,13,13,3,2,2,2,0,0,1,1] + - ["Vienna",0,1,2,3,0,1,2,3,2,2,2,3] + - ["Vienna",1,2,3,3,1,2,3,3,1,1,2,2] + - ["Vienna",2,3,3,3,2,2,2,2,0,0,1,1] diff --git a/sql/src/test/resources/calcite/tests/window/duplicateAggregation.sqlTest b/sql/src/test/resources/calcite/tests/window/duplicateAggregation.sqlTest new file mode 100644 index 000000000000..fd79234a5624 --- /dev/null +++ b/sql/src/test/resources/calcite/tests/window/duplicateAggregation.sqlTest @@ -0,0 +1,10 @@ +type: "operatorValidation" + +sql: | + select + rank() over (order by count(*) desc), + rank() over (order by count(*) desc) + from wikipedia + +expectedResults: + - [1,1] diff --git a/sql/src/test/resources/calcite/tests/window/windowInsideSubquery.sqlTest b/sql/src/test/resources/calcite/tests/window/windowInsideSubquery.sqlTest new file mode 100644 index 000000000000..5672a5a17f35 --- /dev/null +++ b/sql/src/test/resources/calcite/tests/window/windowInsideSubquery.sqlTest @@ -0,0 +1,19 @@ +type: "operatorValidation" + +sql: | + select + c + from + ( + select channel, row_number() over () as c + from wikipedia + group by channel + ) + LIMIT 5 + +expectedResults: + - [1] + - [2] + - [3] + - [4] + - [5] From a38b782dc7bdd44592c413d2ffa5dbf9692b3071 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Thu, 18 Jul 2024 08:48:22 +0530 Subject: [PATCH 5/8] remove test --- .../apache/druid/sql/calcite/CalciteQueryTest.java | 14 -------------- 1 file changed, 14 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 c829b5aefa35..e2972d85a259 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 @@ -15593,20 +15593,6 @@ public void testDistinctNotSupportedWithWindow() assertThat(e, invalidSqlContains("DISTINCT is not supported for window functions")); } - @Test - public void testDistinctSumNotSupportedWithApproximation() - { - DruidException e = assertThrows( - DruidException.class, - () -> testBuilder() - .queryContext(ImmutableMap.of(PlannerConfig.CTX_KEY_USE_APPROXIMATE_COUNT_DISTINCT, true)) - .sql("SELECT sum(distinct m1) from druid.foo") - .run() - ); - - assertThat(e, invalidSqlContains("Only COUNT with DISTINCT is supported")); - } - @Test public void testInGroupByLimitOutGroupByOrderBy() { From c2caecf7ec8c06abcc62c7911debafe311177cf6 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Fri, 19 Jul 2024 09:33:14 +0530 Subject: [PATCH 6/8] fix extra columns for empty window --- .../druid/query/rowsandcols/ArrayListRowsAndColumns.java | 9 +-------- .../apache/druid/sql/calcite/CalciteWindowQueryTest.java | 2 +- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java b/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java index 04f9eddbff0c..49dfcc4d134e 100644 --- a/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java +++ b/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java @@ -212,15 +212,8 @@ public T as(Class clazz) @Override public void addColumn(String name, Column column) { - if (rows.size() == numRows()) { - extraColumns.put(name, column); - columnNames.add(name); - return; - } - // When an ArrayListRowsAndColumns is only a partial view, but adds a column, it believes that the same column - // will eventually be added for all of the rows so we pre-allocate storage for the entire set of data and - // copy. + // will eventually be added for all the rows so we pre-allocate storage for the entire set of data and copy. final ColumnAccessor columnAccessor = column.toAccessor(); if (columnAccessor.numRows() != numRows()) { diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java index 6f2a7dfc9494..9fdd73fb9c74 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java @@ -38,7 +38,7 @@ import org.apache.druid.sql.calcite.QueryVerification.QueryResultsVerifier; import org.apache.druid.sql.calcite.planner.PlannerContext; import org.junit.Assert; -import org.junit.Test; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; From 447f5d9f5e9437c851339464473c8c34c1516b50 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 22 Jul 2024 08:15:25 +0530 Subject: [PATCH 7/8] optimise only when ObjectArrayColumn --- .../druid/query/rowsandcols/ArrayListRowsAndColumns.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java b/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java index 49dfcc4d134e..b053e6893749 100644 --- a/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java +++ b/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java @@ -212,6 +212,12 @@ public T as(Class clazz) @Override public void addColumn(String name, Column column) { + if (rows.size() == numRows() && column instanceof ObjectArrayColumn) { + extraColumns.put(name, column); + columnNames.add(name); + return; + } + // When an ArrayListRowsAndColumns is only a partial view, but adds a column, it believes that the same column // will eventually be added for all the rows so we pre-allocate storage for the entire set of data and copy. From c4e22c86068f56b6c458381198e52aab30f54006 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 22 Jul 2024 16:57:39 +0530 Subject: [PATCH 8/8] comments --- .../apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java b/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java index b053e6893749..05b9dee5458c 100644 --- a/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java +++ b/processing/src/main/java/org/apache/druid/query/rowsandcols/ArrayListRowsAndColumns.java @@ -212,7 +212,7 @@ public T as(Class clazz) @Override public void addColumn(String name, Column column) { - if (rows.size() == numRows() && column instanceof ObjectArrayColumn) { + if (rows.size() == numRows() && column.as(ColumnValueSwapper.class) != null) { extraColumns.put(name, column); columnNames.add(name); return;