tchivs commented on PR #4530:
URL: https://github.com/apache/flink-cdc/pull/4530#issuecomment-5615202229
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. `TransformE2eITCase#testTemporalFunctions` (the four Pipeline E2E jobs)
— my oversight.**
`verifyDataRecord` carried the same `LOCALTIME` expectation that I had
already corrected in `FlinkPipelineTransformITCase`, and I missed the E2E copy.
Since the converters normalise a value to its declared precision, and
`LOCALTIME` / `CURRENT_TIME` are `TIME(0)`, the pipeline now emits whole
seconds there; both tests assert that consistently. I have added this to the
change log in the description, because it is user-visible.
**2. `GenericRecordDataSerializer` — an incompleteness of the fix that CI
does not cover.**
It still encoded TIME as millisecond-of-day, so a `GenericRecordData` round
trip truncated `TIME(p > 3)` even after the binary path was fixed. It now
writes nanosecond-of-day under a new tag and keeps reading the legacy tag, so
old state stays readable.
`RecordDataSerializerTest#testGenericRecordDataKeepsSubMillisecondTime` fails
without that change.
**3.
`PostgresFullTypesITCase#testTimeTypesWithTemporalMode{Adaptive,MicroSeconds}`
(Pipeline Unit Tests 1.x and 2.x) — a pre-existing PostgreSQL snapshot defect
that this change made visible.**
The `18:00:22.123456` expectation is not reachable in the snapshot phase:
`PostgresScanFetchTask` reads snapshot rows with `ResultSet#getObject`, and
pgjdbc materialises `time` as `java.sql.Time`, which only carries milliseconds,
so the microseconds are gone before the value reaches the runtime. Under
millisecond storage both sides of that assertion were equally truncated, which
is why it used to pass. Measured on this branch:
| phase | `time_6_c` in PostgreSQL | emitted `nanoOfDay` |
|---|---|---|
| snapshot | `18:00:22.123456` | `64822123000000` (`18:00:22.123`) |
| change stream | `19:00:22.123456` | `68422123456000` (`19:00:22.123456`) |
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 `[postgres]` change to me (reading `Types.TIME` as `LocalTime`
rather than `java.sql.Time`); I would rather file that as its own JIRA than
widen this PR — happy to do so if you agree.
On the memory-layout question: the serializer keeps the historical four-byte
millisecond encoding and `isCompatibleAsIs` for `precision <= 3`, and only uses
the eight-byte nanosecond-of-day encoding for `precision > 3`, reached through
a versioned snapshot that still reads the legacy envelope. So `TIME(3)` state
and layout are unchanged, and only the precisions that Flink SQL has now
extended in apache/flink#26954 use the wider slot.
Verified locally on the JDK 11 target (built and run with JDK 17, Flink
`1.20.3` profile): `flink-cdc-common` and `flink-cdc-runtime` 993 tests green,
`PostgresFullTypesITCase` time-type and full-type cases green,
`FlinkPipelineTransformITCase#testDateAndTimeCastingFunctions` and
`#testTransformWithTemporalFunction` green, `flink-cdc-pipeline-e2e-tests`
compiles, Spotless passes on the touched modules. I do not have a local
environment to run the pipeline E2E suite itself, so I am relying on CI for
that job.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
