From f770fc020809441be89b9942407ffef4c1b2f69d Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 22 Jul 2024 07:38:25 +0530 Subject: [PATCH 1/9] init --- .../aggregation/NativelySupportsDistinct.java | 64 +++++++++++++++++++ .../calcite/planner/DruidSqlValidator.java | 22 +++++++ 2 files changed, 86 insertions(+) create mode 100644 sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java new file mode 100644 index 000000000000..7603afba2156 --- /dev/null +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java @@ -0,0 +1,64 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.sql.calcite.aggregation; + +import com.google.inject.BindingAnnotation; +import org.apache.druid.error.DruidException; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; + +@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.TYPE}) +@BindingAnnotation +public @interface NativelySupportsDistinct +{ + boolean value() default false; + + class NativelySupportsDistinctProcessor implements InvocationHandler + { + private final Object target; + + public NativelySupportsDistinctProcessor(Object target) { + this.target = target; + } + + @Override + public Object invoke(Object object, Method method, Object[] args) throws Throwable { + Class clazz = target.getClass(); + + if (object instanceof SqlAggregator) { + SqlAggregator aggregator = (SqlAggregator) object; + //if (PlannerConfig.) + if (!clazz.isAnnotationPresent(NativelySupportsDistinct.class) + || !clazz.getAnnotation(NativelySupportsDistinct.class).value()) { + throw DruidException.forPersona(DruidException.Persona.USER).ofCategory(DruidException.Category.UNSUPPORTED) + .build("Aggregation [%s] is not supported with useApproximateCountDistinct enabled.", aggregator.calciteFunction().getKind()); + } + } + // Invoke the actual method + return method.invoke(target, args); + } + } +} 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..884d27449bca 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 @@ -28,6 +28,7 @@ import org.apache.calcite.rel.type.RelRecordType; import org.apache.calcite.runtime.CalciteContextException; import org.apache.calcite.runtime.CalciteException; +import org.apache.calcite.sql.SqlAggFunction; import org.apache.calcite.sql.SqlCall; import org.apache.calcite.sql.SqlIdentifier; import org.apache.calcite.sql.SqlInsert; @@ -36,6 +37,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; @@ -64,6 +66,7 @@ import org.apache.druid.segment.column.ColumnType; import org.apache.druid.segment.column.Types; import org.apache.druid.segment.column.ValueType; +import org.apache.druid.sql.calcite.aggregation.NativelySupportsDistinct; import org.apache.druid.sql.calcite.expression.builtin.ScalarInArrayOperatorConversion; import org.apache.druid.sql.calcite.parser.DruidSqlIngest; import org.apache.druid.sql.calcite.parser.DruidSqlInsert; @@ -771,6 +774,18 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) throw buildCalciteContextException("ASCENDING ordering with NULLS LAST is not supported!", call); } } +// if (plannerContext.getPlannerConfig().isUseApproximateCountDistinct() && isSqlCallDistinct(call)) { +// if (call.getOperator() instanceof SqlAggFunction) { +// Class clazz = ((SqlAggFunction) call.getOperator()).getClass(); +// if (!clazz.isAnnotationPresent(NativelySupportsDistinct.class) +// || !clazz.getAnnotation(NativelySupportsDistinct.class).value()) { +// throw buildCalciteContextException( +// "Only COUNT with DISTINCT is supported when useApproximateCountDistinct is enabled. Run with disabling it.", +// call +// ); +// } +// } +// } super.validateCall(call, scope); } @@ -857,4 +872,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; + } } From a70d5d489b44b7c3f6b7c2b3f37c60ac0c5856e1 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 22 Jul 2024 17:56:45 +0530 Subject: [PATCH 2/9] add NativelySupportsDistinct --- .../ApproxCountDistinctSqlAggregator.java | 1 + .../aggregation/NativelySupportsDistinct.java | 32 ------------------- .../builtin/ArraySqlAggregator.java | 2 ++ .../builtin/StringSqlAggregator.java | 2 ++ .../druid/sql/calcite/rule/GroupByRules.java | 12 +++++++ .../druid/sql/calcite/CalciteQueryTest.java | 14 ++++++++ 6 files changed, 31 insertions(+), 32 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/ApproxCountDistinctSqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/ApproxCountDistinctSqlAggregator.java index eceb4ebbf800..f7c57b07be0f 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/ApproxCountDistinctSqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/ApproxCountDistinctSqlAggregator.java @@ -83,6 +83,7 @@ public Aggregation toDruidAggregation( ); } + @NativelySupportsDistinct private static class ApproxCountDistinctSqlAggFunction extends SqlAggFunction { ApproxCountDistinctSqlAggFunction() diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java index 7603afba2156..0f68dc388919 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java @@ -19,46 +19,14 @@ package org.apache.druid.sql.calcite.aggregation; -import com.google.inject.BindingAnnotation; -import org.apache.druid.error.DruidException; - import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; -import java.lang.reflect.InvocationHandler; -import java.lang.reflect.Method; @Retention(RetentionPolicy.RUNTIME) @Target({ElementType.TYPE}) -@BindingAnnotation public @interface NativelySupportsDistinct { - boolean value() default false; - - class NativelySupportsDistinctProcessor implements InvocationHandler - { - private final Object target; - - public NativelySupportsDistinctProcessor(Object target) { - this.target = target; - } - - @Override - public Object invoke(Object object, Method method, Object[] args) throws Throwable { - Class clazz = target.getClass(); - if (object instanceof SqlAggregator) { - SqlAggregator aggregator = (SqlAggregator) object; - //if (PlannerConfig.) - if (!clazz.isAnnotationPresent(NativelySupportsDistinct.class) - || !clazz.getAnnotation(NativelySupportsDistinct.class).value()) { - throw DruidException.forPersona(DruidException.Persona.USER).ofCategory(DruidException.Category.UNSUPPORTED) - .build("Aggregation [%s] is not supported with useApproximateCountDistinct enabled.", aggregator.calciteFunction().getKind()); - } - } - // Invoke the actual method - return method.invoke(target, args); - } - } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArraySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArraySqlAggregator.java index efb84dca6251..1045a79870bb 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArraySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArraySqlAggregator.java @@ -41,6 +41,7 @@ import org.apache.druid.query.aggregation.ExpressionLambdaAggregatorFactory; import org.apache.druid.segment.column.ColumnType; import org.apache.druid.sql.calcite.aggregation.Aggregation; +import org.apache.druid.sql.calcite.aggregation.NativelySupportsDistinct; import org.apache.druid.sql.calcite.aggregation.SqlAggregator; import org.apache.druid.sql.calcite.expression.DruidExpression; import org.apache.druid.sql.calcite.expression.Expressions; @@ -165,6 +166,7 @@ public RelDataType inferReturnType(SqlOperatorBinding sqlOperatorBinding) } } + @NativelySupportsDistinct private static class ArrayAggFunction extends SqlAggFunction { private static final ArrayAggReturnTypeInference RETURN_TYPE_INFERENCE = new ArrayAggReturnTypeInference(); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/StringSqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/StringSqlAggregator.java index a78b3a7a4797..49469decf996 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/StringSqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/StringSqlAggregator.java @@ -47,6 +47,7 @@ import org.apache.druid.query.filter.SelectorDimFilter; import org.apache.druid.segment.column.ColumnType; import org.apache.druid.sql.calcite.aggregation.Aggregation; +import org.apache.druid.sql.calcite.aggregation.NativelySupportsDistinct; import org.apache.druid.sql.calcite.aggregation.SqlAggregator; import org.apache.druid.sql.calcite.expression.DruidExpression; import org.apache.druid.sql.calcite.expression.Expressions; @@ -226,6 +227,7 @@ public RelDataType inferReturnType(SqlOperatorBinding sqlOperatorBinding) } } + @NativelySupportsDistinct private static class StringAggFunction extends SqlAggFunction { private static final StringAggReturnTypeInference RETURN_TYPE_INFERENCE = new StringAggReturnTypeInference(); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java b/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java index fecabd00ec39..9e1268bced5f 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java @@ -22,11 +22,13 @@ import org.apache.calcite.rel.core.AggregateCall; import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlKind; import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.FilteredAggregatorFactory; import org.apache.druid.query.filter.DimFilter; import org.apache.druid.segment.column.RowSignature; import org.apache.druid.sql.calcite.aggregation.Aggregation; +import org.apache.druid.sql.calcite.aggregation.NativelySupportsDistinct; import org.apache.druid.sql.calcite.aggregation.SqlAggregator; import org.apache.druid.sql.calcite.expression.Expressions; import org.apache.druid.sql.calcite.filtration.Filtration; @@ -69,6 +71,16 @@ public static Aggregation translateAggregateCall( return null; } + if (plannerContext.getPlannerConfig().isUseApproximateCountDistinct() && call.isDistinct()) { + if (call.getAggregation().getKind() != SqlKind.COUNT + && !call.getAggregation().getClass().isAnnotationPresent(NativelySupportsDistinct.class)) { + plannerContext.setPlanningError( + "Aggregation [%s] with DISTINCT is not supported when useApproximateCountDistinct is enabled. Run with disabling it.", + call.getAggregation().getKind() + ); + } + } + final DimFilter filter; if (call.filterArg >= 0) { 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..04e4ddeb84ed 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 @@ -15516,6 +15516,20 @@ public void testWindowingErrorWithoutFeatureFlag() assertThat(e, invalidSqlIs("The query contains window functions; To run these window functions, specify [enableWindowing] in query context. (line [1], column [13])")); } + @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("Aggregation [SUM] with DISTINCT is not supported")); + } + @Test public void testUnSupportedNullsFirst() { From 8eb5a60a6a61dc9821231a39eae18eb82a31c479 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 22 Jul 2024 18:07:56 +0530 Subject: [PATCH 3/9] refactor --- .../calcite/planner/DruidSqlValidator.java | 36 +++++++++++-------- .../druid/sql/calcite/rule/GroupByRules.java | 1 + 2 files changed, 23 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 884d27449bca..ed27b201c139 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 @@ -67,6 +67,7 @@ import org.apache.druid.segment.column.Types; import org.apache.druid.segment.column.ValueType; import org.apache.druid.sql.calcite.aggregation.NativelySupportsDistinct; +import org.apache.druid.sql.calcite.aggregation.SqlAggregator; import org.apache.druid.sql.calcite.expression.builtin.ScalarInArrayOperatorConversion; import org.apache.druid.sql.calcite.parser.DruidSqlIngest; import org.apache.druid.sql.calcite.parser.DruidSqlInsert; @@ -758,8 +759,10 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) throw buildCalciteContextException( StringUtils.format( "The query contains window functions; To run these window functions, specify [%s] in query context.", - PlannerContext.CTX_ENABLE_WINDOW_FNS), - call); + PlannerContext.CTX_ENABLE_WINDOW_FNS + ), + call + ); } } if (call.getKind() == SqlKind.NULLS_FIRST) { @@ -774,18 +777,23 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) throw buildCalciteContextException("ASCENDING ordering with NULLS LAST is not supported!", call); } } -// if (plannerContext.getPlannerConfig().isUseApproximateCountDistinct() && isSqlCallDistinct(call)) { -// if (call.getOperator() instanceof SqlAggFunction) { -// Class clazz = ((SqlAggFunction) call.getOperator()).getClass(); -// if (!clazz.isAnnotationPresent(NativelySupportsDistinct.class) -// || !clazz.getAnnotation(NativelySupportsDistinct.class).value()) { -// throw buildCalciteContextException( -// "Only COUNT with DISTINCT is supported when useApproximateCountDistinct is enabled. Run with disabling it.", -// call -// ); -// } -// } -// } + if (plannerContext.getPlannerConfig().isUseApproximateCountDistinct() && isSqlCallDistinct(call)) { + Class clazz = null; + if (call.getOperator() instanceof SqlAggFunction || call.getOperator() instanceof SqlAggregator) { + clazz = call.getOperator().getClass(); + } + if (call.getOperator().getKind() != SqlKind.COUNT) { + if (clazz != null && !clazz.isAnnotationPresent(NativelySupportsDistinct.class)) { + throw buildCalciteContextException( + String.format( + "Aggregation [%s] with DISTINCT is not supported when useApproximateCountDistinct is enabled. Run with disabling it.", + call.getOperator().getName() + ), + call + ); + } + } + } super.validateCall(call, scope); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java b/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java index 9e1268bced5f..dbe774be6c71 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java @@ -78,6 +78,7 @@ public static Aggregation translateAggregateCall( "Aggregation [%s] with DISTINCT is not supported when useApproximateCountDistinct is enabled. Run with disabling it.", call.getAggregation().getKind() ); + return null; } } From 217641690c86db30ae8500f0e891972af1d34c45 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 22 Jul 2024 18:21:54 +0530 Subject: [PATCH 4/9] javadoc --- .../sql/calcite/aggregation/NativelySupportsDistinct.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java index 0f68dc388919..f8c5a512decf 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java @@ -24,6 +24,10 @@ import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +/** + * This annotation is to distinguish {@link SqlAggregator} or {@link org.apache.calcite.sql.SqlAggFunction} + * which supports the distinct aggregation natively + */ @Retention(RetentionPolicy.RUNTIME) @Target({ElementType.TYPE}) public @interface NativelySupportsDistinct From 9b12cddbc1d7b75951c6e3ffd04f09185f14b4b8 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 22 Jul 2024 18:26:21 +0530 Subject: [PATCH 5/9] 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 ed27b201c139..34c17b5899e7 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 @@ -785,7 +785,7 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) if (call.getOperator().getKind() != SqlKind.COUNT) { if (clazz != null && !clazz.isAnnotationPresent(NativelySupportsDistinct.class)) { throw buildCalciteContextException( - String.format( + StringUtils.format( "Aggregation [%s] with DISTINCT is not supported when useApproximateCountDistinct is enabled. Run with disabling it.", call.getOperator().getName() ), From 623338b507b7e89f7ab2bae62c974177277ea4ed Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 22 Jul 2024 19:04:29 +0530 Subject: [PATCH 6/9] fix tests --- .../calcite/aggregation/NativelySupportsDistinct.java | 2 +- .../aggregation/builtin/ArrayConcatSqlAggregator.java | 2 ++ .../druid/sql/calcite/planner/DruidSqlValidator.java | 9 ++------- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java index f8c5a512decf..19bbaf8a0f26 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/NativelySupportsDistinct.java @@ -25,7 +25,7 @@ import java.lang.annotation.Target; /** - * This annotation is to distinguish {@link SqlAggregator} or {@link org.apache.calcite.sql.SqlAggFunction} + * This annotation is to distinguish {@link org.apache.calcite.sql.SqlAggFunction} * which supports the distinct aggregation natively */ @Retention(RetentionPolicy.RUNTIME) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArrayConcatSqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArrayConcatSqlAggregator.java index a5e62f5e2a9b..d20999d3afc4 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArrayConcatSqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/ArrayConcatSqlAggregator.java @@ -39,6 +39,7 @@ import org.apache.druid.segment.VirtualColumn; import org.apache.druid.segment.column.ColumnType; import org.apache.druid.sql.calcite.aggregation.Aggregation; +import org.apache.druid.sql.calcite.aggregation.NativelySupportsDistinct; import org.apache.druid.sql.calcite.aggregation.SqlAggregator; import org.apache.druid.sql.calcite.expression.DruidExpression; import org.apache.druid.sql.calcite.expression.Expressions; @@ -142,6 +143,7 @@ public Aggregation toDruidAggregation( } } + @NativelySupportsDistinct private static class ArrayConcatAggFunction extends SqlAggFunction { ArrayConcatAggFunction() 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 34c17b5899e7..0ababa23819a 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 @@ -67,7 +67,6 @@ import org.apache.druid.segment.column.Types; import org.apache.druid.segment.column.ValueType; import org.apache.druid.sql.calcite.aggregation.NativelySupportsDistinct; -import org.apache.druid.sql.calcite.aggregation.SqlAggregator; import org.apache.druid.sql.calcite.expression.builtin.ScalarInArrayOperatorConversion; import org.apache.druid.sql.calcite.parser.DruidSqlIngest; import org.apache.druid.sql.calcite.parser.DruidSqlInsert; @@ -778,12 +777,8 @@ public void validateCall(SqlCall call, SqlValidatorScope scope) } } if (plannerContext.getPlannerConfig().isUseApproximateCountDistinct() && isSqlCallDistinct(call)) { - Class clazz = null; - if (call.getOperator() instanceof SqlAggFunction || call.getOperator() instanceof SqlAggregator) { - clazz = call.getOperator().getClass(); - } - if (call.getOperator().getKind() != SqlKind.COUNT) { - if (clazz != null && !clazz.isAnnotationPresent(NativelySupportsDistinct.class)) { + if (call.getOperator().getKind() != SqlKind.COUNT && call.getOperator() instanceof SqlAggFunction) { + if (!call.getOperator().getClass().isAnnotationPresent(NativelySupportsDistinct.class)) { throw buildCalciteContextException( StringUtils.format( "Aggregation [%s] with DISTINCT is not supported when useApproximateCountDistinct is enabled. Run with disabling it.", From 91937302b6081facf05bf83f43d4d6f491181931 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Mon, 22 Jul 2024 20:10:53 +0530 Subject: [PATCH 7/9] fix drill tests --- .../org/apache/druid/sql/calcite/DrillWindowQueryTest.java | 6 +++--- .../java/org/apache/druid/sql/calcite/NotYetSupported.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java index 4e958383945d..1529b0755f41 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java @@ -4426,7 +4426,7 @@ public void test_last_val_lastValFn_39() windowQueryTest(); } - @NotYetSupported(Modes.NOT_ENOUGH_RULES) + @NotYetSupported(Modes.DISTINCT_AGGREGATE_NOT_SUPPORTED) @DrillTest("nestedAggs/emtyOvrCls_7") @Test public void test_nestedAggs_emtyOvrCls_7() @@ -7274,9 +7274,9 @@ public void test_nestedAggs_emtyOvrCls_13() windowQueryTest(); } - @NotYetSupported(Modes.RESULT_MISMATCH) + @NotYetSupported(Modes.DISTINCT_AGGREGATE_NOT_SUPPORTED) @DrillTest("nestedAggs/emtyOvrCls_8") - @Test + @Test public void test_nestedAggs_emtyOvrCls_8() { windowQueryTest(); diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/NotYetSupported.java b/sql/src/test/java/org/apache/druid/sql/calcite/NotYetSupported.java index 5d53593b7ce0..11073befebf9 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/NotYetSupported.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/NotYetSupported.java @@ -77,7 +77,7 @@ enum Modes { // @formatter:off - NOT_ENOUGH_RULES(DruidException.class, "not enough rules"), + DISTINCT_AGGREGATE_NOT_SUPPORTED(DruidException.class, "DISTINCT is not supported"), ERROR_HANDLING(AssertionError.class, "targetPersona: is <[A-Z]+> and category: is <[A-Z_]+> and errorCode: is"), EXPRESSION_NOT_GROUPED(DruidException.class, "Expression '[a-z]+' is not being grouped"), NULLS_FIRST_LAST(DruidException.class, "NULLS (FIRST|LAST)"), From 1b677c7d839f8859d2cd5de418ae4d45bb629216 Mon Sep 17 00:00:00 2001 From: sreemanamala Date: Tue, 23 Jul 2024 10:49:38 +0530 Subject: [PATCH 8/9] comments --- .../org/apache/druid/sql/calcite/rule/GroupByRules.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java b/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java index dbe774be6c71..f0632006d106 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java @@ -71,12 +71,11 @@ public static Aggregation translateAggregateCall( return null; } - if (plannerContext.getPlannerConfig().isUseApproximateCountDistinct() && call.isDistinct()) { - if (call.getAggregation().getKind() != SqlKind.COUNT - && !call.getAggregation().getClass().isAnnotationPresent(NativelySupportsDistinct.class)) { + if (call.isDistinct() && call.getAggregation().getKind() != SqlKind.COUNT) { + if (!call.getAggregation().getClass().isAnnotationPresent(NativelySupportsDistinct.class)) { plannerContext.setPlanningError( "Aggregation [%s] with DISTINCT is not supported when useApproximateCountDistinct is enabled. Run with disabling it.", - call.getAggregation().getKind() + call.getAggregation().getName() ); return null; } From 94b4cab316d501c6dc5ce74b988b9fa4b00a6b51 Mon Sep 17 00:00:00 2001 From: Benedict Jin Date: Tue, 23 Jul 2024 15:06:12 +0800 Subject: [PATCH 9/9] Update sql/src/test/java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java --- .../java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java index 1529b0755f41..c808401543c6 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java @@ -7276,7 +7276,7 @@ public void test_nestedAggs_emtyOvrCls_13() @NotYetSupported(Modes.DISTINCT_AGGREGATE_NOT_SUPPORTED) @DrillTest("nestedAggs/emtyOvrCls_8") - @Test + @Test public void test_nestedAggs_emtyOvrCls_8() { windowQueryTest();