From 6a938bfa641af781deaeb4189c49aaf6d35aea22 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Thu, 13 Jun 2024 15:48:55 +0000 Subject: [PATCH 1/9] fix issue by running ReduceExpressionsRule early --- .../calcite/planner/CalciteRulesManager.java | 9 +++++++ .../sql/calcite/CalciteSelectQueryTest.java | 27 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java index 829c44b18c60..3aaf2edad203 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java @@ -40,6 +40,7 @@ import org.apache.calcite.rel.rules.JoinPushThroughJoinRule; import org.apache.calcite.rel.rules.ProjectMergeRule; import org.apache.calcite.rel.rules.PruneEmptyRules; +import org.apache.calcite.rel.rules.ReduceExpressionsRule.FilterReduceExpressionsRule; import org.apache.calcite.runtime.Hook; import org.apache.calcite.sql.SqlExplainFormat; import org.apache.calcite.sql.SqlExplainLevel; @@ -71,6 +72,7 @@ import org.apache.druid.sql.calcite.run.EngineFeature; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Set; @@ -291,6 +293,7 @@ private Program buildPreProgram(final PlannerContext plannerContext, final boole // Program that pre-processes the tree before letting the full-on VolcanoPlanner loose. final List prePrograms = new ArrayList<>(); prePrograms.add(new LoggingProgram("Start", isDebug)); + prePrograms.add(sqlToRelWorkaroundProgram()); prePrograms.add(Programs.subQuery(DefaultRelMetadataProvider.INSTANCE)); prePrograms.add(new LoggingProgram("Finished subquery program", isDebug)); prePrograms.add(DecorrelateAndTrimFieldsProgram.INSTANCE); @@ -306,6 +309,12 @@ private Program buildPreProgram(final PlannerContext plannerContext, final boole return Programs.sequence(prePrograms.toArray(new Program[0])); } + private Program sqlToRelWorkaroundProgram() + { + Set rules = Collections.singleton(CoreRules.FILTER_REDUCE_EXPRESSIONS); + return Programs.hep(rules, true, DefaultRelMetadataProvider.INSTANCE); + } + /** * Program to perform manipulations on the logical tree prior to starting the cost-based planner. Mainly this * helps the cost-based planner finish faster, and helps the decoupled planner generate the same plans as the diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java index 3203ae9915e5..e5ebb0d8c429 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java @@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import org.apache.calcite.rel.RelNode; import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.DateTimes; import org.apache.druid.java.util.common.Intervals; @@ -60,6 +61,8 @@ import java.util.List; import java.util.Map; +import static org.junit.jupiter.api.Assertions.assertEquals; + public class CalciteSelectQueryTest extends BaseCalciteQueryTest { @Test @@ -2152,4 +2155,28 @@ public void testCacheKeyConsistency() ) .run(); } + + @Test + public void testSqlToRelInConversion() + { + assertEquals( + "1.37.0", + RelNode.class.getPackage().getImplementationVersion(), + "Calcite version changed; check if CALCITE-6435 is fixed and remove:\n * method CalciteRulesManager#sqlToRelWorkaroundProgram\n * this assertion" + ); + + testBuilder() + .sql( + "SELECT channel FROM wikipedia\n" + + "WHERE channel in ('#en.wikipedia') and channel = '#en.wikipedia' and\n" + + "isRobot = 'false'\n" + + "LIMIT 1" + ) + .expectedResults( + ImmutableList.of( + new Object[] {"#en.wikipedia"} + ) + ) + .run(); + } } From 71274cb573598c8fc18ff25fddf88d944b1e0d24 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Tue, 18 Jun 2024 09:23:25 +0000 Subject: [PATCH 2/9] add a different fix --- .../sql/BaseVarianceSqlAggregator.java | 26 ++++- .../sql/VarianceSqlAggregatorTest.java | 28 ++++++ .../calcite/planner/CalciteRulesManager.java | 4 +- .../druid/sql/calcite/rel/Windowing.java | 3 + .../rule/FixIncorrectInExpansionTypes.java | 97 +++++++++++++++++++ 5 files changed, 151 insertions(+), 7 deletions(-) create mode 100644 sql/src/main/java/org/apache/druid/sql/calcite/rule/FixIncorrectInExpansionTypes.java diff --git a/extensions-core/stats/src/main/java/org/apache/druid/query/aggregation/variance/sql/BaseVarianceSqlAggregator.java b/extensions-core/stats/src/main/java/org/apache/druid/query/aggregation/variance/sql/BaseVarianceSqlAggregator.java index b2ed565d6276..53c55c3e6705 100644 --- a/extensions-core/stats/src/main/java/org/apache/druid/query/aggregation/variance/sql/BaseVarianceSqlAggregator.java +++ b/extensions-core/stats/src/main/java/org/apache/druid/query/aggregation/variance/sql/BaseVarianceSqlAggregator.java @@ -97,12 +97,29 @@ public Aggregation toDruidAggregation( final RelDataType dataType = inputOperand.getType(); final ColumnType inputType = Calcites.getColumnTypeForRelDataType(dataType); final DimensionSpec dimensionSpec; - final String aggName = StringUtils.format("%s:agg", name); + + final String aggName; + + final SqlAggFunction func = calciteFunction(); final String estimator; final String inputTypeName; PostAggregator postAggregator = null; + String tempAggName; + if (func.getName().equals(STDDEV_NAME) + || func.getName().equals(SqlKind.STDDEV_POP.name()) + || func.getName().equals(SqlKind.STDDEV_SAMP.name())) { + + aggName = StringUtils.format("%s:agg", name); + tempAggName = name; + }else { +// aggName = name; + aggName = StringUtils.format("%s:agg", name); + tempAggName=null; + } + + if (input.isSimpleExtraction()) { dimensionSpec = input.getSimpleExtraction().toDimensionSpec(null, inputType); } else { @@ -136,15 +153,14 @@ public Aggregation toDruidAggregation( inputTypeName ); - if (func.getName().equals(STDDEV_NAME) - || func.getName().equals(SqlKind.STDDEV_POP.name()) - || func.getName().equals(SqlKind.STDDEV_SAMP.name())) { + if(tempAggName!=null) { postAggregator = new StandardDeviationPostAggregator( - name, + tempAggName, aggregatorFactory.getName(), estimator); } + return Aggregation.create( ImmutableList.of(aggregatorFactory), postAggregator 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 7c63d63aab8d..38582ff4c8ab 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 @@ -20,6 +20,7 @@ package org.apache.druid.query.aggregation.variance.sql; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.inject.Injector; import org.apache.druid.common.config.NullHandling; import org.apache.druid.data.input.InputRow; @@ -32,6 +33,7 @@ import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.query.Druids; +import org.apache.druid.query.QueryContexts; import org.apache.druid.query.QueryRunnerFactoryConglomerate; import org.apache.druid.query.aggregation.CountAggregatorFactory; import org.apache.druid.query.aggregation.DoubleSumAggregatorFactory; @@ -61,6 +63,7 @@ import org.apache.druid.sql.calcite.SqlTestFrameworkConfig; 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.util.CalciteTests; import org.apache.druid.sql.calcite.util.SqlTestFramework.StandardComponentSupplier; import org.apache.druid.sql.calcite.util.TestDataBuilder; @@ -698,4 +701,29 @@ public void testVarianceAggAsInput() expectedResults ); } + + + @Test + public void testOverWindow() + { + testBuilder() + .sql( + "select dim4, dim5, mod(m1, 3), var_pop(mod(m1, 3)) over (partition by dim4 order by dim5) c\n" + + "from numfoo\n" + + "group by dim4, dim5, mod(m1, 3)") + .queryContext(ImmutableMap.of( + PlannerContext.CTX_ENABLE_WINDOW_FNS, true, + QueryContexts.ENABLE_DEBUG, true, + QueryContexts.WINDOWING_STRICT_VALIDATION, false + )) + .expectedResults(ImmutableList.of( + new Object[]{"a", "aa", 1.0D, 0.0D}, + new Object[]{"a", "ab", 2.0D, 0.25D}, + new Object[]{"a", "ba", 0.0D, 0.6666666666666666D}, + new Object[]{"b", "aa", 2.0D, 0.0D}, + new Object[]{"b", "ab", 0.0D, 1.0D}, + new Object[]{"b", "ad", 1.0D, 0.6666666666666666D} + )) + .run(); + } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java index 3aaf2edad203..dc323bc796fb 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java @@ -40,7 +40,6 @@ import org.apache.calcite.rel.rules.JoinPushThroughJoinRule; import org.apache.calcite.rel.rules.ProjectMergeRule; import org.apache.calcite.rel.rules.PruneEmptyRules; -import org.apache.calcite.rel.rules.ReduceExpressionsRule.FilterReduceExpressionsRule; import org.apache.calcite.runtime.Hook; import org.apache.calcite.sql.SqlExplainFormat; import org.apache.calcite.sql.SqlExplainLevel; @@ -66,6 +65,7 @@ import org.apache.druid.sql.calcite.rule.ProjectAggregatePruneUnusedCallRule; import org.apache.druid.sql.calcite.rule.ReverseLookupRule; import org.apache.druid.sql.calcite.rule.RewriteFirstValueLastValueRule; +import org.apache.druid.sql.calcite.rule.FixIncorrectInExpansionTypes; import org.apache.druid.sql.calcite.rule.SortCollapseRule; import org.apache.druid.sql.calcite.rule.logical.DruidAggregateRemoveRedundancyRule; import org.apache.druid.sql.calcite.rule.logical.DruidLogicalRules; @@ -311,7 +311,7 @@ private Program buildPreProgram(final PlannerContext plannerContext, final boole private Program sqlToRelWorkaroundProgram() { - Set rules = Collections.singleton(CoreRules.FILTER_REDUCE_EXPRESSIONS); + Set rules = Collections.singleton(new FixIncorrectInExpansionTypes()); return Programs.hep(rules, true, DefaultRelMetadataProvider.INSTANCE); } 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 4f0f0eda21b9..172904abcc6a 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 @@ -163,6 +163,9 @@ public static Windowing fromCalciteStuff( false // Windowed aggregations don't currently finalize. This means that sketches won't work as expected. ); + if(!aggName.equals( aggregation.getOutputName())) { + throw new CannotBuildQueryException(window, aggregateCall); + } if (aggregation == null || aggregation.getPostAggregator() != null || aggregation.getAggregatorFactories().size() != 1) { diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rule/FixIncorrectInExpansionTypes.java b/sql/src/main/java/org/apache/druid/sql/calcite/rule/FixIncorrectInExpansionTypes.java new file mode 100644 index 000000000000..000e3567acd5 --- /dev/null +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rule/FixIncorrectInExpansionTypes.java @@ -0,0 +1,97 @@ +/* + * 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.rule; + +import org.apache.calcite.plan.RelOptRule; +import org.apache.calcite.plan.RelOptRuleCall; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.rules.SubstitutionRule; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.rex.RexShuttle; +import org.apache.calcite.rex.RexUtil; +import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.type.SqlTypeName; + +/** + * Rewrites comparisions to avoid bug FIXME. + * + * Rewrites RexCall::VARCHAR = RexLiteral::CHAR to RexCall::VARCHAR = + * RexLiteral::VARCHAR + * + * needed until CALCITE-6435 is fixed & released. + */ +public class FixIncorrectInExpansionTypes extends RelOptRule implements SubstitutionRule +{ + public FixIncorrectInExpansionTypes() + { + super(operand(RelNode.class, any())); + } + + @Override + public void onMatch(RelOptRuleCall call) + { + final RelNode oldNode = call.rel(0); + final RewriteShuttle shuttle = new RewriteShuttle(oldNode.getCluster().getRexBuilder()); + final RelNode newNode = oldNode.accept(shuttle); + + // noinspection ObjectEquality + if (newNode != oldNode) { + call.transformTo(newNode); + call.getPlanner().prune(oldNode); + } + } + + private static class RewriteShuttle extends RexShuttle + { + private final RexBuilder rexBuilder; + + public RewriteShuttle(RexBuilder rexBuilder) + { + this.rexBuilder = rexBuilder; + } + + @Override + public RexNode visitCall(RexCall call) + { + RexNode newNode = super.visitCall(call); + if (newNode.getKind() == SqlKind.EQUALS) { + RexCall newCall = (RexCall) newNode; + RexNode op0 = newCall.getOperands().get(0); + RexNode op1 = newCall.getOperands().get(1); + if (RexUtil.isLiteral(op1, false)) { + + if (op1.getType().getSqlTypeName() == SqlTypeName.CHAR + && op0.getType().getSqlTypeName() == SqlTypeName.VARCHAR) { + + RexNode newLiteral = rexBuilder.ensureType(op0.getType(), op1, true); + return rexBuilder.makeCall( + newCall.getOperator(), + op0, + newLiteral + ); + } + } + } + return newNode; + } + } +} From 76f743e64fa420a3829b0e577912afb7aa427e63 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Tue, 18 Jun 2024 09:23:45 +0000 Subject: [PATCH 3/9] undo --- .../sql/BaseVarianceSqlAggregator.java | 26 ++++------------- .../sql/VarianceSqlAggregatorTest.java | 28 ------------------- 2 files changed, 5 insertions(+), 49 deletions(-) diff --git a/extensions-core/stats/src/main/java/org/apache/druid/query/aggregation/variance/sql/BaseVarianceSqlAggregator.java b/extensions-core/stats/src/main/java/org/apache/druid/query/aggregation/variance/sql/BaseVarianceSqlAggregator.java index 53c55c3e6705..b2ed565d6276 100644 --- a/extensions-core/stats/src/main/java/org/apache/druid/query/aggregation/variance/sql/BaseVarianceSqlAggregator.java +++ b/extensions-core/stats/src/main/java/org/apache/druid/query/aggregation/variance/sql/BaseVarianceSqlAggregator.java @@ -97,29 +97,12 @@ public Aggregation toDruidAggregation( final RelDataType dataType = inputOperand.getType(); final ColumnType inputType = Calcites.getColumnTypeForRelDataType(dataType); final DimensionSpec dimensionSpec; - - final String aggName; - - + final String aggName = StringUtils.format("%s:agg", name); final SqlAggFunction func = calciteFunction(); final String estimator; final String inputTypeName; PostAggregator postAggregator = null; - String tempAggName; - if (func.getName().equals(STDDEV_NAME) - || func.getName().equals(SqlKind.STDDEV_POP.name()) - || func.getName().equals(SqlKind.STDDEV_SAMP.name())) { - - aggName = StringUtils.format("%s:agg", name); - tempAggName = name; - }else { -// aggName = name; - aggName = StringUtils.format("%s:agg", name); - tempAggName=null; - } - - if (input.isSimpleExtraction()) { dimensionSpec = input.getSimpleExtraction().toDimensionSpec(null, inputType); } else { @@ -153,14 +136,15 @@ public Aggregation toDruidAggregation( inputTypeName ); - if(tempAggName!=null) { + if (func.getName().equals(STDDEV_NAME) + || func.getName().equals(SqlKind.STDDEV_POP.name()) + || func.getName().equals(SqlKind.STDDEV_SAMP.name())) { postAggregator = new StandardDeviationPostAggregator( - tempAggName, + name, aggregatorFactory.getName(), estimator); } - return Aggregation.create( ImmutableList.of(aggregatorFactory), postAggregator 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 38582ff4c8ab..7c63d63aab8d 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 @@ -20,7 +20,6 @@ package org.apache.druid.query.aggregation.variance.sql; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.inject.Injector; import org.apache.druid.common.config.NullHandling; import org.apache.druid.data.input.InputRow; @@ -33,7 +32,6 @@ import org.apache.druid.java.util.common.granularity.Granularities; import org.apache.druid.math.expr.ExprMacroTable; import org.apache.druid.query.Druids; -import org.apache.druid.query.QueryContexts; import org.apache.druid.query.QueryRunnerFactoryConglomerate; import org.apache.druid.query.aggregation.CountAggregatorFactory; import org.apache.druid.query.aggregation.DoubleSumAggregatorFactory; @@ -63,7 +61,6 @@ import org.apache.druid.sql.calcite.SqlTestFrameworkConfig; 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.util.CalciteTests; import org.apache.druid.sql.calcite.util.SqlTestFramework.StandardComponentSupplier; import org.apache.druid.sql.calcite.util.TestDataBuilder; @@ -701,29 +698,4 @@ public void testVarianceAggAsInput() expectedResults ); } - - - @Test - public void testOverWindow() - { - testBuilder() - .sql( - "select dim4, dim5, mod(m1, 3), var_pop(mod(m1, 3)) over (partition by dim4 order by dim5) c\n" - + "from numfoo\n" - + "group by dim4, dim5, mod(m1, 3)") - .queryContext(ImmutableMap.of( - PlannerContext.CTX_ENABLE_WINDOW_FNS, true, - QueryContexts.ENABLE_DEBUG, true, - QueryContexts.WINDOWING_STRICT_VALIDATION, false - )) - .expectedResults(ImmutableList.of( - new Object[]{"a", "aa", 1.0D, 0.0D}, - new Object[]{"a", "ab", 2.0D, 0.25D}, - new Object[]{"a", "ba", 0.0D, 0.6666666666666666D}, - new Object[]{"b", "aa", 2.0D, 0.0D}, - new Object[]{"b", "ab", 0.0D, 1.0D}, - new Object[]{"b", "ad", 1.0D, 0.6666666666666666D} - )) - .run(); - } } From a3d3967160ecd0315fc893cc1718b58f4713df49 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Tue, 18 Jun 2024 09:24:34 +0000 Subject: [PATCH 4/9] remove unrelated --- .../apache/druid/sql/calcite/planner/CalciteRulesManager.java | 2 +- .../main/java/org/apache/druid/sql/calcite/rel/Windowing.java | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java index dc323bc796fb..8d3b6a743e38 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java @@ -61,11 +61,11 @@ import org.apache.druid.sql.calcite.rule.FilterDecomposeCoalesceRule; import org.apache.druid.sql.calcite.rule.FilterDecomposeConcatRule; import org.apache.druid.sql.calcite.rule.FilterJoinExcludePushToChildRule; +import org.apache.druid.sql.calcite.rule.FixIncorrectInExpansionTypes; import org.apache.druid.sql.calcite.rule.FlattenConcatRule; import org.apache.druid.sql.calcite.rule.ProjectAggregatePruneUnusedCallRule; import org.apache.druid.sql.calcite.rule.ReverseLookupRule; import org.apache.druid.sql.calcite.rule.RewriteFirstValueLastValueRule; -import org.apache.druid.sql.calcite.rule.FixIncorrectInExpansionTypes; import org.apache.druid.sql.calcite.rule.SortCollapseRule; import org.apache.druid.sql.calcite.rule.logical.DruidAggregateRemoveRedundancyRule; import org.apache.druid.sql.calcite.rule.logical.DruidLogicalRules; 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 172904abcc6a..4f0f0eda21b9 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 @@ -163,9 +163,6 @@ public static Windowing fromCalciteStuff( false // Windowed aggregations don't currently finalize. This means that sketches won't work as expected. ); - if(!aggName.equals( aggregation.getOutputName())) { - throw new CannotBuildQueryException(window, aggregateCall); - } if (aggregation == null || aggregation.getPostAggregator() != null || aggregation.getAggregatorFactories().size() != 1) { From 8a3c2b8ee89ab5fefa048656e2882f9f404e9267 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Tue, 18 Jun 2024 09:26:38 +0000 Subject: [PATCH 5/9] update msg --- .../org/apache/druid/sql/calcite/CalciteSelectQueryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java index e5ebb0d8c429..d6113a154da6 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java @@ -2162,7 +2162,7 @@ public void testSqlToRelInConversion() assertEquals( "1.37.0", RelNode.class.getPackage().getImplementationVersion(), - "Calcite version changed; check if CALCITE-6435 is fixed and remove:\n * method CalciteRulesManager#sqlToRelWorkaroundProgram\n * this assertion" + "Calcite version changed; check if CALCITE-6435 is fixed and remove:\n * method CalciteRulesManager#sqlToRelWorkaroundProgram\n * FixIncorrectInExpansionTypes class\n* this assertion" ); testBuilder() From 8efd057a812c4458bf5a0925399731ac0f7eef72 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Tue, 18 Jun 2024 12:24:38 +0000 Subject: [PATCH 6/9] accept plan changes --- .../quidem/org.apache.druid.quidem.SqlQuidemTest/decoupled.iq | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/decoupled.iq b/sql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/decoupled.iq index be52c7c4c65b..530bbe172bb9 100644 --- a/sql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/decoupled.iq +++ b/sql/src/test/quidem/org.apache.druid.quidem.SqlQuidemTest/decoupled.iq @@ -26,13 +26,13 @@ LogicalSort(sort0=[$0], dir0=[ASC]) LogicalSort(sort0=[$0], dir0=[ASC]) LogicalAggregate(group=[{0}], cnt=[COUNT($1) FILTER $2], aall=[COUNT()]) LogicalProject(cityName=[$2], channel=[$1], $f3=[IS TRUE(>($17, 0))]) - LogicalFilter(condition=[SEARCH($2, Sarg['Aarhus':VARCHAR(8), 'New York':VARCHAR(8)]:VARCHAR(8))]) + LogicalFilter(condition=[SEARCH($2, Sarg['Aarhus':VARCHAR, 'New York':VARCHAR]:VARCHAR)]) LogicalTableScan(table=[[druid, wikipedia]]) !logicalPlan DruidAggregate(group=[{0}], cnt=[COUNT($1) FILTER $2], aall=[COUNT()], druid=[logical]) DruidProject(cityName=[$2], channel=[$1], $f3=[IS TRUE(>($17, 0))], druid=[logical]) - DruidFilter(condition=[SEARCH($2, Sarg['Aarhus':VARCHAR(8), 'New York':VARCHAR(8)]:VARCHAR(8))]) + DruidFilter(condition=[SEARCH($2, Sarg['Aarhus':VARCHAR, 'New York':VARCHAR]:VARCHAR)]) DruidTableScan(table=[[druid, wikipedia]], druid=[logical]) !druidPlan From 730b127c423d934044538234deac849efa84b14c Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Sat, 29 Jun 2024 05:39:19 +0000 Subject: [PATCH 7/9] add not_equals --- .../druid/sql/calcite/rule/FixIncorrectInExpansionTypes.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/rule/FixIncorrectInExpansionTypes.java b/sql/src/main/java/org/apache/druid/sql/calcite/rule/FixIncorrectInExpansionTypes.java index 000e3567acd5..9c049ac89dab 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/rule/FixIncorrectInExpansionTypes.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/rule/FixIncorrectInExpansionTypes.java @@ -73,7 +73,7 @@ public RewriteShuttle(RexBuilder rexBuilder) public RexNode visitCall(RexCall call) { RexNode newNode = super.visitCall(call); - if (newNode.getKind() == SqlKind.EQUALS) { + if (newNode.getKind() == SqlKind.EQUALS || newNode.getKind() == SqlKind.NOT_EQUALS) { RexCall newCall = (RexCall) newNode; RexNode op0 = newCall.getOperands().get(0); RexNode op1 = newCall.getOperands().get(1); From e0a1525c776e59bfb52c7c89902a881decf3f8cb Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Sat, 29 Jun 2024 05:39:30 +0000 Subject: [PATCH 8/9] add test --- .../sql/calcite/CalciteSelectQueryTest.java | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java index d2699e77f79b..0b93fc824d56 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java @@ -2202,4 +2202,27 @@ public void testSqlToRelInConversion() ) .run(); } + @Test + public void testSqlToRelNotInConversion() + { + assertEquals( + "1.37.0", + RelNode.class.getPackage().getImplementationVersion(), + "Calcite version changed; check if CALCITE-6435 is fixed and remove:\n * method CalciteRulesManager#sqlToRelWorkaroundProgram\n * FixIncorrectInExpansionTypes class\n* this assertion" + ); + + testBuilder() + .sql( + "SELECT channel FROM wikipedia\n" + + "WHERE (channel not in ('#en.wikipedia') or channel <> '#en.wikipedia') is false and\n" + + "isRobot = 'false'\n" + + "LIMIT 1" + ) + .expectedResults( + ImmutableList.of( + new Object[] {"#en.wikipedia"} + ) + ) + .run(); + } } From c40736ad7bd34bc709e6fb94881bb04ecbfecf26 Mon Sep 17 00:00:00 2001 From: Zoltan Haindrich Date: Sat, 29 Jun 2024 05:39:33 +0000 Subject: [PATCH 9/9] Revert "add test" This reverts commit e0a1525c776e59bfb52c7c89902a881decf3f8cb. --- .../sql/calcite/CalciteSelectQueryTest.java | 23 ------------------- 1 file changed, 23 deletions(-) diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java index 0b93fc824d56..d2699e77f79b 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteSelectQueryTest.java @@ -2202,27 +2202,4 @@ public void testSqlToRelInConversion() ) .run(); } - @Test - public void testSqlToRelNotInConversion() - { - assertEquals( - "1.37.0", - RelNode.class.getPackage().getImplementationVersion(), - "Calcite version changed; check if CALCITE-6435 is fixed and remove:\n * method CalciteRulesManager#sqlToRelWorkaroundProgram\n * FixIncorrectInExpansionTypes class\n* this assertion" - ); - - testBuilder() - .sql( - "SELECT channel FROM wikipedia\n" - + "WHERE (channel not in ('#en.wikipedia') or channel <> '#en.wikipedia') is false and\n" - + "isRobot = 'false'\n" - + "LIMIT 1" - ) - .expectedResults( - ImmutableList.of( - new Object[] {"#en.wikipedia"} - ) - ) - .run(); - } }