Fixed avro timestamp types #613
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes handling of Avro timestamp types by improving DateTime64 type conversions with proper precision support. The changes ensure that Avro logical types like timestamp-millis and time-millis are correctly converted and written to ClickHouse with appropriate precision handling.
Key Changes
- Enhanced DateTime64 handling to support multiple precision levels (3, 6, 9 digits) with proper conversion logic
- Added test coverage for Avro timestamp types with new
Eventclass and corresponding test cases - Refactored Avro-to-SinkRecord conversion logic into a reusable utility method
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ClickHouseWriter.java | Added precision-based timestamp conversion logic for DateTime64 types with scaling factor array |
| SchemaTestData.java | Added utility method to convert Avro records to SinkRecords and expanded DateTime64 test schemas |
| SchemalessTestData.java | Added helper method to create test records with various DateTime types |
| ClickHouseSinkTaskWithSchemaTest.java | Added comprehensive test for Avro timestamp types and enhanced existing date/time tests |
| Event.java | Auto-generated Avro class for testing timestamp and time logical types |
| event.idl | Avro schema definition for Event record with timestamp-millis and time-millis fields |
| README.md | Documentation for generating Avro schemas and Java classes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/testFixtures/avro/event.idl
Outdated
| // The time-millis logical type represents a time of day, with no reference to a particular calendar, time zone or date, with a precision of one millisecond. | ||
| timestamp_ms time1; | ||
|
|
||
| // The timestamp-millis logical type represents an instant on the global timeline, independent of a particular time zone or calendar, with a precision of one millisecond. |
There was a problem hiding this comment.
The comments for time1 and time2 appear to be swapped. The field time1 uses timestamp_ms but has a comment describing time-millis, while time2 uses time_ms but has a comment describing timestamp-millis. The comments should match their respective field types.
| // The time-millis logical type represents a time of day, with no reference to a particular calendar, time zone or date, with a precision of one millisecond. | |
| timestamp_ms time1; | |
| // The timestamp-millis logical type represents an instant on the global timeline, independent of a particular time zone or calendar, with a precision of one millisecond. | |
| // The timestamp-millis logical type represents an instant on the global timeline, independent of a particular time zone or calendar, with a precision of one millisecond. | |
| timestamp_ms time1; | |
| // The time-millis logical type represents a time of day, with no reference to a particular calendar, time zone or date, with a precision of one millisecond. |
| } | ||
| return validSchema; | ||
| } | ||
|
|
There was a problem hiding this comment.
The BASES array lacks documentation explaining the mapping between array indices and precision levels. Add a comment describing how each index corresponds to a specific precision value (e.g., index 3 = precision 3, value 1 = no scaling).
| /** | |
| * BASES array maps precision levels to scaling factors for date/time values. | |
| * The index corresponds to the precision (e.g., index 3 = precision 3). | |
| * Value at each index is the scaling factor (e.g., value 1 = no scaling). | |
| * Example: BASES[3] == 1 means precision 3 uses no scaling. | |
| */ |
| }).collect(Collectors.toList()); | ||
|
|
||
| List<SinkRecord> records = SchemaTestData.convertAvroToSinkRecord(topic, new AvroSchema(Event.getClassSchema()), events); | ||
| System.out.println(records); |
There was a problem hiding this comment.
Debug print statement System.out.println(records) should be removed or replaced with proper logging before merging to production.
| List<Object> events = IntStream.range(0, 3).mapToObj(i -> { | ||
| Event event = Event.newBuilder() | ||
| .setId(i) | ||
| .setTime1(now) |
There was a problem hiding this comment.
we using same now for all records
There was a problem hiding this comment.
Got it. Will change to producing random time.
| ClickHouseSinkTask chst = new ClickHouseSinkTask(); | ||
| chst.start(props); | ||
| chst.put(records); | ||
| chst.stop(); |
There was a problem hiding this comment.
I do not see any assertion to validate the test
There was a problem hiding this comment.
Sorry, missed it :-(
Will add.
Summary
Closes: #599
Checklist
Delete items not relevant to your PR: