snuyanzin commented on code in PR #26217:
URL: https://github.com/apache/flink/pull/26217#discussion_r1973343687


##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/serde/JsonSmileSerdeUtil.java:
##########
@@ -71,51 +73,71 @@
 import java.io.IOException;
 import java.util.Optional;
 
-/** A utility class that provide abilities for JSON serialization and 
deserialization. */
+/** A utility class that provide abilities for JSON and Smile serialization 
and deserialization. */
 @Internal
-public class JsonSerdeUtil {
+public class JsonSmileSerdeUtil {
 
     /**
      * Object mapper shared instance to serialize and deserialize the plan. 
Note that creating and
      * copying of object mappers is expensive and should be avoided.
      *
      * <p>This is not exposed to avoid bad usages, like adding new modules. If 
you need to read and
-     * write json persisted plans, use {@link 
#createObjectWriter(SerdeContext)} and {@link
-     * #createObjectReader(SerdeContext)}.
+     * write json or smile persisted plans, use {@link 
#createJsonObjectWriter(SerdeContext)} and
+     * {@link #createJsonObjectReader(SerdeContext)}.
      */
-    private static final ObjectMapper OBJECT_MAPPER_INSTANCE;
+    private static final ObjectMapper JSON_OBJECT_MAPPER_INSTANCE;
+
+    private static final ObjectMapper SMILE_OBJECT_MAPPER_INSTANCE =
+            JacksonMapperFactory.createObjectMapper(new SmileFactory())
+                    .disable(SerializationFeature.FAIL_ON_EMPTY_BEANS)
+                    .registerModule(createFlinkTableJacksonModule());

Review Comment:
   Added `MapperFeature.USE_GETTERS_AS_SETTERS` for consistency.
   
   about `SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS`, IIRC the only reason 
to have it is our tests against actual plans and the fact that it looks like in 
jdk (between 18 and 21) something has been changed in hashcode and same tests 
started to fail with jdk21. More info at 
https://github.com/apache/flink/pull/23562 and FLINK-33334.
   
   So this is not the case for `Smile`, so we could omit it



-- 
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-unsubscr...@flink.apache.org

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

Reply via email to