[GitHub] [druid] clintropolis commented on a change in pull request #11959: SQL INSERT planner support.

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

clintropolis commented on a change in pull request #11959:
URL: https://github.com/apache/druid/pull/11959#discussion_r755849785



##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -83,115 +95,120 @@
   private final FrameworkConfig frameworkConfig;
   private final Planner planner;
   private final PlannerContext plannerContext;
-  private final ObjectMapper jsonMapper;
+  private final QueryMakerFactory queryMakerFactory;
+
   private RexBuilder rexBuilder;
 
-  public DruidPlanner(
+  DruidPlanner(
       final FrameworkConfig frameworkConfig,
       final PlannerContext plannerContext,
-      final ObjectMapper jsonMapper
+      final QueryMakerFactory queryMakerFactory
   )
   {
     this.frameworkConfig = frameworkConfig;
     this.planner = Frameworks.getPlanner(frameworkConfig);
     this.plannerContext = plannerContext;
-    this.jsonMapper = jsonMapper;
+    this.queryMakerFactory = queryMakerFactory;
   }
 
   /**
-   * Validates an SQL query and collects a {@link ValidationResult} which contains a set of
-   * {@link org.apache.druid.server.security.Resource} corresponding to any Druid datasources or views which are taking
-   * part in the query
+   * Validates a SQL query and populates {@link PlannerContext#getResourceActions()}.

Review comment:
       As an “es-cue-el” person, this will never read right to me :trollface: 
   
   On a serious note, the refactors in `DruidPlanner` are nice :+1:

##########
File path: sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java
##########
@@ -1097,9 +1101,24 @@ public void testParameterBinding() throws Exception
   }
 
   @Test
-  public void testSysTableParameterBinding() throws Exception
+  public void testSysTableParameterBindingRegularUser() throws Exception
   {
-    PreparedStatement statement = client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");
+    PreparedStatement statement =
+        client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");

Review comment:
       is it a bug that this didn't explode before or is it an artifact of changes elsewhere or just a flaw in the original test?

##########
File path: sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java
##########
@@ -1097,9 +1101,24 @@ public void testParameterBinding() throws Exception
   }
 
   @Test
-  public void testSysTableParameterBinding() throws Exception
+  public void testSysTableParameterBindingRegularUser() throws Exception
   {
-    PreparedStatement statement = client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");
+    PreparedStatement statement =
+        client.prepareStatement("SELECT COUNT(*) AS cnt FROM sys.servers WHERE servers.host = ?");

Review comment:
       ah nevermind, found the change in `CalciteTests` that limits to datasource and view permissions

##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidOuterQueryRel.java
##########
@@ -85,21 +84,6 @@ public PartialDruidQuery getPartialDruidQuery()
     return partialQuery;
   }
 
-  @Override
-  public Sequence<Object[]> runQuery()
-  {
-    // runQuery doesn't need to finalize aggregations, because the fact that runQuery is happening suggests this
-    // is the outermost query and it will actually get run as a native query. Druid's native query layer will
-    // finalize aggregations for the outermost query even if we don't explicitly ask it to.
-
-    final DruidQuery query = toDruidQuery(false);
-    if (query != null) {

Review comment:
       should this null check be moved into `NativeQueryMaker` or the `DruidRel` method or can it not be null here?




-- 
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