[GitHub] [druid] paul-rogers commented on a diff in pull request #13992: Add `msqDenySelect` planner config to (dis)allow MSQ SELECT queries.

["paul-rogers (via GitHub)" <gi...@apache.org>]

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


##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteSelectQueryMSQTest.java:
##########
@@ -183,4 +190,53 @@ public void testArrayAggQueryOnComplexDatatypes()
       );
     }
   }
+
+  @Test
+  public void testMsqDenySelectEnabledQuery()
+  {
+    msqCompatible();
+    try {
+      testQuery(
+          PlannerConfig.builder().msqDenySelect(true).build(),
+          "SELECT COUNT(*) FROM druid.foo "
+          + "WHERE dim2 <> 'a' "
+          + "and __time BETWEEN TIMESTAMP '2000-01-01 00:00:00' AND TIMESTAMP '2000-12-31 23:59:59.999'",
+          CalciteTests.REGULAR_USER_AUTH_RESULT,
+          ImmutableList.of(),
+          ImmutableList.of()
+      );
+      Assert.fail("query execution should fail");
+    }
+    catch (SqlPlanningException e) {
+      Assert.assertTrue(
+          e.getMessage().contains("Cannot execute SELECT with SQL engine 'msq-task'")
+      );
+    }
+  }
+
+  @Test
+  public void testMsqDenySelectDisabledQuery()

Review Comment:
   A test that denies access should probably expect an error, I would think.



##########
processing/src/main/java/org/apache/druid/query/QueryContexts.java:
##########
@@ -110,6 +110,7 @@
   public static final boolean DEFAULT_ENABLE_DEBUG = false;
   public static final int DEFAULT_IN_SUB_QUERY_THRESHOLD = Integer.MAX_VALUE;
   public static final boolean DEFAULT_ENABLE_TIME_BOUNDARY_PLANNING = false;
+  public static final boolean DEFAULT_MSQ_DENY_SELECT = false;

Review Comment:
   No longer needed.



##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteSelectQueryMSQTest.java:
##########
@@ -183,4 +190,53 @@ public void testArrayAggQueryOnComplexDatatypes()
       );
     }
   }
+
+  @Test
+  public void testMsqDenySelectEnabledQuery()
+  {
+    msqCompatible();

Review Comment:
   These tests probably want to be in `CalciteInsertDmlTest`, if they can share config. Those tests are run only for MSQ. The test in this form runs for both MSQ and non-MSQ, which may not be what you wanted.



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