[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #11350: [multistage] Convert TIMESTAMP type value at reduce phase

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

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


##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -416,8 +419,14 @@ public Serializable convertAndFormat(Object value) {
         case BIG_DECIMAL:
           return (BigDecimal) value;
         case BOOLEAN:
+          if (value instanceof Boolean) {
+            return (boolean) value;

Review Comment:
   (minor)
   ```suggestion
               return value;
   ```



##########
pinot-query-planner/src/main/java/org/apache/pinot/query/parser/CalciteRexExpressionParser.java:
##########
@@ -194,6 +196,10 @@ public static Expression toExpression(RexExpression rexNode, PinotQuery pinotQue
   private static Expression rexLiteralToExpression(RexExpression.Literal rexLiteral) {
     // TODO: currently literals are encoded as strings for V1, remove this and use directly literal type when it
     // supports strong-type in V1.
+    if (rexLiteral.getDataType() == FieldSpec.DataType.TIMESTAMP
+        && rexLiteral.getValue() instanceof GregorianCalendar) {

Review Comment:
   (minor) Suggest removing the `rexLiteral.getValue() instanceof GregorianCalendar` so that if it returns unexpected value we can get exception instead of a completely wrong literal



##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ScalarTransformFunctionWrapper.java:
##########
@@ -121,6 +121,10 @@ public void init(List<TransformFunction> arguments, Map<String, ColumnContext> c
                 parameterTypes[i].convert(literalTransformFunction.getBigDecimalLiteral(), PinotDataType.BIG_DECIMAL);
             break;
           case TIMESTAMP:
+            if (parameterTypes[i] == PinotDataType.STRING) {

Review Comment:
   I feel this applies to all data types



##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -151,7 +151,11 @@ public static RowDataBlock buildFromRows(List<Object[]> rows, DataSchema dataSch
             byteBuffer.putInt(((Boolean) value) ? 1 : 0);
             break;
           case TIMESTAMP:
-            byteBuffer.putLong(((Timestamp) value).getTime());
+            if (value instanceof Long) {

Review Comment:
   Are we going to hit both? If so, can you add some java doc explaining when will we hit both? If we can hit both, then `BOOLEAN` will also break



##########
pinot-common/src/main/java/org/apache/pinot/common/utils/DataSchema.java:
##########
@@ -353,6 +353,9 @@ public Serializable convert(Object value) {
         case BOOLEAN:
           return ((Number) value).intValue() == 1;
         case TIMESTAMP:
+          if (value instanceof Timestamp) {

Review Comment:
   Does this still apply? Ideally we should always have internal format value. If we break that contract, `BOOLEAN` will also break



##########
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java:
##########
@@ -124,6 +124,10 @@ public enum TransformFunctionType {
               SqlTypeFamily.CHARACTER),
           ordinal -> ordinal > 1)),
 
+  FROMDATETIME("fromDateTime", ReturnTypes.TIMESTAMP_NULLABLE,

Review Comment:
   Please add a TODO to auto generate signature for scalar function. We should not manually add this for scalar 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