[GitHub] [druid] paul-rogers commented on a diff in pull request #12897: SQL: Morph QueryMakerFactory into SqlEngine.

[GitBox <gi...@apache.org>]

paul-rogers commented on code in PR #12897:
URL: https://github.com/apache/druid/pull/12897#discussion_r944797807


##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -422,8 +451,19 @@ private PlannerResult planWithDruidConvention(
     if (explain != null) {
       return planExplanation(druidRel, explain, true);
     } else {
-      final Supplier<Sequence<Object[]>> resultsSupplier = () -> {
+      // Compute row type.
+      final RelDataTypeFactory typeFactory = rootQueryRel.rel.getCluster().getTypeFactory();
+      final RelDataType rowType;
 
+      if (parsed.isSelect()) {
+        rowType = engine.resultTypeForSelect(typeFactory, rootQueryRel.validatedRowType);
+      } else {
+        assert parsed.insertOrReplace != null;
+        rowType = engine.resultTypeForInsert(typeFactory, rootQueryRel.validatedRowType);
+      }

Review Comment:
   This is exactly the kind of silliness that the handler refactoring will address.



##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -319,31 +345,34 @@ public PlannerResult plan() throws ValidationException
       rootQueryRel = planner.rel(validatedQueryNode);
     }
 
+    final boolean hasBindableTables = hasBindableTables(rootQueryRel.rel);
+
     // the planner's type factory is not available until after parsing
     this.rexBuilder = new RexBuilder(planner.getTypeFactory());
     state = State.PLANNED;
 
     try {
-      return planWithDruidConvention(rootQueryRel, parsed.getExplainNode(), parsed.getInsertOrReplace());
+      if (hasBindableTables) {
+        // Consider BINDABLE convention if necessary. Used for metadata tables.
+        if (!parsed.isSelect()) {

Review Comment:
   All this will change in a PR that will be added after #12845 is in. We'll refactor this code into "handlers" for each statement type.



##########
sql/src/main/java/org/apache/druid/sql/calcite/run/EngineFeature.java:
##########
@@ -20,12 +20,18 @@
 package org.apache.druid.sql.calcite.run;
 
 import org.apache.druid.sql.calcite.external.ExternalDataSource;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
 
 /**
- * Arguments to {@link QueryFeatureInspector#feature(QueryFeature)}.
+ * Arguments to {@link EngineFeatureInspector#feature(EngineFeature, PlannerContext)}.
  */
-public enum QueryFeature
+public enum EngineFeature
 {
+  /**
+   * Can execute INSERT and REPLACE statements.
+   */
+  CAN_INSERT,

Review Comment:
   If we do, perhaps make this more granular for each statement type: CAN_SELECT, CAN_INSERT, CAN_REPLACE, CAN_UPSERT, CAN_DELETE, ...



##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java:
##########
@@ -165,9 +170,26 @@ public void validate() throws SqlParseException, ValidationException
   {
     Preconditions.checkState(state == State.START);
     resetPlanner();
+
+    // Validate SQL query context parameters.
+    for (String contextParameterName : plannerContext.getQueryContext().getMergedParams().keySet()) {
+      if (engine.isSystemContextParameter(contextParameterName)) {
+        throw new ValidationException(
+            StringUtils.format(
+                "Cannot execute query with context parameter [%s]",
+                contextParameterName
+            )
+        );
+      }
+    }

Review Comment:
   Suggestion, move this into the engine as `validateContext()` so that code here does not need to now what rules each engine might enforce.



##########
sql/src/main/java/org/apache/druid/sql/SqlLifecycleFactory.java:
##########
@@ -57,9 +58,10 @@ public SqlLifecycleFactory(
     this.defaultQueryConfig = defaultQueryConfig.get();
   }
 
-  public SqlLifecycle factorize()
+  public SqlLifecycle factorize(final SqlEngine engine)

Review Comment:
   With this change, engine selection is up to the client. In the PR #12845 we'll need to pass this into the three factory method for the three kinds of statements. There is code that chooses the factory. It could be that factory selection affects other attributes.
   
   So, I wonder, should there be a "factory selector" that can sniff the sql info and make a choice? Or, should there be a planner factory per engine?



##########
sql/src/main/java/org/apache/druid/sql/calcite/run/EngineFeature.java:
##########
@@ -36,6 +42,11 @@
    */
   CAN_RUN_TOPN,
 
+  /**
+   * Queries of type {@link org.apache.druid.query.timeboundary.TimeBoundaryQuery} are usable.
+   */
+  CAN_RUN_TIME_BOUNDARY,

Review Comment:
   Nit: Since these items are logically part of a feature set, maybe just use the feature name itself. TOPN_QUERY, SELECT, TIME_BOUNDARY_QUERY, etc.



##########
sql/src/main/java/org/apache/druid/sql/calcite/external/ExternalTableScanRule.java:
##########
@@ -43,17 +43,26 @@ public ExternalTableScanRule(final PlannerContext plannerContext)
   @Override
   public boolean matches(RelOptRuleCall call)
   {
-    if (plannerContext.getQueryMaker().feature(QueryFeature.CAN_READ_EXTERNAL_DATA)) {
+    if (plannerContext.engineHasFeature(EngineFeature.CAN_READ_EXTERNAL_DATA)) {
       return super.matches(call);
     } else {
-      plannerContext.setPlanningError("SQL query requires scanning external datasources that is not suported.");
+      plannerContext.setPlanningError(
+          "Cannot use '%s' with the current SQL engine.",

Review Comment:
   Perhaps at least include the class name and we can improve it later. Else, we'll wonder what the "current engine" is if we get this error.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org