[GitHub] [pinot] walterddr commented on a diff in pull request #10292: [feature] Add jdbc multistage support

["walterddr (via GitHub)" <gi...@apache.org>]

walterddr commented on code in PR #10292:
URL: https://github.com/apache/pinot/pull/10292#discussion_r1111057861


##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotPreparedStatement.java:
##########
@@ -51,7 +51,7 @@ public PinotPreparedStatement(PinotConnection connection, String query) {
     if (!DriverUtils.queryContainsLimitStatement(_query)) {
       _query += " " + LIMIT_STATEMENT + " " + _maxRows;
     }
-    _query = DriverUtils.enableNullHandling(_connection, _query);
+    _query = _connection.enableQueryOptions(_query);

Review Comment:
   I am a bit confused on how would the user control whether to use pinot-v1 or pinot-v2.
   enable null handling is configured by option but are we planning to do so as well for v1 vs. v2?
   
   should we create a different connection string format for v1 or v2, for example:
   - for V1: `jdbc:pinot:default_database`
   - for V2: `jdbc:pinot_v2:default_database`



##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -67,15 +74,24 @@ public class PinotConnection extends AbstractBaseConnection {
     }
     _session = new org.apache.pinot.client.Connection(properties, brokers, transport);
 
-    _enableNullHandling = Boolean.parseBoolean(properties.getProperty(QueryOptionKey.ENABLE_NULL_HANDLING));
+    for (String possibleQueryOption: POSSIBLE_QUERY_OPTIONS) {
+      _queryOptions.put(possibleQueryOption, Boolean.parseBoolean(properties.getProperty(possibleQueryOption)));
+    }
   }
 
   public org.apache.pinot.client.Connection getSession() {
     return _session;
   }
 
-  public boolean isNullHandlingEnabled() {
-    return _enableNullHandling;
+  protected String enableQueryOptions(String sql) {

Review Comment:
   +1. also is there any case we want to enable null handling but not v2. or vice-versa?



##########
pinot-clients/pinot-jdbc-client/src/test/java/org/apache/pinot/client/PinotPreparedStatementTest.java:
##########
@@ -120,4 +123,30 @@ public void testSetAdditionalDataTypes()
     Assert.assertEquals(lastExecutedQuery.substring(0, lastExecutedQuery.indexOf("LIMIT")).trim(),
         String.format("SELECT * FROM dummy WHERE value = '%s'", Hex.encodeHexString(value.getBytes())));
   }
+
+  @Test
+  public void testSetEnableNullHandling()
+      throws Exception {
+    Properties props = new Properties();
+    props.put(QueryOptionKey.ENABLE_NULL_HANDLING, "true");
+    PinotConnection pinotConnection =
+        new PinotConnection(props, "dummy", _dummyPinotClientTransport, "dummy", _dummyPinotControllerTransport);
+    PreparedStatement preparedStatement = pinotConnection.prepareStatement(BASIC_TEST_QUERY);
+    preparedStatement.executeQuery();
+    String expectedSql = DriverUtils.createSetQueryOptionString(QueryOptionKey.ENABLE_NULL_HANDLING) + BASIC_TEST_QUERY;
+    Assert.assertEquals(expectedSql, _dummyPinotClientTransport.getLastQuery().substring(0, expectedSql.length()));
+  }
+
+  @Test
+  public void testSetEnableNullHandling2()
+      throws Exception {
+    Properties props = new Properties();
+    props.put(QueryOptionKey.ENABLE_NULL_HANDLING, "true");
+    PinotConnection pinotConnection =
+        new PinotConnection(props, "dummy", _dummyPinotClientTransport, "dummy", _dummyPinotControllerTransport);
+    PreparedStatement preparedStatement = pinotConnection.prepareStatement("");
+    preparedStatement.executeQuery(BASIC_TEST_QUERY);

Review Comment:
   the 2 tests is only different by the execution is whether prepared or now? can't we test them in just 1 method?



-- 
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@pinot.apache.org

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


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