[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #8503: Support single-valued BigDecimal in schema, type conversion, SQL statements and minimum set of transforms.

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

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


##########
pinot-core/src/main/java/org/apache/pinot/core/common/BlockValSet.java:
##########
@@ -29,6 +29,13 @@
  */
 public interface BlockValSet {
 
+  /**
+   * Returns the number of entries for a single-valued column.
+   *
+   * @return number of single-valued entries
+   */
+  int getNumSVEntries();

Review Comment:
   I don't think we need this API. If it is really required, let's rename it to `getNumDocs()`



##########
pinot-clients/pinot-jdbc-client/src/main/java/org/apache/pinot/client/utils/DriverUtils.java:
##########
@@ -134,6 +135,10 @@ public static String getJavaClassName(String columnDataType) {
       case "DOUBLE":
         columnsJavaClassName = Double.class.getTypeName();
         break;
+      case "BIG_DECIMAL":

Review Comment:
   Should we add this to `getSQLDataType()` as well? Is it possible to be `BIGDECIMAL` if the value is from the `ResultSet`?



##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataBlockCache.java:
##########
@@ -255,9 +255,10 @@ public String[] getStringValuesForSVColumn(String column) {
    * @param column Column name
    * @param evaluator transform evaluator
    * @param buffer values to fill
+   * @param parseExactBigDecimal parse exact BigDecimal values
    */
-  public void fillValues(String column, TransformEvaluator evaluator, String[] buffer) {
-    _dataFetcher.fetchStringValues(column, evaluator, _docIds, _length, buffer);
+  public void fillValues(String column, TransformEvaluator evaluator, String[] buffer, boolean parseExactBigDecimal) {

Review Comment:
   We don't need to parse big decimal in the physical data access layer. It should be read as `byte[]`. It is not feasible to add physical data access for every individual logical types.



##########
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/IntermediateResultsBlock.java:
##########
@@ -328,14 +328,14 @@ public DataTable getDataTable()
   private DataTable getResultDataTable()
       throws IOException {
     DataTableBuilder dataTableBuilder = new DataTableBuilder(_dataSchema);
-    ColumnDataType[] storedColumnDataTypes = _dataSchema.getStoredColumnDataTypes();
+    ColumnDataType[] columnDataTypes = _dataSchema.getColumnDataTypes();

Review Comment:
   We don't need to change this. `BIG_DECIMAL` will be handled as `BYTES`



##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java:
##########
@@ -219,13 +250,74 @@ public double[] transformToDoubleValuesSV(ProjectionBlock projectionBlock) {
           String[] stringValues = transformToStringValuesSV(projectionBlock);
           ArrayCopyUtils.copy(stringValues, _doubleValuesSV, length);
           break;
+        case BYTES:
+          if (getResultMetadata().getDataType().equals(DataType.BIG_DECIMAL)) {
+            BigDecimal[] bigDecimalValues = transformToBigDecimalValuesSV(projectionBlock);
+            ArrayCopyUtils.copy(bigDecimalValues, _doubleValuesSV, length);
+            break;
+          }
+          throw new IllegalStateException();
         default:
           throw new IllegalStateException();
       }
     }
     return _doubleValuesSV;
   }
 
+  // Note 1: Why need transformToBigDecimalValuesSV(projectionBlock) while there is no one for TIMESTAMP data type?
+  // This is because TIMESTAMP's underlying stored type is LONG and we already have a method for LONG that can convert
+  // to other numeric data types (check: transformToLongValuesSV(projectionBlock) method).
+  // However, BIG_DECIMAL's underlying stored type is BYTES and transformToBytesValuesSV(projectionBlock) handles only
+  // the conversion from STRING (check: transformToBytesValuesSV(projectionBlock) method).
+  //
+  // Note 2: An alternative way of transforming projectionBlock into BigDecimal[] values is to call
+  // transformToBytesValuesSV(projectionBlock) method that return byte[] values and then transform byte[] to
+  // BigDecimal[]. However, this requires three operations (vs. two in this method):
+  // 1- transformTo[Type]ValuesSV(projectionBlock) based on the underlying stored type.
+  // 2- ArrayCopyUtils.copy([Type]Values, bytesValues, length).
+  // 3- ArrayCopyUtils.copy(bytesValues, bigDecimals, length).
+  @Override
+  public BigDecimal[] transformToBigDecimalValuesSV(ProjectionBlock projectionBlock) {

Review Comment:
   Let's not add this method for now. We might not want to support implicit cast to/from big decimal to other data types (this is not the SQL semantic as well). Users can cast explicitly using transform function



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