Skip to content

feat: support new CDC keys structure#46

Merged
venikkin merged 5 commits intoneo4j:mainfrom
venikkin:change-cdc-key-structure
Dec 11, 2023
Merged

feat: support new CDC keys structure#46
venikkin merged 5 commits intoneo4j:mainfrom
venikkin:change-cdc-key-structure

Conversation

@venikkin
Copy link
Contributor

@venikkin venikkin commented Dec 5, 2023

  • Updated CDC client versions
  • Updated CDC schema according to CDC client change. This is a breaking change. Unfortunately, no IT tests captured it (added suggestion to improve it into relevant card). Tested manually.

@venikkin
Copy link
Contributor Author

venikkin commented Dec 5, 2023

@venikkin venikkin marked this pull request as ready for review December 6, 2023 19:33
@venikkin venikkin requested review from ali-ince and fbiville December 6, 2023 19:33
Comment on lines +283 to +290
SchemaBuilder.struct()
.apply {
keys.forEach { key ->
key.forEach { field(it.key, DynamicTypes.schemaFor(it.value, true)) }
}
}
.optional()
.build())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got a bit confused about having keys as an array of structs, probably worth commenting on this.

Copy link
Contributor Author

@venikkin venikkin Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Before the change, each key, e.g. Map<String, Object> was mapped to a struct (struct of struct if we talk about about key per label). Now we have multiple keys, so I just wrapped the struct representing a key with an array. However, to reflect that each element if the array can have different structure, we have to define all possible key name in struct. For example: [ { "id": 1 }, { "name": "Joe" } ] corresponds to

array(struct { 
  optional int id,
  optional string name 
  })

Optional because id is null in the second element and name is null in the first element. For Avro value will like like (preudo-Avro here, just to show the structure): [{id: 1, name: null}, {id: null, name: "Joe"}].
Alternative option would be something like

  array(struct {
     string key, 
     optional int intValue,
     optional string stringValue
  })

with corresponding value [{"key": "id", "intValue": 1, "stringValue": null}, {"key": "name", "intValue": null, "stringValue": "Joe"}]
Maybe there could be a better option. Let's discuss.

Comment on lines +617 to +619
SchemaBuilder.map(
Schema.STRING_SCHEMA, Schema.STRING_SCHEMA)
.build())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a struct, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can be a struct, but I left it as map, just wrapped it in array. Let's discuss it we should change it to struct.

Comment on lines +635 to +638
SchemaBuilder.array(
SchemaBuilder.map(
Schema.STRING_SCHEMA, Schema.STRING_SCHEMA)
.build())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this too.

@venikkin venikkin requested a review from ali-ince December 11, 2023 09:27
Copy link
Contributor

@ali-ince ali-ince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@venikkin venikkin merged commit 750f829 into neo4j:main Dec 11, 2023
@venikkin venikkin deleted the change-cdc-key-structure branch December 11, 2023 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants