From b98ffcf4c32b438b83b503158ae0fb22b7d97b18 Mon Sep 17 00:00:00 2001 From: Vishesh Garg Date: Tue, 10 Oct 2023 13:15:16 +0530 Subject: [PATCH] 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; } }