From 6d2399434cb86e4384cdee8a1d93b11984c0c99e Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Thu, 25 Jul 2024 10:20:27 +0530 Subject: [PATCH 1/2] add check --- .../calcite/planner/DruidSqlValidator.java | 15 +++++++++++++++ .../sql/calcite/BaseCalciteQueryTest.java | 9 +++++++++ .../druid/sql/calcite/CalciteQueryTest.java | 19 +++++++++++++++++++ 3 files changed, 43 insertions(+) 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 e00a2915a2e8..12998913d44e 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 @@ -59,6 +59,7 @@ import org.apache.druid.catalog.model.facade.DatasourceFacade; import org.apache.druid.catalog.model.table.ClusterKeySpec; import org.apache.druid.common.utils.IdUtils; +import org.apache.druid.error.DruidException; import org.apache.druid.error.InvalidSqlInput; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.common.granularity.Granularity; @@ -796,6 +797,20 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) super.validateCall(call, scope); } + @Override + protected void validateWindowClause(SqlSelect select) + { + SqlNodeList windows = select.getWindowList(); + for (SqlNode sqlNode : windows) { + if (SqlUtil.containsAgg(sqlNode)) { + throw DruidException.forPersona(DruidException.Persona.USER).ofCategory(DruidException.Category.UNSUPPORTED) + .build("Aggregation inside window is currently not supported with syntax WINDOW W AS . " + + "Try providing window definition directly without alias"); + } + } + super.validateWindowClause(select); + } + @Override protected SqlNode performUnconditionalRewrites(SqlNode node, final boolean underFrom) { 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 0ad2ba48c58c..d3e45adeeaf0 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 @@ -621,6 +621,15 @@ protected static DruidExceptionMatcher invalidSqlContains(String s) return DruidExceptionMatcher.invalidSqlInput().expectMessageContains(s); } + protected static DruidExceptionMatcher unsupportedSqlContains(String s) + { + return new DruidExceptionMatcher( + DruidException.Persona.USER, + DruidException.Category.UNSUPPORTED, + "general" + ).expectMessageContains(s); + } + @RegisterExtension static SqlTestFrameworkConfig.Rule queryFrameworkRule = new SqlTestFrameworkConfig.Rule(); 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 7b5749bd8a16..d2f72a81e254 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 @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import org.apache.calcite.rel.RelNode; import org.apache.calcite.runtime.CalciteContextException; import org.apache.druid.common.config.NullHandling; import org.apache.druid.error.DruidException; @@ -149,6 +150,7 @@ import java.util.stream.Collectors; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeFalse; @@ -15592,6 +15594,23 @@ public void testDistinctNotSupportedWithWindow() assertThat(e, invalidSqlContains("DISTINCT is not supported for window functions")); } + @Test + public void testUnSupportedAggInSelectWindow() + { + assertEquals( + "1.37.0", + RelNode.class.getPackage().getImplementationVersion(), + "Calcite version changed; check if CALCITE-6500 is fixed and update:\n * method DruidSqlValidator#validateWindowClause" + ); + + DruidException e = assertThrows(DruidException.class, () -> testBuilder() + .queryContext(ImmutableMap.of(PlannerContext.CTX_ENABLE_WINDOW_FNS, true)) + .sql("SELECT dim1, ROW_NUMBER() OVER W from druid.foo WINDOW W as (ORDER BY max(length(dim1)))") + .run()); + + assertThat(e, unsupportedSqlContains("not supported with syntax WINDOW W AS ")); + } + @Test public void testInGroupByLimitOutGroupByOrderBy() { From 4eeaff3854bc686fb04c637b5798bd2435616f54 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Fri, 26 Jul 2024 08:09:57 +0530 Subject: [PATCH 2/2] comments --- .../druid/sql/calcite/planner/DruidSqlValidator.java | 9 +++++---- .../apache/druid/sql/calcite/BaseCalciteQueryTest.java | 9 --------- .../org/apache/druid/sql/calcite/CalciteQueryTest.java | 2 +- 3 files changed, 6 insertions(+), 14 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 12998913d44e..16d3541e96c6 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 @@ -59,7 +59,6 @@ import org.apache.druid.catalog.model.facade.DatasourceFacade; import org.apache.druid.catalog.model.table.ClusterKeySpec; import org.apache.druid.common.utils.IdUtils; -import org.apache.druid.error.DruidException; import org.apache.druid.error.InvalidSqlInput; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.common.granularity.Granularity; @@ -803,9 +802,11 @@ protected void validateWindowClause(SqlSelect select) SqlNodeList windows = select.getWindowList(); for (SqlNode sqlNode : windows) { if (SqlUtil.containsAgg(sqlNode)) { - throw DruidException.forPersona(DruidException.Persona.USER).ofCategory(DruidException.Category.UNSUPPORTED) - .build("Aggregation inside window is currently not supported with syntax WINDOW W AS . " - + "Try providing window definition directly without alias"); + throw buildCalciteContextException( + "Aggregation inside window is currently not supported with syntax WINDOW W AS . " + + "Try providing window definition directly without alias", + sqlNode + ); } } super.validateWindowClause(select); 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 d3e45adeeaf0..0ad2ba48c58c 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 @@ -621,15 +621,6 @@ protected static DruidExceptionMatcher invalidSqlContains(String s) return DruidExceptionMatcher.invalidSqlInput().expectMessageContains(s); } - protected static DruidExceptionMatcher unsupportedSqlContains(String s) - { - return new DruidExceptionMatcher( - DruidException.Persona.USER, - DruidException.Category.UNSUPPORTED, - "general" - ).expectMessageContains(s); - } - @RegisterExtension static SqlTestFrameworkConfig.Rule queryFrameworkRule = new SqlTestFrameworkConfig.Rule(); 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 d2f72a81e254..36ebac6a226a 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 @@ -15608,7 +15608,7 @@ public void testUnSupportedAggInSelectWindow() .sql("SELECT dim1, ROW_NUMBER() OVER W from druid.foo WINDOW W as (ORDER BY max(length(dim1)))") .run()); - assertThat(e, unsupportedSqlContains("not supported with syntax WINDOW W AS ")); + assertThat(e, invalidSqlContains("not supported with syntax WINDOW W AS ")); } @Test