fix: struct be converted to String for json_sr format #9678

aliehsaeedii · 2022-10-31T19:39:45Z

fixes issue #9580

Description

A topic that has a STRUCT column cannot be deserialized from KSQL if VARCHAR is used in the KSQL schema. This issue is found only on JSON_SR topics. This fix addresses this issue.

Testing done

manual testing
QTT

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

jnh5y · 2022-10-31T23:56:23Z

ksqldb-serde/src/test/java/io/confluent/ksql/serde/avro/KsqlAvroDeserializerTest.java

    // Then:
-    assertThat(e.getCause(), (hasMessage(containsString(


@spena wrote this code. Maybe he knows about this...

spena · 2022-11-01T16:18:22Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/json_sr.json

+        {"topic": "test_topic", "value": {"FOO": {"F0": 1}}}
+      ],
+      "outputs": [
+        {"topic": "OUTPUT", "value": {"FOO": "Struct{F0=1}"}}


Could you check a plain JSON format to see how the Struct looks like? I think the plain JSON is only {F0=1} (withouth the Struct word).

Could you check a plain JSON format to see how the Struct looks like? I think the plain JSON is only {F0=1} (withouth the Struct word).

The plain json does not have the word Struct (As you told). I believe that the word Struct comes from the toString() method implemented by the Struct class: org.apache.kafka.connect.data.Struct.toString()

but protobuf and avro show the word Struct as well.

spena · 2022-11-01T16:21:16Z

ksqldb-serde/src/main/java/io/confluent/ksql/serde/connect/ConnectDataTranslator.java

@@ -231,7 +232,7 @@ private Object toKsqlValue(
      case STRUCT:
        return toKsqlStruct(schema, connectSchema, (Struct) convertedValue, pathStr);
      case STRING:
-        // use String.valueOf to convert various int types and Boolean to string
+        // use String.valueOf to convert various int types, Struct and Boolean to string
        return String.valueOf(convertedValue);


I think something wrong is gonna happen if we done here. The ConnectDataTranslator is a generic class for all Connect types (avro, protobuf, json). The struct->string conversion should be in JSON string, i.e. {k1=v1}. You can verify this with a plain JSON format or with JSON_SR in version 0.26 or lower. I actually not sure what Avro/Protobuf should do wit Struct->String.

Could you check in older ksql versions if Struct->String is supported with Avro and Protobuf?

Could you check in older ksql versions what Struct->String wit JSON_SR and plain JSON display?

I think something wrong is gonna happen if we done here.

The issue addresses exactly this line for change (please have a look at the issue explanation)

spena · 2022-11-04T16:23:49Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/json_sr.json

+        {"topic": "test_topic", "value": {"FOO": {"F0": 1, "F1": "bar"}}}
+      ],
+      "outputs": [
+        {"topic": "OUTPUT", "value": {"FOO": "{F0=1,F1=bar}"}}


I run these cases and got the following JSON output in the old version:

{"F0":1,"F1":"2"} {"F0":3,"F1":"this is a string"} {"F0":3,"F1":"\"quotes\""} {"F0":3,"F1":""} null

Could you verify the JSON string will be formed like that?

I tried to add a ' single-quote escaped but KSQL didn't let me. Can you figure out how to add that in case KSQL finds a string with it?

Also, could you add more complex columns? say f2 struct<a1 int, a2 struct <b1 string>>. Include maps too. I think the code does not take into consideration structs inside a struct.

Also add all the types we support to make sure everything is well covered and tested.

Also, could you add more complex columns? say f2 struct<a1 int, a2 struct <b1 string>>. Include maps too. I think the code does not take into consideration structs inside a struct.

You mean the whole f2 struct<a1 int, a2 struct <b1 string>> must be convertible to string as well? Currently the output is {a1=1,a2=Struct{b1=bar}}. Is it acceptable, or you think the Struct word must be removed?

The Struct should be removed too. You could check the old JSON_SR deserialization to see how it works.

spena · 2022-11-04T16:28:29Z

ksqldb-serde/src/main/java/io/confluent/ksql/serde/connect/ConnectDataTranslator.java

@@ -129,7 +129,8 @@ private static void validateType(
      Schema.Type.FLOAT32,
      Schema.Type.FLOAT64,
      Schema.Type.BOOLEAN,
-      Schema.Type.STRING
+      Schema.Type.STRING,
+      Schema.Type.STRUCT


This issue was created for STRUCT->STRING. But the issue also happens for ARRAY->STRING. Look at the KsqlJsonDeserializer class and processString method. That's where the old JSON_SR deserializer was doing.

Do you think you can add the ARRAY conversion to this PR or would you need another one?

spena · 2022-11-04T16:36:32Z

ksqldb-serde/src/main/java/io/confluent/ksql/serde/connect/ConnectDataTranslator.java

@@ -244,6 +248,27 @@ private Object toKsqlValue(
    }
  }

+  private String structToString(final Struct input) {


Is there an API we can use to generate a JSON string? Something that handle cases with String characters (special chars, nulls)? Not sure if we can re-use the ObjectMapper to generate a JSON string.

aliehsaeedii requested a review from a team as a code owner October 31, 2022 19:39

aliehsaeedii assigned aliehsaeedii and spena and unassigned aliehsaeedii Oct 31, 2022

jnh5y reviewed Oct 31, 2022

View reviewed changes

spena reviewed Nov 1, 2022

View reviewed changes

spena reviewed Nov 4, 2022

View reviewed changes

aliehsaeedii added 8 commits December 28, 2022 19:48

fix: struct be converted to String for json_sr format

2f3a042

test: run manuallyGeneratePlans()

89700f5

test: fix related unit tests

33f232d

style: remove comments

3bddc47

style: efficient use of entryset instead of keyset

abe597f

fix: remove Struct word

938210c

style: remove checkstyle errors

f1b0101

fix: cover all types

139e3ec

aliehsaeedii force-pushed the fix-9580 branch from 03355ec to 139e3ec Compare December 29, 2022 23:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: struct be converted to String for json_sr format #9678

fix: struct be converted to String for json_sr format #9678

aliehsaeedii commented Oct 31, 2022

jnh5y Oct 31, 2022

spena Nov 1, 2022

aliehsaeedii Nov 1, 2022

aliehsaeedii Nov 2, 2022

spena Nov 1, 2022

aliehsaeedii Nov 1, 2022 •

edited

spena Nov 4, 2022

spena Nov 4, 2022

spena Nov 4, 2022

aliehsaeedii Nov 7, 2022 •

edited

spena Nov 7, 2022

spena Nov 4, 2022

spena Nov 4, 2022

		// Then:
		assertThat(e.getCause(), (hasMessage(containsString(

fix: struct be converted to String for json_sr format #9678

Are you sure you want to change the base?

fix: struct be converted to String for json_sr format #9678

Conversation

aliehsaeedii commented Oct 31, 2022

Description

Testing done

Reviewer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliehsaeedii Nov 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliehsaeedii Nov 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliehsaeedii Nov 1, 2022 •

edited

aliehsaeedii Nov 7, 2022 •

edited