[GitHub] [flink] LadyForest commented on a diff in pull request #22478: [FLINK-31917][table-planner] Fix the idempotence lost in JsonSerDe round trip for AggregateCall and RexNode

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

LadyForest commented on code in PR #22478:
URL: https://github.com/apache/flink/pull/22478#discussion_r1179903642


##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/serde/RexNodeJsonSerializer.java:
##########
@@ -230,12 +226,10 @@ private static void serializeLiteralValue(
             case BIGINT:
                 gen.writeNumberField(FIELD_NAME_VALUE, ((BigDecimal) value).longValue());
                 break;
-            case FLOAT:

Review Comment:
   In `RexNodeJsonSerdeTest`, we have a test spec, see https://github.com/apache/flink/blob/3664609c7622ccae80e36e85099a1b79b5935fe9/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/plan/nodes/exec/serde/RexNodeJsonSerdeTest.java#L578
   
   ```java
   rexBuilder.makeApproxLiteral(
                           BigDecimal.valueOf(Float.MAX_VALUE),
                           FACTORY.createSqlType(SqlTypeName.FLOAT))
   ```
   
   `makeApproxLiteral` will create the literal with `FLOAT` as the type of the literal, and `DOUBLE` as the SQL type of literal, see
   ```java
   /**
      * Creates an approximate numeric literal (double or float).
      *
      * @param bd   literal value
      * @param type approximate numeric type
      * @return new literal
      */
     public RexLiteral makeApproxLiteral(@Nullable BigDecimal bd, RelDataType type) {
       assert SqlTypeFamily.APPROXIMATE_NUMERIC.getTypeNames().contains(
           type.getSqlTypeName());
       return makeLiteral(bd, type, SqlTypeName.DOUBLE);
     }
   ```
   
   So now we're passing the type of the literal (i.e. the `FLOAT`) to the `serializeLiteral` method
   ```java
   serializeLiteralValue(value, literal.getType().getSqlTypeName(), gen)
   ```
   If we still serialize the `FLOAT` type using `FLOAT`, the serialized data may lose precision. 
   The reason why the previous version does not have this issue is that the parameter passed to the method is`DOUBLE`(the SQL type of literal)



-- 
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: issues-unsubscribe@flink.apache.org

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