From 295d4a74645598f890b3225b4232419ea18b7ef8 Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Tue, 10 Oct 2023 17:20:35 +0530 Subject: [PATCH] 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; } }