DBZ-7536 Add support for default value for text_ and varchar_ in post… #5308

akanimesh7 · 2024-02-21T10:15:27Z

Solves https://issues.redhat.com/browse/DBZ-7536

Before

Newly added column definition

col2 text[] DEFAULT '{}'::text[] NOT NULL

Representation of the field in the generated Avro Schema

{
  "name": "col2",
  "type": {
    "items": [
      "null",
      "string"
    ],
    "type": "array"
  }
}

After

Empty array default

Newly added column definition

col2 text[] DEFAULT '{}'::text[] NOT NULL

Representation of the field in the generated Avro Schema

{
    "default": [],
    "name": "col2",
    "type": {
        "connect.default": [],
        "items": "string",
        "type": "array"
    }
}

Non Empty array default

Newly added column definition

col2 text[] DEFAULT '{a,b}'::text[] NOT NULL

Representation of the field in the generated Avro Schema

{
    "default": [
        "a",
        "b"
    ],
    "name": "col2",
    "type": {
        "connect.default": [
            "a",
            "b"
        ],
        "items": "string",
        "type": "array"
    }
}

jpechane

@akanimesh7 This is good start, thanks! Could you please take a look at the comments I left?

jpechane · 2024-02-21T12:17:20Z

...onnector-postgres/src/main/java/io/debezium/connector/postgresql/PostgresValueConverter.java

@@ -253,7 +253,7 @@ public SchemaBuilder schemaBuilder(Column column) {
            case PgOid.INT4RANGE_ARRAY:
            case PgOid.NUM_RANGE_ARRAY:
            case PgOid.INT8RANGE_ARRAY:
-                return SchemaBuilder.array(SchemaBuilder.OPTIONAL_STRING_SCHEMA);
+                return SchemaBuilder.array(SchemaBuilder.STRING_SCHEMA);


Can't we keep the type optional?

Let's say the DDL operation was
ALTER TABLE CUSTOMERS ADD COLUMN col2 text[] DEFAULT '{a,b}'::text[] NOT NULL;
So how it works is when the avro schema is created from the connect schema, The default value {"a","b"} for col2 is proposed to the avro schema. It verifies this default value. Refer this. Now since we have kept the type optional so this verification fails since the string "a" is not a null, which is the first type in the array of types in this union.

Having said that, i also see that there's a fix for this which has been merged 5 months before, but there has been no release after that.

@akanimesh7 OK, TBH I am really not fan of this change if it just by-pass a bug in Avro. To handle this properly we there should be an SMT developed that will flip the schema optionality if needed but I don't think this should go to the core.

@jpechane Understood. In that case rather than doing it for all array types, can we just do it for text[] and varchar[] ? As soon as the newer version of avro is released, these two also can be corrected. Even earlier, these two types were not supported, hence there is no harm, rather this is just an addition.

On the other hand creation of SMT will need some more time and will also be temporary code that will come in.

Well, the problem is that you focus on it from Avro PoV only. But there are also converters for JSON and Protobuf. It does not feel correct to me to emit incorrect schema type for everybody to fix issue in Avro only.

Okay, makes sense. Let's halt this one then until a new version of avro comes out with the bug fix.

jpechane · 2024-02-21T12:18:16Z

...src/main/java/io/debezium/connector/postgresql/connection/PostgresDefaultValueConverter.java

@@ -173,6 +176,8 @@ private static Map<String, DefaultValueMapper> createDefaultValueMappers(Timesta
        result.put("time", (c, v) -> timestampUtils.toLocalTime(extractDefault(v, "00:00")));
        result.put("timestamp", (c, v) -> timestampUtils.toOffsetDateTime(extractDefault(v, "1970-01-01")));
        result.put("timestamptz", (c, v) -> timestampUtils.toOffsetDateTime(extractDefault(v, "1970-01-01")).atZoneSameInstant(ZoneOffset.UTC));
+        result.put("_text", parseNullDefault((c, v) -> createStringArr(extractDefault(v, "{}"))));


Is this issue for _text/_varchar only or are all array datatypes affected?

Now that i think of it, looks like all array datatypes will be affected. Fixed only for these since this was impacting us. Will need a thorough review of other array datatypes.

On the same note the default mapper did not have any array data types to begin with. These are the first few additions.

@akanimesh7 Would it be possible to provide a sensible default - thus providing at least the empty List/Array for all array types and later on add support for the specific types?

Yes it's possible. I'll work on that change.

jpechane · 2024-02-21T12:19:32Z

...src/main/java/io/debezium/connector/postgresql/connection/PostgresDefaultValueConverter.java

@@ -188,6 +193,25 @@ private static Map<String, DefaultValueMapper> createDefaultValueMappers(Timesta
        return result;
    }

+    private static List<String> createStringArr(String arrayString) {


Suggested change

private static List<String> createStringArr(String arrayString) {

private static List<String> parseArrayString(String arrayString) {

…gres

akanimesh7 · 2024-02-21T12:36:17Z

@jpechane Addressed comments. PTAL

akanimesh7 · 2024-02-21T15:36:48Z

@jpechane
Understood your concern and added a proposal here.
I can make the default value change for other array types once we agree on something for the above.

jpechane reviewed Feb 21, 2024

View reviewed changes

DBZ-7536 Add support for default value for text_ and varchar_ in post…

82cd778

…gres

akanimesh7 force-pushed the DBZ-7536 branch from 061997e to 82cd778 Compare February 21, 2024 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DBZ-7536 Add support for default value for text_ and varchar_ in post… #5308

DBZ-7536 Add support for default value for text_ and varchar_ in post… #5308

akanimesh7 commented Feb 21, 2024 •

edited

jpechane left a comment

jpechane Feb 21, 2024

akanimesh7 Feb 21, 2024

jpechane Feb 21, 2024

akanimesh7 Feb 21, 2024 •

edited

jpechane Feb 22, 2024

akanimesh7 Feb 22, 2024

jpechane Feb 21, 2024

akanimesh7 Feb 21, 2024

jpechane Feb 21, 2024

akanimesh7 Feb 21, 2024

jpechane Feb 21, 2024

akanimesh7 commented Feb 21, 2024

akanimesh7 commented Feb 21, 2024

	private static List<String> createStringArr(String arrayString) {
	private static List<String> parseArrayString(String arrayString) {

DBZ-7536 Add support for default value for text_ and varchar_ in post… #5308

Are you sure you want to change the base?

DBZ-7536 Add support for default value for text_ and varchar_ in post… #5308

Conversation

akanimesh7 commented Feb 21, 2024 • edited

Before

After

Empty array default

Non Empty array default

jpechane left a 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

akanimesh7 Feb 21, 2024 • 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

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

akanimesh7 commented Feb 21, 2024

akanimesh7 commented Feb 21, 2024

akanimesh7 commented Feb 21, 2024 •

edited

akanimesh7 Feb 21, 2024 •

edited