From 77606749edc0d1f9052df40221857d184f30b6bb Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Wed, 4 Oct 2023 15:26:51 +0530 Subject: [PATCH 01/12] Add tests for week, month and year granularities Fix code style --- .../EarliestLatestAnySqlAggregator.java | 2 +- .../convertlet/DruidConvertletTable.java | 1 + .../EarliestLatestConvertletFactory.java | 91 +++++++++++++++++++ 3 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/EarliestLatestConvertletFactory.java diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 5f1b3c3228d4..4d340a059712 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -290,7 +290,7 @@ static String getColumnName( return columnName; } - static class EarliestLatestReturnTypeInference implements SqlReturnTypeInference + public static class EarliestLatestReturnTypeInference implements SqlReturnTypeInference { private final int ordinal; diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/DruidConvertletTable.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/DruidConvertletTable.java index 59ad4ef24f05..b335233e4ee4 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/DruidConvertletTable.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/DruidConvertletTable.java @@ -45,6 +45,7 @@ public class DruidConvertletTable implements SqlRexConvertletTable ImmutableList.builder() .add(CurrentTimestampAndFriendsConvertletFactory.INSTANCE) .add(TimeInIntervalConvertletFactory.INSTANCE) + .add(EarliestLatestConvertletFactory.INSTANCE) .add(NestedDataOperatorConversions.DRUID_JSON_VALUE_CONVERTLET_FACTORY_INSTANCE) .build(); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/EarliestLatestConvertletFactory.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/EarliestLatestConvertletFactory.java new file mode 100644 index 000000000000..89243033e216 --- /dev/null +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/EarliestLatestConvertletFactory.java @@ -0,0 +1,91 @@ +/* + * 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.planner.convertlet; + +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.*; +import org.apache.calcite.sql.parser.SqlParserPos; +import org.apache.calcite.sql.type.OperandTypes; +import org.apache.calcite.sql2rel.SqlRexContext; +import org.apache.calcite.sql2rel.SqlRexConvertlet; +import org.apache.druid.sql.calcite.aggregation.builtin.EarliestLatestAnySqlAggregator; +import org.apache.druid.sql.calcite.aggregation.builtin.EarliestLatestBySqlAggregator; +import org.apache.druid.sql.calcite.expression.OperatorConversions; +import org.apache.druid.sql.calcite.planner.PlannerContext; + +import java.util.Collections; +import java.util.List; + +public class EarliestLatestConvertletFactory implements DruidConvertletFactory +{ + public static final EarliestLatestConvertletFactory INSTANCE = new EarliestLatestConvertletFactory(); + + private static final String NAME = "LATER"; + + private static final SqlOperator OPERATOR = OperatorConversions + .operatorBuilder(NAME) + .operandTypeChecker( + OperandTypes.or( + OperandTypes.ANY, + OperandTypes.sequence( + "'" + NAME + "(expr, maxBytesPerString)'", + OperandTypes.ANY, + OperandTypes.and(OperandTypes.NUMERIC, OperandTypes.LITERAL) + ) + ) + ) + .returnTypeInference(new EarliestLatestAnySqlAggregator.EarliestLatestReturnTypeInference(0)) + .functionCategory(SqlFunctionCategory.USER_DEFINED_FUNCTION) + .build(); + + private EarliestLatestConvertletFactory() + { + // Singleton. + } + + @Override + public SqlRexConvertlet createConvertlet(PlannerContext plannerContext) + { + return new EarliestLatestConvertlet(); + } + + @Override + public List operators() + { + return Collections.singletonList(OPERATOR); + } + + private static class EarliestLatestConvertlet implements SqlRexConvertlet + { + @Override + public RexNode convertCall(final SqlRexContext cx, final SqlCall call) + { + final RexBuilder rexBuilder = cx.getRexBuilder(); + final RexNode columnOperand = cx.convertExpression(call.getOperandList().get(0)); + final RexNode timeColumnOperand = cx.convertExpression(new SqlIdentifier("__time", SqlParserPos.ZERO)); + + return rexBuilder.makeCall( + EarliestLatestBySqlAggregator.LATEST_BY.calciteFunction(), + columnOperand, timeColumnOperand + ); + } + } +} \ No newline at end of file From 9100e22b7f9008bc0fe05f522396ea74170e4a05 Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Thu, 5 Oct 2023 11:09:04 +0530 Subject: [PATCH 02/12] Revert "Add tests for week, month and year granularities" This reverts commit 77606749edc0d1f9052df40221857d184f30b6bb. --- .../EarliestLatestAnySqlAggregator.java | 2 +- .../convertlet/DruidConvertletTable.java | 1 - .../EarliestLatestConvertletFactory.java | 91 ------------------- 3 files changed, 1 insertion(+), 93 deletions(-) delete mode 100644 sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/EarliestLatestConvertletFactory.java diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 4d340a059712..5f1b3c3228d4 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -290,7 +290,7 @@ static String getColumnName( return columnName; } - public static class EarliestLatestReturnTypeInference implements SqlReturnTypeInference + static class EarliestLatestReturnTypeInference implements SqlReturnTypeInference { private final int ordinal; diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/DruidConvertletTable.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/DruidConvertletTable.java index b335233e4ee4..59ad4ef24f05 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/DruidConvertletTable.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/DruidConvertletTable.java @@ -45,7 +45,6 @@ public class DruidConvertletTable implements SqlRexConvertletTable ImmutableList.builder() .add(CurrentTimestampAndFriendsConvertletFactory.INSTANCE) .add(TimeInIntervalConvertletFactory.INSTANCE) - .add(EarliestLatestConvertletFactory.INSTANCE) .add(NestedDataOperatorConversions.DRUID_JSON_VALUE_CONVERTLET_FACTORY_INSTANCE) .build(); diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/EarliestLatestConvertletFactory.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/EarliestLatestConvertletFactory.java deleted file mode 100644 index 89243033e216..000000000000 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/convertlet/EarliestLatestConvertletFactory.java +++ /dev/null @@ -1,91 +0,0 @@ -/* - * 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.planner.convertlet; - -import org.apache.calcite.rex.RexBuilder; -import org.apache.calcite.rex.RexNode; -import org.apache.calcite.sql.*; -import org.apache.calcite.sql.parser.SqlParserPos; -import org.apache.calcite.sql.type.OperandTypes; -import org.apache.calcite.sql2rel.SqlRexContext; -import org.apache.calcite.sql2rel.SqlRexConvertlet; -import org.apache.druid.sql.calcite.aggregation.builtin.EarliestLatestAnySqlAggregator; -import org.apache.druid.sql.calcite.aggregation.builtin.EarliestLatestBySqlAggregator; -import org.apache.druid.sql.calcite.expression.OperatorConversions; -import org.apache.druid.sql.calcite.planner.PlannerContext; - -import java.util.Collections; -import java.util.List; - -public class EarliestLatestConvertletFactory implements DruidConvertletFactory -{ - public static final EarliestLatestConvertletFactory INSTANCE = new EarliestLatestConvertletFactory(); - - private static final String NAME = "LATER"; - - private static final SqlOperator OPERATOR = OperatorConversions - .operatorBuilder(NAME) - .operandTypeChecker( - OperandTypes.or( - OperandTypes.ANY, - OperandTypes.sequence( - "'" + NAME + "(expr, maxBytesPerString)'", - OperandTypes.ANY, - OperandTypes.and(OperandTypes.NUMERIC, OperandTypes.LITERAL) - ) - ) - ) - .returnTypeInference(new EarliestLatestAnySqlAggregator.EarliestLatestReturnTypeInference(0)) - .functionCategory(SqlFunctionCategory.USER_DEFINED_FUNCTION) - .build(); - - private EarliestLatestConvertletFactory() - { - // Singleton. - } - - @Override - public SqlRexConvertlet createConvertlet(PlannerContext plannerContext) - { - return new EarliestLatestConvertlet(); - } - - @Override - public List operators() - { - return Collections.singletonList(OPERATOR); - } - - private static class EarliestLatestConvertlet implements SqlRexConvertlet - { - @Override - public RexNode convertCall(final SqlRexContext cx, final SqlCall call) - { - final RexBuilder rexBuilder = cx.getRexBuilder(); - final RexNode columnOperand = cx.convertExpression(call.getOperandList().get(0)); - final RexNode timeColumnOperand = cx.convertExpression(new SqlIdentifier("__time", SqlParserPos.ZERO)); - - return rexBuilder.makeCall( - EarliestLatestBySqlAggregator.LATEST_BY.calciteFunction(), - columnOperand, timeColumnOperand - ); - } - } -} \ No newline at end of file From 521d1ea499f0550e699a9c4b2df6e421855d020a Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Thu, 5 Oct 2023 11:11:22 +0530 Subject: [PATCH 03/12] Rewrite EARLIEST and LATEST functions to _%-BY counterparts. --- .../EarliestLatestAnySqlAggregator.java | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 5f1b3c3228d4..1af65b89cf9b 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -26,14 +26,19 @@ import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; import org.apache.calcite.sql.SqlAggFunction; +import org.apache.calcite.sql.SqlCall; import org.apache.calcite.sql.SqlFunctionCategory; +import org.apache.calcite.sql.SqlIdentifier; import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.SqlNode; import org.apache.calcite.sql.SqlOperatorBinding; +import org.apache.calcite.sql.parser.SqlParserPos; import org.apache.calcite.sql.type.InferTypes; import org.apache.calcite.sql.type.OperandTypes; import org.apache.calcite.sql.type.SqlReturnTypeInference; import org.apache.calcite.sql.type.SqlTypeName; import org.apache.calcite.sql.type.SqlTypeUtil; +import org.apache.calcite.sql.validate.SqlValidator; import org.apache.calcite.util.Optionality; import org.apache.druid.error.DruidException; import org.apache.druid.error.InvalidSqlInput; @@ -340,5 +345,44 @@ private static class EarliestLatestSqlAggFunction extends SqlAggFunction Optionality.FORBIDDEN ); } + + @Override + public SqlNode rewriteCall( + SqlValidator validator, + SqlCall call + ) + { + List operands = call.getOperandList(); + + SqlParserPos pos = call.getParserPosition(); + + SqlAggFunction aggFunction; + + switch (getName()){ + case "EARLIEST": + aggFunction = EarliestLatestBySqlAggregator.EARLIEST_BY.calciteFunction(); + break; + case "LATEST": + aggFunction = EarliestLatestBySqlAggregator.LATEST_BY.calciteFunction(); + break; + default: + return call; + } + + switch (operands.size()) { + case 1: + return aggFunction.createCall(pos, operands.get(0), new SqlIdentifier( + "__time", pos)); + case 2: + return aggFunction.createCall(pos, operands.get(0), new SqlIdentifier( + "__time", pos), operands.get(1)); + default: + throw InvalidSqlInput.exception( + "Function [%s] expects 1 or 2 arguments but found [%s]", + getName(), + operands.size() + ); + } + } } } From 2af7edef09682ab35fe11b9fd81a48ac2f746e31 Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Thu, 5 Oct 2023 11:48:44 +0530 Subject: [PATCH 04/12] Add comment --- .../aggregation/builtin/EarliestLatestAnySqlAggregator.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 1af65b89cf9b..3cee010dbda4 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -352,6 +352,9 @@ public SqlNode rewriteCall( SqlCall call ) { + // Rewrite EARLIEST to EARLIEST_BY and LATEST to LATEST_BY to make + // reference to __time column explicit so that Calcite tracks it + List operands = call.getOperandList(); SqlParserPos pos = call.getParserPosition(); From 178bee6bb13ccdd284d605273204a4a3db31eefd Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Thu, 5 Oct 2023 15:08:46 +0530 Subject: [PATCH 05/12] Add test --- .../EarliestLatestAnySqlAggregator.java | 2 +- .../druid/sql/calcite/CalciteQueryTest.java | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 3cee010dbda4..10f989d87dc8 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -361,7 +361,7 @@ public SqlNode rewriteCall( SqlAggFunction aggFunction; - switch (getName()){ + switch (getName()) { case "EARLIEST": aggFunction = EarliestLatestBySqlAggregator.EARLIEST_BY.calciteFunction(); break; 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 35f85c5fbf2b..8d7d438d7ee8 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 @@ -675,6 +675,35 @@ public void testEarliestAggregators() ); } + @Test + public void testLatestToLatestByConversion() + { + notMsqCompatible(); + testQuery( + "SELECT LATEST(dim1,10) FROM (SELECT DISTINCT __time, dim1 from foo)", + ImmutableList.of( + new GroupByQuery.Builder() + .setDataSource( + GroupByQuery.builder() + .setDataSource(CalciteTests.DATASOURCE1) + .setInterval(querySegmentSpec(Filtration.eternity())) + .setGranularity(Granularities.ALL) + .setDimensions(dimensions( + new DefaultDimensionSpec("__time", "d0", ColumnType.LONG), + new DefaultDimensionSpec("dim1", "d1", ColumnType.STRING) + )) + .setContext(QUERY_CONTEXT_DEFAULT) + .build()) + .setInterval(querySegmentSpec(Filtration.eternity())) + .setGranularity(Granularities.ALL) + .setAggregatorSpecs( + new StringLastAggregatorFactory("a0", "d1", "d0", 10)) + .setContext(QUERY_CONTEXT_DEFAULT) + .build()), + ImmutableList.of(new Object[]{"abc"}) + ); + } + @Test public void testLatestVectorAggregators() { From 8d36064ed110fae483147184ba84e9def746cdb4 Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Thu, 5 Oct 2023 15:41:43 +0530 Subject: [PATCH 06/12] Update recent function change in Master notMsqCompatible -> msqIncompatible() --- .../java/org/apache/druid/sql/calcite/CalciteQueryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 7408df5ea222..ac839d82db86 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 @@ -678,7 +678,7 @@ public void testEarliestAggregators() @Test public void testLatestToLatestByConversion() { - notMsqCompatible(); + msqIncompatible(); testQuery( "SELECT LATEST(dim1,10) FROM (SELECT DISTINCT __time, dim1 from foo)", ImmutableList.of( From 14a0d6441d82af3e3bf06414cd127b06e9866f0d Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Fri, 6 Oct 2023 15:52:58 +0530 Subject: [PATCH 07/12] Address review comments and fix tests --- .../EarliestLatestAnySqlAggregator.java | 64 ++++++++++--------- .../sql/calcite/CalciteJoinQueryTest.java | 12 ++-- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 10f989d87dc8..8628644913ea 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -68,15 +68,22 @@ import org.apache.druid.sql.calcite.rel.VirtualColumnRegistry; import javax.annotation.Nullable; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.stream.Collectors; public class EarliestLatestAnySqlAggregator implements SqlAggregator { - public static final SqlAggregator EARLIEST = new EarliestLatestAnySqlAggregator(AggregatorType.EARLIEST); - public static final SqlAggregator LATEST = new EarliestLatestAnySqlAggregator(AggregatorType.LATEST); - public static final SqlAggregator ANY_VALUE = new EarliestLatestAnySqlAggregator(AggregatorType.ANY_VALUE); + public static final SqlAggregator EARLIEST = new EarliestLatestAnySqlAggregator( + AggregatorType.EARLIEST, + EarliestLatestBySqlAggregator.EARLIEST_BY.calciteFunction() + ); + public static final SqlAggregator LATEST = new EarliestLatestAnySqlAggregator( + AggregatorType.LATEST, + EarliestLatestBySqlAggregator.LATEST_BY.calciteFunction() + ); + public static final SqlAggregator ANY_VALUE = new EarliestLatestAnySqlAggregator(AggregatorType.ANY_VALUE, null); enum AggregatorType { @@ -169,10 +176,10 @@ abstract AggregatorFactory createAggregatorFactory( private final AggregatorType aggregatorType; private final SqlAggFunction function; - private EarliestLatestAnySqlAggregator(final AggregatorType aggregatorType) + private EarliestLatestAnySqlAggregator(final AggregatorType aggregatorType, final SqlAggFunction replacementAggFunc) { this.aggregatorType = aggregatorType; - this.function = new EarliestLatestSqlAggFunction(aggregatorType); + this.function = new EarliestLatestSqlAggFunction(aggregatorType, replacementAggFunc); } @Override @@ -322,8 +329,9 @@ private static class EarliestLatestSqlAggFunction extends SqlAggFunction { private static final EarliestLatestReturnTypeInference EARLIEST_LATEST_ARG0_RETURN_TYPE_INFERENCE = new EarliestLatestReturnTypeInference(0); + private final SqlAggFunction replacementAggFunc; - EarliestLatestSqlAggFunction(AggregatorType aggregatorType) + EarliestLatestSqlAggFunction(AggregatorType aggregatorType, SqlAggFunction replacementAggFunc) { super( aggregatorType.name(), @@ -344,6 +352,7 @@ private static class EarliestLatestSqlAggFunction extends SqlAggFunction false, Optionality.FORBIDDEN ); + this.replacementAggFunc = replacementAggFunc; } @Override @@ -352,40 +361,33 @@ public SqlNode rewriteCall( SqlCall call ) { - // Rewrite EARLIEST to EARLIEST_BY and LATEST to LATEST_BY to make + // Rewrite EARLIEST/LATEST to EARLIEST_BY/LATEST_BY to make // reference to __time column explicit so that Calcite tracks it + if (replacementAggFunc == null) { + return call; + } + List operands = call.getOperandList(); SqlParserPos pos = call.getParserPosition(); - SqlAggFunction aggFunction; - - switch (getName()) { - case "EARLIEST": - aggFunction = EarliestLatestBySqlAggregator.EARLIEST_BY.calciteFunction(); - break; - case "LATEST": - aggFunction = EarliestLatestBySqlAggregator.LATEST_BY.calciteFunction(); - break; - default: - return call; + if (operands.isEmpty() || operands.size() > 2) { + throw InvalidSqlInput.exception("Function [%s] expects 1 or 2 arguments but found [%s]", + getName(), + operands.size() + ); } - switch (operands.size()) { - case 1: - return aggFunction.createCall(pos, operands.get(0), new SqlIdentifier( - "__time", pos)); - case 2: - return aggFunction.createCall(pos, operands.get(0), new SqlIdentifier( - "__time", pos), operands.get(1)); - default: - throw InvalidSqlInput.exception( - "Function [%s] expects 1 or 2 arguments but found [%s]", - getName(), - operands.size() - ); + List newOperands = new ArrayList<>(); + newOperands.add(operands.get(0)); + newOperands.add(new SqlIdentifier("__time", pos)); + + if (operands.size() == 2) { + newOperands.add(operands.get(1)); } + + return replacementAggFunc.createCall(pos, newOperands); } } } diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java index d1300ff19b24..e8bf088a06ce 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java @@ -1502,13 +1502,11 @@ public void testTimeColumnAggregationsOnLookups(Map queryContext catch (DruidException e) { MatcherAssert.assertThat( e, - new DruidExceptionMatcher(DruidException.Persona.ADMIN, DruidException.Category.INVALID_INPUT, "general") - .expectMessageIs( - "Query could not be planned. A possible reason is " - + "[LATEST and EARLIEST aggregators implicitly depend on the __time column, " - + "but the table queried doesn't contain a __time column. " - + "Please use LATEST_BY or EARLIEST_BY and specify the column explicitly.]" - ) + new DruidExceptionMatcher( + DruidException.Persona.USER, + DruidException.Category.INVALID_INPUT, + "invalidInput" + ).expectMessageContains("Column '__time' not found in any table") ); } } From 6e23061d48e98ec6fe53fb00a2dfd7c9dddf08c9 Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Mon, 9 Oct 2023 10:56:20 +0530 Subject: [PATCH 08/12] Retain exceptions corresponding to EARLIEST/LATEST operators --- .../EarliestLatestAnySqlAggregator.java | 25 ++++++++++++++++--- .../sql/calcite/CalciteJoinQueryTest.java | 12 +++++---- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 8628644913ea..a518646b9115 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -25,6 +25,7 @@ import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; +import org.apache.calcite.runtime.CalciteContextException; import org.apache.calcite.sql.SqlAggFunction; import org.apache.calcite.sql.SqlCall; import org.apache.calcite.sql.SqlFunctionCategory; @@ -364,6 +365,21 @@ public SqlNode rewriteCall( // Rewrite EARLIEST/LATEST to EARLIEST_BY/LATEST_BY to make // reference to __time column explicit so that Calcite tracks it + try { + validator.validate(new SqlIdentifier("__time", SqlParserPos.ZERO)); + } + catch (CalciteContextException e) { + + throw DruidException.forPersona(DruidException.Persona.ADMIN) + .ofCategory(DruidException.Category.INVALID_INPUT) + .build( + e, + "Query could not be planned. A possible reason is [%s]", + "LATEST and EARLIEST aggregators implicitly depend on the __time column, but the " + + "table queried doesn't contain a __time column. Please use LATEST_BY or EARLIEST_BY " + + "and specify the column explicitly." + ); + } if (replacementAggFunc == null) { return call; } @@ -373,15 +389,16 @@ public SqlNode rewriteCall( SqlParserPos pos = call.getParserPosition(); if (operands.isEmpty() || operands.size() > 2) { - throw InvalidSqlInput.exception("Function [%s] expects 1 or 2 arguments but found [%s]", - getName(), - operands.size() + throw InvalidSqlInput.exception( + "Function [%s] expects 1 or 2 arguments but found [%s]", + getName(), + operands.size() ); } List newOperands = new ArrayList<>(); newOperands.add(operands.get(0)); - newOperands.add(new SqlIdentifier("__time", pos)); + newOperands.add(new SqlIdentifier("__time", SqlParserPos.ZERO)); if (operands.size() == 2) { newOperands.add(operands.get(1)); diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java index e8bf088a06ce..d1300ff19b24 100644 --- a/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java +++ b/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java @@ -1502,11 +1502,13 @@ public void testTimeColumnAggregationsOnLookups(Map queryContext catch (DruidException e) { MatcherAssert.assertThat( e, - new DruidExceptionMatcher( - DruidException.Persona.USER, - DruidException.Category.INVALID_INPUT, - "invalidInput" - ).expectMessageContains("Column '__time' not found in any table") + new DruidExceptionMatcher(DruidException.Persona.ADMIN, DruidException.Category.INVALID_INPUT, "general") + .expectMessageIs( + "Query could not be planned. A possible reason is " + + "[LATEST and EARLIEST aggregators implicitly depend on the __time column, " + + "but the table queried doesn't contain a __time column. " + + "Please use LATEST_BY or EARLIEST_BY and specify the column explicitly.]" + ) ); } } From 354029e0052d180d8cbd7266ed4676744c9cdbb7 Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Tue, 10 Oct 2023 11:31:50 +0530 Subject: [PATCH 09/12] Introduce and use a new conversion boolean in Validator --- .../prepare/BaseDruidSqlValidator.java | 13 +++++++++++++ .../EarliestLatestAnySqlAggregator.java | 19 +++---------------- .../sql/calcite/planner/CalcitePlanner.java | 16 ++++++++++++++++ 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/sql/src/main/java/org/apache/calcite/prepare/BaseDruidSqlValidator.java b/sql/src/main/java/org/apache/calcite/prepare/BaseDruidSqlValidator.java index 0750a8a2baed..9f0d17e74570 100644 --- a/sql/src/main/java/org/apache/calcite/prepare/BaseDruidSqlValidator.java +++ b/sql/src/main/java/org/apache/calcite/prepare/BaseDruidSqlValidator.java @@ -31,6 +31,8 @@ */ public class BaseDruidSqlValidator extends CalciteSqlValidator { + private Boolean earliestLatestByConverted; + public BaseDruidSqlValidator( SqlOperatorTable opTab, CalciteCatalogReader catalogReader, @@ -39,5 +41,16 @@ public BaseDruidSqlValidator( ) { super(opTab, catalogReader, typeFactory, validatorConfig); + earliestLatestByConverted = false; + } + + public void setEarliestLatestByConverted() + { + this.earliestLatestByConverted = true; + } + + public Boolean getEarliestLatestByConverted() + { + return earliestLatestByConverted; } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index a518646b9115..5763dc023096 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -19,13 +19,13 @@ package org.apache.druid.sql.calcite.aggregation.builtin; +import org.apache.calcite.prepare.BaseDruidSqlValidator; import org.apache.calcite.rel.core.AggregateCall; import org.apache.calcite.rel.core.Project; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; -import org.apache.calcite.runtime.CalciteContextException; import org.apache.calcite.sql.SqlAggFunction; import org.apache.calcite.sql.SqlCall; import org.apache.calcite.sql.SqlFunctionCategory; @@ -365,21 +365,6 @@ public SqlNode rewriteCall( // Rewrite EARLIEST/LATEST to EARLIEST_BY/LATEST_BY to make // reference to __time column explicit so that Calcite tracks it - try { - validator.validate(new SqlIdentifier("__time", SqlParserPos.ZERO)); - } - catch (CalciteContextException e) { - - throw DruidException.forPersona(DruidException.Persona.ADMIN) - .ofCategory(DruidException.Category.INVALID_INPUT) - .build( - e, - "Query could not be planned. A possible reason is [%s]", - "LATEST and EARLIEST aggregators implicitly depend on the __time column, but the " - + "table queried doesn't contain a __time column. Please use LATEST_BY or EARLIEST_BY " - + "and specify the column explicitly." - ); - } if (replacementAggFunc == null) { return call; } @@ -404,6 +389,8 @@ public SqlNode rewriteCall( newOperands.add(operands.get(1)); } + ((BaseDruidSqlValidator) validator).setEarliestLatestByConverted(); + return replacementAggFunc.createCall(pos, newOperands); } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java index 1abec772e313..78886bcfb1b1 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java @@ -34,6 +34,7 @@ import org.apache.calcite.plan.RelTraitDef; import org.apache.calcite.plan.RelTraitSet; import org.apache.calcite.plan.volcano.VolcanoPlanner; +import org.apache.calcite.prepare.BaseDruidSqlValidator; import org.apache.calcite.prepare.CalciteCatalogReader; import org.apache.calcite.rel.RelCollationTraitDef; import org.apache.calcite.rel.RelNode; @@ -59,6 +60,7 @@ import org.apache.calcite.tools.RelBuilder; import org.apache.calcite.tools.ValidationException; import org.apache.calcite.util.Pair; +import org.apache.druid.error.DruidException; import javax.annotation.Nullable; import java.io.Reader; @@ -240,6 +242,20 @@ public SqlNode validate(SqlNode sqlNode) throws ValidationException validatedSqlNode = validator.validate(sqlNode); } catch (RuntimeException e) { + if (((BaseDruidSqlValidator) validator).getEarliestLatestByConverted() && e.getCause() + .getMessage() + .contains("__time")) { + throw DruidException.forPersona(DruidException.Persona.ADMIN) + .ofCategory(DruidException.Category.INVALID_INPUT) + .build( + e, + "Query could not be planned. A possible reason is [%s]", + "LATEST and EARLIEST aggregators implicitly depend on the __time column, but the " + + "table queried doesn't contain a __time column. Please use LATEST_BY or EARLIEST_BY " + + "and specify the column explicitly." + ); + + } throw new ValidationException(e); } state = CalcitePlanner.State.STATE_4_VALIDATED; From b98ffcf4c32b438b83b503158ae0fb22b7d97b18 Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Tue, 10 Oct 2023 13:15:16 +0530 Subject: [PATCH 10/12] Move convert field boolean to DruidSqlValidator from BaseDruidSqlValidator --- .../calcite/prepare/BaseDruidSqlValidator.java | 13 ------------- .../builtin/EarliestLatestAnySqlAggregator.java | 5 +++-- .../sql/calcite/planner/CalcitePlanner.java | 14 ++++++++++---- .../sql/calcite/planner/DruidSqlValidator.java | 16 +++++++++++++++- 4 files changed, 28 insertions(+), 20 deletions(-) diff --git a/sql/src/main/java/org/apache/calcite/prepare/BaseDruidSqlValidator.java b/sql/src/main/java/org/apache/calcite/prepare/BaseDruidSqlValidator.java index 9f0d17e74570..0750a8a2baed 100644 --- a/sql/src/main/java/org/apache/calcite/prepare/BaseDruidSqlValidator.java +++ b/sql/src/main/java/org/apache/calcite/prepare/BaseDruidSqlValidator.java @@ -31,8 +31,6 @@ */ public class BaseDruidSqlValidator extends CalciteSqlValidator { - private Boolean earliestLatestByConverted; - public BaseDruidSqlValidator( SqlOperatorTable opTab, CalciteCatalogReader catalogReader, @@ -41,16 +39,5 @@ public BaseDruidSqlValidator( ) { super(opTab, catalogReader, typeFactory, validatorConfig); - earliestLatestByConverted = false; - } - - public void setEarliestLatestByConverted() - { - this.earliestLatestByConverted = true; - } - - public Boolean getEarliestLatestByConverted() - { - return earliestLatestByConverted; } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 5763dc023096..8bda0c116776 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -19,7 +19,6 @@ package org.apache.druid.sql.calcite.aggregation.builtin; -import org.apache.calcite.prepare.BaseDruidSqlValidator; import org.apache.calcite.rel.core.AggregateCall; import org.apache.calcite.rel.core.Project; import org.apache.calcite.rel.type.RelDataType; @@ -65,6 +64,7 @@ import org.apache.druid.sql.calcite.expression.DruidExpression; import org.apache.druid.sql.calcite.expression.Expressions; import org.apache.druid.sql.calcite.planner.Calcites; +import org.apache.druid.sql.calcite.planner.DruidSqlValidator; import org.apache.druid.sql.calcite.planner.PlannerContext; import org.apache.druid.sql.calcite.rel.VirtualColumnRegistry; @@ -330,6 +330,7 @@ private static class EarliestLatestSqlAggFunction extends SqlAggFunction { private static final EarliestLatestReturnTypeInference EARLIEST_LATEST_ARG0_RETURN_TYPE_INFERENCE = new EarliestLatestReturnTypeInference(0); + private final SqlAggFunction replacementAggFunc; EarliestLatestSqlAggFunction(AggregatorType aggregatorType, SqlAggFunction replacementAggFunc) @@ -389,7 +390,7 @@ public SqlNode rewriteCall( newOperands.add(operands.get(1)); } - ((BaseDruidSqlValidator) validator).setEarliestLatestByConverted(); + ((DruidSqlValidator) validator).setEarliestLatestByConverted(); return replacementAggFunc.createCall(pos, newOperands); } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java index 78886bcfb1b1..6d6f45e9ecc2 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java @@ -34,7 +34,6 @@ import org.apache.calcite.plan.RelTraitDef; import org.apache.calcite.plan.RelTraitSet; import org.apache.calcite.plan.volcano.VolcanoPlanner; -import org.apache.calcite.prepare.BaseDruidSqlValidator; import org.apache.calcite.prepare.CalciteCatalogReader; import org.apache.calcite.rel.RelCollationTraitDef; import org.apache.calcite.rel.RelNode; @@ -242,9 +241,16 @@ public SqlNode validate(SqlNode sqlNode) throws ValidationException validatedSqlNode = validator.validate(sqlNode); } catch (RuntimeException e) { - if (((BaseDruidSqlValidator) validator).getEarliestLatestByConverted() && e.getCause() - .getMessage() - .contains("__time")) { + if (((DruidSqlValidator) validator).getEarliestLatestByConverted() && e.getCause() + .getMessage() + .contains("__time")){ + + // Since __time column may have been introduced by query rewrite from EARLIEST/LATEST to EARLIEST_BY/LATEST_BY, + // raise a custom exception informing the user of the implicit dependency. Pre-existence of __time col separately + // in query and being the cause of validation failure is possible, but the validation order between the + // new %_BY operator and existing __time col. is unclear, and may lead to confusing "col __time not found in + // any table (row x, col y)" error pointing to the rewritten operator. + throw DruidException.forPersona(DruidException.Persona.ADMIN) .ofCategory(DruidException.Category.INVALID_INPUT) .build( 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 5a901c72296e..e266247f5152 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,8 +28,11 @@ * Druid extended SQL validator. (At present, it doesn't actually * have any extensions yet, but it will soon.) */ -class DruidSqlValidator extends BaseDruidSqlValidator +public class DruidSqlValidator extends BaseDruidSqlValidator { + + private Boolean earliestLatestByConverted; + protected DruidSqlValidator( SqlOperatorTable opTab, CalciteCatalogReader catalogReader, @@ -38,5 +41,16 @@ protected DruidSqlValidator( ) { super(opTab, catalogReader, typeFactory, validatorConfig); + earliestLatestByConverted = false; + } + + public Boolean getEarliestLatestByConverted() + { + return earliestLatestByConverted; + } + + public void setEarliestLatestByConverted() + { + this.earliestLatestByConverted = true; } } From 295d4a74645598f890b3225b4232419ea18b7ef8 Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Tue, 10 Oct 2023 17:20:35 +0530 Subject: [PATCH 11/12] Reverting previous changes of a new DruidSqlValidator field and instead creating a custom SQLIdentifier class for the Time column --- .../EarliestLatestAnySqlAggregator.java | 41 +++++++++++++++++-- .../sql/calcite/planner/CalcitePlanner.java | 22 ---------- .../calcite/planner/DruidSqlValidator.java | 16 +------- 3 files changed, 38 insertions(+), 41 deletions(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 8bda0c116776..34cff7f1f34b 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -25,6 +25,7 @@ import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; +import org.apache.calcite.runtime.CalciteContextException; import org.apache.calcite.sql.SqlAggFunction; import org.apache.calcite.sql.SqlCall; import org.apache.calcite.sql.SqlFunctionCategory; @@ -39,6 +40,7 @@ import org.apache.calcite.sql.type.SqlTypeName; import org.apache.calcite.sql.type.SqlTypeUtil; import org.apache.calcite.sql.validate.SqlValidator; +import org.apache.calcite.sql.validate.SqlValidatorException; import org.apache.calcite.util.Optionality; import org.apache.druid.error.DruidException; import org.apache.druid.error.InvalidSqlInput; @@ -64,7 +66,6 @@ import org.apache.druid.sql.calcite.expression.DruidExpression; import org.apache.druid.sql.calcite.expression.Expressions; import org.apache.druid.sql.calcite.planner.Calcites; -import org.apache.druid.sql.calcite.planner.DruidSqlValidator; import org.apache.druid.sql.calcite.planner.PlannerContext; import org.apache.druid.sql.calcite.rel.VirtualColumnRegistry; @@ -326,6 +327,40 @@ public RelDataType inferReturnType(SqlOperatorBinding sqlOperatorBinding) } } + private static class TimeColIdentifer extends SqlIdentifier + { + + public TimeColIdentifer() + { + super("__time", SqlParserPos.ZERO); + } + + @Override + public R accept(org.apache.calcite.sql.util.SqlVisitor visitor) + { + + try { + return super.accept(visitor); + } + catch (CalciteContextException e) { + if (e.getCause() instanceof SqlValidatorException) { + throw DruidException.forPersona(DruidException.Persona.ADMIN) + .ofCategory(DruidException.Category.INVALID_INPUT) + .build( + e, + "Query could not be planned. A possible reason is [%s]", + "LATEST and EARLIEST aggregators implicitly depend on the __time column, but the " + + "table queried doesn't contain a __time column. Please use LATEST_BY or EARLIEST_BY " + + "and specify the column explicitly." + ); + + } else { + throw e; + } + } + } + } + private static class EarliestLatestSqlAggFunction extends SqlAggFunction { private static final EarliestLatestReturnTypeInference EARLIEST_LATEST_ARG0_RETURN_TYPE_INFERENCE = @@ -384,14 +419,12 @@ public SqlNode rewriteCall( List newOperands = new ArrayList<>(); newOperands.add(operands.get(0)); - newOperands.add(new SqlIdentifier("__time", SqlParserPos.ZERO)); + newOperands.add(new TimeColIdentifer()); if (operands.size() == 2) { newOperands.add(operands.get(1)); } - ((DruidSqlValidator) validator).setEarliestLatestByConverted(); - return replacementAggFunc.createCall(pos, newOperands); } } diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java index 6d6f45e9ecc2..1abec772e313 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalcitePlanner.java @@ -59,7 +59,6 @@ import org.apache.calcite.tools.RelBuilder; import org.apache.calcite.tools.ValidationException; import org.apache.calcite.util.Pair; -import org.apache.druid.error.DruidException; import javax.annotation.Nullable; import java.io.Reader; @@ -241,27 +240,6 @@ public SqlNode validate(SqlNode sqlNode) throws ValidationException validatedSqlNode = validator.validate(sqlNode); } catch (RuntimeException e) { - if (((DruidSqlValidator) validator).getEarliestLatestByConverted() && e.getCause() - .getMessage() - .contains("__time")){ - - // Since __time column may have been introduced by query rewrite from EARLIEST/LATEST to EARLIEST_BY/LATEST_BY, - // raise a custom exception informing the user of the implicit dependency. Pre-existence of __time col separately - // in query and being the cause of validation failure is possible, but the validation order between the - // new %_BY operator and existing __time col. is unclear, and may lead to confusing "col __time not found in - // any table (row x, col y)" error pointing to the rewritten operator. - - throw DruidException.forPersona(DruidException.Persona.ADMIN) - .ofCategory(DruidException.Category.INVALID_INPUT) - .build( - e, - "Query could not be planned. A possible reason is [%s]", - "LATEST and EARLIEST aggregators implicitly depend on the __time column, but the " - + "table queried doesn't contain a __time column. Please use LATEST_BY or EARLIEST_BY " - + "and specify the column explicitly." - ); - - } throw new ValidationException(e); } state = CalcitePlanner.State.STATE_4_VALIDATED; 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 e266247f5152..5a901c72296e 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,11 +28,8 @@ * Druid extended SQL validator. (At present, it doesn't actually * have any extensions yet, but it will soon.) */ -public class DruidSqlValidator extends BaseDruidSqlValidator +class DruidSqlValidator extends BaseDruidSqlValidator { - - private Boolean earliestLatestByConverted; - protected DruidSqlValidator( SqlOperatorTable opTab, CalciteCatalogReader catalogReader, @@ -41,16 +38,5 @@ protected DruidSqlValidator( ) { super(opTab, catalogReader, typeFactory, validatorConfig); - earliestLatestByConverted = false; - } - - public Boolean getEarliestLatestByConverted() - { - return earliestLatestByConverted; - } - - public void setEarliestLatestByConverted() - { - this.earliestLatestByConverted = true; } } From 0d9cba93773edc0767d432a239ac45b1a0ca5c3f Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Wed, 11 Oct 2023 11:57:53 +0530 Subject: [PATCH 12/12] Fix style errors --- .../aggregation/builtin/EarliestLatestAnySqlAggregator.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java index 34cff7f1f34b..c7dab6ad02f1 100644 --- a/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java +++ b/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java @@ -39,6 +39,7 @@ import org.apache.calcite.sql.type.SqlReturnTypeInference; import org.apache.calcite.sql.type.SqlTypeName; import org.apache.calcite.sql.type.SqlTypeUtil; +import org.apache.calcite.sql.util.SqlVisitor; import org.apache.calcite.sql.validate.SqlValidator; import org.apache.calcite.sql.validate.SqlValidatorException; import org.apache.calcite.util.Optionality; @@ -336,7 +337,7 @@ public TimeColIdentifer() } @Override - public R accept(org.apache.calcite.sql.util.SqlVisitor visitor) + public R accept(SqlVisitor visitor) { try {