[FLINK-40594][cdc-common] Preserve TIME precision in the pipeline runtime - #4530
[FLINK-40594][cdc-common] Preserve TIME precision in the pipeline runtime#4530tchivs wants to merge 2 commits into
Conversation
…time TimeType accepts a precision of up to 9 and the Debezium deserializer already produces microsecond and nanosecond values, but the runtime stored TIME as millisecond-of-day. Every TIME(p) with p > 3 was silently truncated, and two values differing only below the millisecond compared equal, collapsing them during deduplication, ordering and key comparison. TimeData now keeps nanosecond-of-day and exposes toMicroOfDay/toNanoOfDay; toMillisOfDay keeps its signature and truncating behaviour. TimeDataSerializer becomes precision-aware like TimestampDataSerializer: precision <= 3 keeps the historical four-byte millisecond encoding and stays compatible as is, while precision > 3 uses an eight-byte nanosecond encoding reached through a versioned snapshot that reports compatible-after-migration and still reads the legacy snapshot envelope and four-byte payload. The binary writers/readers and InternalSerializers thread the declared precision through. Two existing expectations pinned the truncation and are corrected rather than relaxed: the TIME(6)/TIME(9) transform expectation now asserts the retained fraction, and the temporal-function check compares LOCALTIME at whole-second granularity because LOCALTIME and CURRENT_TIME are TIME(0).
|
Thanks Lanny for the contribution, please check the CI failing cases as they seems relevant. @leonardBang IIRC the TIME(3) precision has been discussed earlier, and the conclusion is we'd better keep the same memory layout with Flink SQL, truncating to 3-digits. As apache/flink#26954 has extended precision up to 9, should this be reconsidered now? |
Thanks @tchivs for the contribution and @yuxiqian for the investigation, I think it's time to support TIME(p) with p > 3 |
…sion change GenericRecordDataSerializer still wrote TIME as millisecond-of-day, so a GenericRecordData round trip truncated every TIME(p > 3). It now writes nanosecond-of-day under a new tag and keeps reading the legacy one. PostgresFullTypesITCase expected 18:00:22.123456 from the snapshot phase, but the snapshot reads TIME columns with ResultSet#getObject, which yields a java.sql.Time carrying only milliseconds; the truncation is upstream of the runtime representation. The snapshot expectation now records that, and microsecond retention is asserted on the change stream, where the emitted value really carries microseconds. TransformE2eITCase carried the same LOCALTIME expectation that was corrected in FlinkPipelineTransformITCase: LOCALTIME and CURRENT_TIME are TIME(0), so only whole seconds belong to their declared type.
|
Thanks @yuxiqian and @leonardBang. I pushed 2e37478 which fixes the CI failures; all six red jobs came down to three causes, and only two of them were failures of this change. 1. 2. 3.
So the runtime change does preserve microseconds where the source actually delivers them. The snapshot expectation now states the millisecond resolution with a comment, and microsecond retention is asserted on the change stream instead of the vacuous snapshot comparison. Fixing the snapshot read looks like a separate On the memory-layout question: the serializer keeps the historical four-byte millisecond encoding and Verified locally on the JDK 11 target (built and run with JDK 17, Flink |
|
CI came back on 2e37478: 18 of 19 checks green, and every case that this PR actually broke now passes.
The two remaining failures look unrelated to TIME:
Both are in the 4-parallelism I cannot re-run it myself ( |
What is the purpose of this pull request?
Fixes FLINK-40594: the pipeline runtime stores TIME as millisecond-of-day, so every
TIME(p)withp > 3is silently truncated even thoughTimeTypeaccepts a precision of up to 9 andDebeziumEventDeserializationSchemaalready builds values throughfromMicroOfDay/fromNanoOfDay.Beyond the lost digits,
TimeData.equals,hashCodeandcompareTocomparedmillisOfDay, so twoTIME(6)values differing only in microseconds were indistinguishable and collapsed during deduplication, ordering and key comparison.Brief change log
TimeDatastores nanosecond-of-day and exposestoMicroOfDay()/toNanoOfDay(); equality, hash and comparison now use that value.toMillisOfDay()keeps its signature and truncating behaviour, so existing callers stay source and binary compatible.TimeDataSerializerbecomes precision-aware, following the existing shape ofTimestampDataSerializer/LocalZonedTimestampDataSerializerinstead of introducing a new pattern:precision <= 3keeps the historical four-byte millisecond encoding and resolves asisCompatibleAsIs, so existing state keeps working;precision > 3uses an eight-byte nanosecond-of-day encoding and resolves asisCompatibleAfterMigration, through a versionedTypeSerializerSnapshotthat still reads the legacy snapshot envelope and four-byte payload.BinaryRecordData,BinaryArrayData,AbstractBinaryWriter,BinaryArrayWriter), the converters andInternalSerializersthread the declared precision through.TIME(6)/TIME(9)transform expectation now asserts the retained fraction instead of21:48:25.123;LOCALTIMEat whole-second granularity, becauseLOCALTIMEandCURRENT_TIMEareTIME(0)and previously carried a fraction their declared type does not have.Verifying this change
This change added tests and can be verified as follows:
flink-cdc-common/src/test/java/org/apache/flink/cdc/common/data/TimeDataTest.javacovering nanosecond retention and the equality/ordering behaviour.flink-cdc-runtime/.../serializer/data/TimeDataSerializerTest.java: it becomes an abstract base with concreteTimeDataSerializer3Test/6Test/9Test, plus aTimeDataSerializerCompatibilityTestassertingisCompatibleAsIsfor the millisecond encoding,isCompatibleAfterMigrationfor the widened one, and successful reading of the legacy snapshot envelope and four-byte payload.flink-cdc-common/.../converter/InternalObjectConverterTest.javaandJavaObjectConverterTest.java.flink-cdc-composer/.../FlinkPipelineTransformITCase.javaandflink-cdc-pipeline-connector-postgres/.../PostgresFullTypesITCase.java. The Postgres case gains a direct microsecond-of-day assertion, because the existingTIME(6)expectation compared two equally truncated values and therefore passed vacuously.Local results on top of
9f23c0356, built and run on the JDK 11 baseline (openjdk 11.0.24; verified the emitted bytecode is class file major 55):flink-cdc-commonflink-cdc-runtimeflink-cdc-composer(FlinkPipelineTransformITCase)The same modules were also green on JDK 17.
Two notes for reviewers:
TimeDataSerializerTestdata includedTimeData.fromNanoOfDay(102400),204800and409600. Under millisecond storage all three collapsed to zero at construction, so the previous round-trip assertion passed while the intended distinct values never existed; the precision-parameterised tests replace it.TIME(3)on the legacy four-byte encoding,TIME(6)andTIME(9)on the widened one, and the snapshot compatibility matrix, so a regression in either encoding path fails a specific test rather than a shared one.Documentation
Update: follow-up commit for the CI failures
GenericRecordDataSerializerstill encoded TIME as millisecond-of-day, so aGenericRecordDataround trip kept truncatingTIME(p > 3). It now writes nanosecond-of-day under a new tag and still reads the legacy tag, covered byRecordDataSerializerTest#testGenericRecordDataKeepsSubMillisecondTime.Behaviour note for the change log: because the internal converters normalise a value to its declared precision,
LOCALTIMEandCURRENT_TIME— bothTIME(0)— now emit whole seconds instead of carrying a millisecond fraction their declared type does not have.FlinkPipelineTransformITCaseandTransformE2eITCaseassert that.PostgresFullTypesITCaseexpected18:00:22.123456from the snapshot phase. That is not reachable today:PostgresScanFetchTaskreads snapshot rows withResultSet#getObject, and pgjdbc materialisestimeasjava.sql.Time, which only carries milliseconds — the value is already truncated before it reaches the runtime. Measured on this branch: the fixture rowtime_6_c = 18:00:22.123456arrives from the snapshot asnanoOfDay = 64822123000000(18:00:22.123), while a row inserted afterwards withtime_6_c = 19:00:22.123456arrives from the change stream asnanoOfDay = 68422123456000(19:00:22.123456). The snapshot expectation now records the millisecond resolution, and microsecond retention is asserted on the change stream. The snapshot read itself looks like a separate[postgres]defect and is left for its own JIRA.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Anthropic)
The change was prepared, verified and described with AI assistance; I have reviewed every hunk, ran the suites listed above, and take responsibility for the correctness of the change.