[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10177: [bugfix] Fix null value handling in PinotResultSet

["Jackie-Jiang (via GitHub)" <gi...@apache.org>]

Jackie-Jiang commented on code in PR #10177:
URL: https://github.com/apache/pinot/pull/10177#discussion_r1087137829


##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/utils/DriverUtils.java:
##########
@@ -50,6 +51,8 @@ public class DriverUtils {
   public static final Logger LOG = LoggerFactory.getLogger(DriverUtils.class);
   private static final String LIMIT_STATEMENT_REGEX = "\\s(limit)\\s";
 
+  public static final String ENABLE_NULL_HANDLING = "enableNullHandling";

Review Comment:
   Shall we reference the key in `CommonConstants.Broker.Request.QueryOptionKey.ENABLE_NULL_HANDLING`



##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/utils/DriverUtils.java:
##########
@@ -217,4 +220,14 @@ public static boolean queryContainsLimitStatement(String query) {
     Matcher matcher = pattern.matcher(query);
     return matcher.find();
   }
+
+  public static String checkEnableNullHandling(PinotConnection connection, String query) {
+    if (query.contains(ENABLE_NULL_HANDLING)) {
+      return query;
+    }
+
+    return connection.isNullHandlingEnabled()
+      ? String.format("%s option(%s=true)", query, ENABLE_NULL_HANDLING)

Review Comment:
   Let's use the new query option format:
   `SET enableNullHandling = true; ...`



##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/utils/DriverUtils.java:
##########
@@ -217,4 +220,14 @@ public static boolean queryContainsLimitStatement(String query) {
     Matcher matcher = pattern.matcher(query);
     return matcher.find();
   }
+
+  public static String checkEnableNullHandling(PinotConnection connection, String query) {

Review Comment:
   Suggest renaming to `enableNullHandling` because it will modify the query



##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotConnection.java:
##########
@@ -64,12 +66,18 @@ public class PinotConnection extends AbstractBaseConnection {
       brokers = getBrokerList(controllerURL, tenant);
     }
     _session = new org.apache.pinot.client.Connection(properties, brokers, transport);
+
+    _enableNullHandling = Boolean.parseBoolean(properties.getProperty(DriverUtils.ENABLE_NULL_HANDLING, "FALSE"));

Review Comment:
   (minor)
   ```suggestion
       _enableNullHandling = Boolean.parseBoolean(properties.getProperty(DriverUtils.ENABLE_NULL_HANDLING));
   ```



##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/PinotResultSet.java:
##########
@@ -194,8 +194,7 @@ public BigDecimal getBigDecimal(int columnIndex, int scale)
       throws SQLException {
     try {
       String value = getString(columnIndex);
-      BigDecimal bigDecimal = new BigDecimal(value).setScale(scale);
-      return bigDecimal;
+      return wasNull() ? null : new BigDecimal(value).setScale(scale);

Review Comment:
   Seems `getString()` already checks whether the value is null, so here we can simply check if the `value == null`. Same for other places



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