Move convert field boolean to DruidSqlValidator from BaseDruidSqlVali…

…dator
apache · Oct 10, 2023 · b98ffcf · b98ffcf
1 parent 354029e
commit b98ffcf
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 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
@@ -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/...java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java b/...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
@@ -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
@@ -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;
   }
 }