-
Notifications
You must be signed in to change notification settings - Fork 56
Added support of writing union {string, bytes} to string #606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for writing Avro messages with union {string, bytes} fields to ClickHouse String columns. The implementation handles union types by extracting the non-null value from the union and converting it appropriately for string storage.
Key changes:
- Enhanced schema validation to allow union {string, bytes} types for String columns
- Added union type handling in the data writing logic to extract and convert union values
- Extended test coverage with a new nullable union field
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/testFixtures/avro/test.idl | Added nullable union field description to test schema |
| src/testFixtures/java/com/clickhouse/kafka/connect/avro/test/Image.java | Generated Avro class updated with new description field and constructor |
| src/test/java/com/clickhouse/kafka/connect/sink/ClickHouseSinkTaskWithSchemaTest.java | Enhanced test to verify union field handling with database operations |
| src/main/java/com/clickhouse/kafka/connect/sink/db/ClickHouseWriter.java | Core logic for validating and writing union {string, bytes} to String columns |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } | ||
| if (!validSchema) { | ||
| LOGGER.error(String.format("Cannot write field of union schema '%s' to column [%s] of `String`: only unions of `string` and `bytes` are allowed in this case", | ||
| objSchema.schema().fields(), colName)); |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is calling objSchema.schema().fields() but objSchema is already a Schema object, so this should be objSchema.fields() to avoid a potential NullPointerException or incorrect field access.
| objSchema.schema().fields(), colName)); | |
| objSchema.fields(), colName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even if it is primitive type it will have empty field.
src/main/java/com/clickhouse/kafka/connect/sink/db/ClickHouseWriter.java
Show resolved
Hide resolved
src/main/java/com/clickhouse/kafka/connect/sink/db/ClickHouseWriter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
union {string, bytes}toStringcolumnsNote:
unions should be resolved before passing records to the connector because
Currently we allow only this case because it is essential and simple to handle.
Closes #572
Checklist
Delete items not relevant to your PR: