dalelane commented on code in PR #29010:
URL: https://github.com/apache/flink/pull/29010#discussion_r3851529313


##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/formats/raw/RawFormatDeserializationSchema.java:
##########
@@ -278,6 +283,42 @@ public Object convert(byte[] data) {
         };
     }
 
+    /**
+     * Creates a converter that reads the bytes as a JSON document and parses 
it into a {@link
+     * Variant}. Duplicate object keys are rejected, matching the default of 
{@code PARSE_JSON}.
+     */
+    private static DeserializationRuntimeConverter createVariantConverter(
+            final String charsetName) {
+        // this also checks the charsetName is valid
+        final Charset configured = Charset.forName(charsetName);
+        final boolean parseFromBytes = 
configured.equals(StandardCharsets.UTF_8);
+
+        return new DeserializationRuntimeConverter() {
+            private static final long serialVersionUID = 1L;
+            private transient Charset charset;
+
+            @Override
+            public void open() {
+                charset = Charset.forName(charsetName);
+            }
+
+            @Override
+            public Object convert(byte[] data) {
+                try {
+                    return parseFromBytes
+                            ? BinaryVariantInternalBuilder.parseJson(data, 
false)
+                            : BinaryVariantInternalBuilder.parseJson(
+                                    new String(data, charset), false);

Review Comment:
   I'm curious why you're not using the charset in both paths. 
   
   If the user configures anything that isn't UTF-8, we'd respect and enforce 
what they declared. 
   
   If the user configures it to use UTF-8, we give the data to Jackson which 
IIUC will do it's own pattern-based encoding detection. So if Jackson doesn't 
think the data looks like UTF-8, the UTF-8 config would be ignored. 
   
   Is that intentional? (Am I mis-reading the intent of the config?)



##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/formats/raw/RawFormatSerializationSchema.java:
##########


Review Comment:
   Do you think we need to update the catch here to cover VariantTypeException 
that would be possible now? 



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to