[GitHub] [incubator-pinot] mayankshriv commented on a change in pull request #6403: Fix upcasting/downcasting of numerical values used in SQL predicates.

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

mayankshriv commented on a change in pull request #6403:
URL: https://github.com/apache/incubator-pinot/pull/6403#discussion_r556908539



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/FloatDictionary.java
##########
@@ -30,7 +31,24 @@ public FloatDictionary(PinotDataBuffer dataBuffer, int length) {
 
   @Override
   public int insertionIndexOf(String stringValue) {
-    return binarySearch(Float.parseFloat(stringValue));
+    // First convert string to BigDecimal and then downcast to int. This allows type conversion from any compatible
+    // numerical value represented as string to an int value.
+    BigDecimal bigDecimal = new BigDecimal(stringValue);

Review comment:
       This is a FloatDictionary, so we can safely assume that the caller stack ensure it is called with a stringValue that is expected to be float parsed. I don't think we should convert it into BigDecimal.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/index/readers/IntDictionary.java
##########
@@ -30,7 +31,24 @@ public IntDictionary(PinotDataBuffer dataBuffer, int length) {
 
   @Override
   public int insertionIndexOf(String stringValue) {
-    return binarySearch(Integer.parseInt(stringValue));
+    // First convert string to BigDecimal and then downcast to int. This allows type conversion from any compatible
+    // numerical value represented as string to an int value.
+    BigDecimal bigDecimal = new BigDecimal(stringValue);

Review comment:
       Same here, and all other data type dependent dictionaries.

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ColumnValueSegmentPruner.java
##########
@@ -239,17 +240,24 @@ private boolean pruneRangePredicate(IndexSegment segment, RangePredicate rangePr
     return false;
   }
 
+  /**
+   * Convert String value to specified numerical type. We first verify that the input string contains a number by parsing
+   * it as BigDecimal. The resulting BigDecimal is then downcast to specified numerical type. This allows us to create predicates
+   * which allow for comparing values of two different numerical types such as:
+   * SELECT * FROM table WHERE a > 5.0
+   * SELECT * FROM table WHERE timestamp > NOW() - 5.0.
+   */
   private static Comparable convertValue(String stringValue, DataType dataType) {
     try {
       switch (dataType) {
         case INT:
-          return Integer.valueOf(stringValue);
+          return (new BigDecimal(stringValue)).intValue();

Review comment:
       Using BigDecimal may have performance penalty. If we already know the data type for the column, why not down-cast early on?




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

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