[jira] [Commented] (FLINK-31917) Loss of Idempotence in JsonSerDe Round Trip for AggregateCall and RexNode
[
https://issues.apache.org/jira/browse/FLINK-31917?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17719123#comment-17719123
]
Shengkai Fang commented on FLINK-31917:
---
Merged into master: 333e023196d90d265c286632f8c01c41b8911ef8
> Loss of Idempotence in JsonSerDe Round Trip for AggregateCall and RexNode
> -
>
> Key: FLINK-31917
> URL: https://issues.apache.org/jira/browse/FLINK-31917
> Project: Flink
> Issue Type: Bug
> Components: Table SQL / Planner
>Affects Versions: 1.18.0
>Reporter: Jane Chan
>Assignee: Jane Chan
>Priority: Major
> Labels: pull-request-available
> Fix For: 1.18.0
>
>
> JsonSerDeTestUtil#testJsonRoundTrip only checks the equality between spec and
> deserialized object. Some corner cases are detected when serializing the
> deserialized object again.
> {code:java}
> static T testJsonRoundTrip(SerdeContext serdeContext, T spec, Class
> clazz)
> throws IOException {
> String actualJson = toJson(serdeContext, spec);
> T actual = toObject(serdeContext, actualJson, clazz);
> assertThat(actual).isEqualTo(spec);
> assertThat(actualJson).isEqualTo(toJson(serdeContext, actual)); //
> this will eval some corner cases
> return actual;
> }
> {code}
> The discovered corner cases are listed as follows.
> h5. 1. SerDe for AggregateCall
> When deserializing the aggregate call, we should check the JsonNodeType to
> avoid converting null to "null" string.
> [https://github.com/apache/flink/blob/f9b3e0b7bc0432001b4a197539a0712b16e0b33b/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/serde/AggregateCallJsonDeserializer.java#L64]
> h5. Suggested Fix
> {code:java}
> JsonNode nameNode = jsonNode.required(FIELD_NAME_NAME);
> final String name = JsonNodeType.NULL ? null : nameNode.asText();
> {code}
> h5. 2. SerDe for RexNode
> RexNodeJsonSerdeTest#testSystemFunction should create the temporary system
> function instead of the temporary catalog function.
> [https://github.com/apache/flink/blob/f9b3e0b7bc0432001b4a197539a0712b16e0b33b/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/plan/nodes/exec/serde/RexNodeJsonSerdeTest.java#L209]
> h5. Suggested Fix
> Use functionCatalog#registerTemporarySystemFunction to test.
> h5. 3. About RexLiteral type
> RexNodeJsonSerdeTest#testRexNodeSerde has a test spec as follows
> {code:java}
> //This will create the literal with DOUBLE as the literal type, and DECIMAL
> as the broad type of this literal. You can refer to Calcite for more details
> rexBuilder.makeExactLiteral(BigDecimal.valueOf(Double.MAX_VALUE),
> FACTORY.createSqlType(SqlTypeName.DOUBLE))
> {code}
> The RexNodeJsonSerializer uses `typeName`(which is DECIMAL) as the literal's
> type, as a result, the rel data type is serialized as double, but the value
> is serialized as a string (in case lost the precision)
> [https://github.com/apache/flink/blob/f9b3e0b7bc0432001b4a197539a0712b16e0b33b/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/serde/RexNodeJsonSerializer.java#L197]
> And then, during the deserialization, according to the JSON, the deserialized
> literal will assign DOUBLE as the literal type and the broad type of the
> literal.
> This will cause the comparison failure
> {code:java}
> expected: {"kind": "LITERAL", "value": "1.7976931348623157E+308"}
> actual: {"kind": "LITERAL", "value": 1.7976931348623157E+308}
> {code}
> h5. Suggested Fix
> SARG is a special case and can be coped first, and for the rest type, we can
> use literal.getType().getSqlTypeName() instead of literal.getTypeName().
> {code:java}
> // first cope with SARG type
> if (literal.getTypeName() == SARG) {
> serializeSargValue(
> (Sarg) value, literal.getType().getSqlTypeName(), gen,
> serializerProvider);
> } else {
> serializeLiteralValue(
> value,
> literal.getType().getSqlTypeName(),
> gen,
> serializerProvider);
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (FLINK-31917) Loss of Idempotence in JsonSerDe Round Trip for AggregateCall and RexNode
[
https://issues.apache.org/jira/browse/FLINK-31917?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17715711#comment-17715711
]
Godfrey He commented on FLINK-31917:
good catch [~qingyue]
> Loss of Idempotence in JsonSerDe Round Trip for AggregateCall and RexNode
> -
>
> Key: FLINK-31917
> URL: https://issues.apache.org/jira/browse/FLINK-31917
> Project: Flink
> Issue Type: Bug
> Components: Table SQL / Planner
>Affects Versions: 1.18.0
>Reporter: Jane Chan
>Assignee: Jane Chan
>Priority: Major
> Fix For: 1.18.0
>
>
> JsonSerDeTestUtil#testJsonRoundTrip only checks the equality between spec and
> deserialized object. Some corner cases are detected when serializing the
> deserialized object again.
> {code:java}
> static T testJsonRoundTrip(SerdeContext serdeContext, T spec, Class
> clazz)
> throws IOException {
> String actualJson = toJson(serdeContext, spec);
> T actual = toObject(serdeContext, actualJson, clazz);
> assertThat(actual).isEqualTo(spec);
> assertThat(actualJson).isEqualTo(toJson(serdeContext, actual)); //
> this will eval some corner cases
> return actual;
> }
> {code}
> The discovered corner cases are listed as follows.
> h5. 1. SerDe for AggregateCall
> When deserializing the aggregate call, we should check the JsonNodeType to
> avoid converting null to "null" string.
> [https://github.com/apache/flink/blob/f9b3e0b7bc0432001b4a197539a0712b16e0b33b/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/serde/AggregateCallJsonDeserializer.java#L64]
> h5. Suggested Fix
> {code:java}
> JsonNode nameNode = jsonNode.required(FIELD_NAME_NAME);
> final String name = JsonNodeType.NULL ? null : nameNode.asText();
> {code}
> h5. 2. SerDe for RexNode
> RexNodeJsonSerdeTest#testSystemFunction should create the temporary system
> function instead of the temporary catalog function.
> [https://github.com/apache/flink/blob/f9b3e0b7bc0432001b4a197539a0712b16e0b33b/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/plan/nodes/exec/serde/RexNodeJsonSerdeTest.java#L209]
> h5. Suggested Fix
> Use functionCatalog#registerTemporarySystemFunction to test.
> h5. 3. About RexLiteral type
> RexNodeJsonSerdeTest#testRexNodeSerde has a test spec as follows
> {code:java}
> //This will create the literal with DOUBLE as the literal type, and DECIMAL
> as the broad type of this literal. You can refer to Calcite for more details
> rexBuilder.makeExactLiteral(BigDecimal.valueOf(Double.MAX_VALUE),
> FACTORY.createSqlType(SqlTypeName.DOUBLE))
> {code}
> The RexNodeJsonSerializer uses `typeName`(which is DECIMAL) as the literal's
> type, as a result, the rel data type is serialized as double, but the value
> is serialized as a string (in case lost the precision)
> [https://github.com/apache/flink/blob/f9b3e0b7bc0432001b4a197539a0712b16e0b33b/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/serde/RexNodeJsonSerializer.java#L197]
> And then, during the deserialization, according to the JSON, the deserialized
> literal will assign DOUBLE as the literal type and the broad type of the
> literal.
> This will cause the comparison failure
> {code:java}
> expected: {"kind": "LITERAL", "value": "1.7976931348623157E+308"}
> actual: {"kind": "LITERAL", "value": 1.7976931348623157E+308}
> {code}
> h5. Suggested Fix
> SARG is a special case and can be coped first, and for the rest type, we can
> use literal.getType().getSqlTypeName() instead of literal.getTypeName().
> {code:java}
> // first cope with SARG type
> if (literal.getTypeName() == SARG) {
> serializeSargValue(
> (Sarg) value, literal.getType().getSqlTypeName(), gen,
> serializerProvider);
> } else {
> serializeLiteralValue(
> value,
> literal.getType().getSqlTypeName(),
> gen,
> serializerProvider);
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
