Repository navigation
Conversation
7681a1c to
2bd8780
Compare
1f3861a to
c7a4fe2
Compare
|
@JingsongLi Can I get a review on this? |
|
Thanks for the feature. I reviewed the write and read paths end to end and found two blocking issues plus a consistency concern. 1. [P1] The option is applied for every changelog producer, but only the
|
c7a4fe2 to
5ec5070
Compare
@JingsongLi Let me explain this in detail below. Yeah I agree the changes are more complex that's why I initially went with minimal change approach. Use case and motivationPaimon’s lookup changelog producer reconstructs UPDATE_BEFORE and DELETE records by looking up the previous table state. As a result, regular columns in those records describe the old row, not necessarily the event that caused the change. For example: UPDATE_BEFORE: event_ts = 50, __internal__event_ts = 100 Downstream CDC consumers often need the event timestamp or source sequence to apply changes deterministically. Concurrent Sinks to a Single Cassandra tableMultiple sources can fan into the same Cassandra table while being processed by independent jobs. A common example is migrating from Kafka to Paimon: Kafka retains the original event payload for retract events, whereas Paimon’s lookup-generated changelog may represent UPDATE_BEFORE and DELETE records using the previous row image. This makes it difficult to replace a Kafka-based job with a Paimon-based job without changing downstream conflict-resolution behavior. Exposing selected fields as changelog metadata preserves the event-level values and gives the sink a consistent, globally comparable event version for conditional writes and tombstones, preventing stale updates from overwriting newer changes or resurrecting deleted records. This behavior allows creating Sinks from multiple Paimon tables merging into a single Cassandra column. |
JingsongLi
left a comment
There was a problem hiding this comment.
The Cassandra WRITETIME use case gives this feature concrete end-to-end value. The earlier lookup-only validation, physical before-image preservation, and Spark write-side filtering are present on this head. I found one remaining schema issue before this should ship:
P2 — Allocate synthetic metadata field IDs above nested field IDs (ChangelogEventMetadata.java:117). extraValueFields takes the maximum top-level field ID. A valid table can have id (0), payload ROW<nested INT> (top-level 1, nested field ID 3), and event_ts (2). Exposing event_ts assigns the new __internal__event_ts field ID 3, duplicating payload.nested. I added this case to ChangelogEventMetadataTest in an isolated checkout: appendMetadataFields(...).collectFieldIds(...) fails with Broken schema, field id 3 is duplicated. The generated row type is used by the changelog writer and both read integrations, so its IDs need to remain valid for nested schemas and field-ID based mapping. Please derive the next ID from the full recursive field-ID set (for example, RowType.currentHighestFieldId(valueType.getFields()) + 1) and test a nested table through a changelog read.
Validation: the added core tests passed 16/16 and Spark 3 PaimonCDCSourceTest passed 6/6 on 5ec5070; the strengthened nested-ID test fails as above. I attempted the 3 Flink integration tests locally, but all stopped on the local Avro classpath mismatch DataFileWriter.setEncoder before exercising this change. The PR's Flink and other CI jobs are green on this head.
5ec5070 to
b0fd1ea
Compare
Thanks a lot for pointing it out. I have made that fix. |
|
Re-reviewed head Local JDK 8 core suites passed: |
|
I took another look at head 1. [P1] Event metadata breaks retract semantics when used by normal SQL operators.
2. [P1] Changing The prefix is not marked immutable, so it can be changed after changelog files exist. The write path stored the old prefixed field name, but There is also a source/sink mismatch in the documentation example: Finally, the option names obscure the contract. |
e201136 to
c287adb
Compare
|
@JingsongLi Thanks for the review.
|
c287adb to
2b5f23f
Compare
|
Reviewed current head [P1] Dropping an ordinary column silently replaces event metadata with that column's historical value. [P2] Avro historical metadata loses the source-type conversion after a legal ALTER. For an Avro lookup table exposing [P2] The documented pass-through support boundary must include automatic sink materialization. A plain Validation: 147 Core tests passed on JDK 8; all four existing Flink integration cases passed on Flink 1.20.1 and 2.2.0 after aligning the local Avro runtime; Spark 3.5.8 CDC suite passed 6/6. Spark 4.1.2 CDC suite passed 6/6 with normal Maven checks. Independent actual persisted tests passed scalar rename, ordinary-column addition, metadata-field addition/reordering, and nine ROW/ARRAY/MAP × Parquet/Avro/ORC full-row and metadata-only projections. Exact-head CI is green, but the new schema-evolution probes above fail on this head and pass with isolated controls. No shared source changes were retained. |
2b5f23f to
3ee0664
Compare
|
@JingsongLi Thanks for the detailed reproductions. All three are addressed as of 36ba0fb.
One behavior change: renaming or dropping a column listed in changelog-producer.event-metadata-fields is now rejected, because it would silently change metadata names that downstream jobs reference. Remove the column from the option first. On Spark scope: I'm happy to split the Spark changes into a follow-up PR if you'd still prefer that. |
JingsongLi
left a comment
There was a problem hiding this comment.
Re-reviewed this head for production. The Cassandra WRITETIME use case has end-to-end value, and the prior dropped-ID aliasing, historical type conversion, and documented sink-materialization fixes are present. Local JDK 8 package verification without fast-build passed 165 core, 6 Flink SQL integration, and 6 Spark CDC tests. Three remaining issues are below. Added real-file probes reproduce nested projection corruption on both Parquet and Avro, and a real Flink temporal join fails while its physical-only control passes. The exact-head E2E CI also fails during POM parsing on the duplicate Avro dependency. Spark 4.1 CI additionally has a concurrent data-evolution compaction snapshot-expiration failure; I have not established that it is caused by this change.
| if (readFieldNames.contains(metadataName) | ||
| && !readFieldNames.contains(preserveColumn) | ||
| && schema.logicalRowType().containsField(preserveColumn)) { | ||
| dependencies.add(schema.logicalRowType().getField(preserveColumn)); |
There was a problem hiding this comment.
[P2] Project nested fallback values to the requested metadata shape
Adding the whole source field here is unsafe when metadata is nested-pruned. With payload ROW<a INT,b INT> and event-metadata-fields=payload, requesting only __internal__payload.b gives a metadata type ROW<b INT>, but FallbackMappingRow.getRow returns the full physical ROW<a,b> unchanged. A real table updated to (a=33,b=44) returns 33 instead of 44 for that metadata child. I reproduced this with Parquet and Avro on the raw path and with the merge reader forced via forceKeepDelete(). Spark nested pruning produces this requested type. MergeFileSplitRead.adjustReadType has the same dependency logic. Please combine physical/metadata dependency projections and project each fallback value to its requested nested shape, including when the physical source and metadata request different children.
| throw new UnsupportedOperationException( | ||
| "Unsupported Paimon metadata column: " + metadataKey); | ||
| } | ||
| this.metadataKeys = metadataKeys; |
There was a problem hiding this comment.
[P2] Apply declared metadata to temporal lookup results too
The scan path consumes these metadata keys, but the inherited getLookupRuntimeProvider still passes the physical FileStoreTable and physical projection to FileStoreLookupFunction. Flink accepts metadata on a lookup table and expects the produced metadata column in each returned row. Reproduction: declare (id INT PRIMARY KEY NOT ENFORCED, event_ts BIGINT, writetime BIGINT METADATA FROM '__internal__event_ts' VIRTUAL), preserve event_ts, insert (1,50), and run a processing-time temporal join selecting d.event_ts,d.writetime. The actual job fails with ArrayIndexOutOfBoundsException: 2 in GenericRow.isNullAt through the generated lookup collector because only the two physical fields are returned. The same join selecting only d.event_ts passes. Please supply the requested metadata in the lookup runtime (or explicitly reject the unsupported lookup plan before launching a job) and add this integration case.
Preserve correct before-images in changelog retraction records and append event values as configurable metadata columns, accessible via Flink SupportsReadingMetadata and Spark table schema.
36ba0fb to
5a99846
Compare
|
Thanks for the suggestions. I have tried addressing the feedback in 5a99846 (also rebased on master)
While testing, I also fixed I have also raised a different PR that preserves the field with additional configuration option. That's much more simpler, but it comes at the expense of breaking CDC contract (which should be ok IMO as long as we clearly document it). |
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS. The previous nested-ROW, temporal-lookup rejection and duplicate-dependency fixes are present. Regular JDK 8 verification passed 186 Core metadata/schema/merge tests, 7 real Flink SQL integration tests and 6 Spark CDC tests. The existing IncrementalTableTest has 14 passes and one reproducible regression matching exact-head JDK 8/11 CI. Additional real-file and Spark SQL probes, with controls and independent challenges, expose four issues below. The MAP probe uses the exact current Spark 3.5 engine rule because the common UT classpath otherwise loads its no-op placeholder.
| this.readType = readType; | ||
| // Let the merge read add its internal dependencies, such as sequence fields and the | ||
| // source fields of event metadata, so the diff is computed on a consistent value layout. | ||
| mergeRead.withReadType(readType); |
There was a problem hiding this comment.
[P1] Compute diff membership before applying the requested projection
Pushing the requested columns into both merge readers makes DiffMerger compare only those columns. A key whose unselected value changed is therefore dropped as unchanged. This also affects ordinary PK tables without event metadata: after committing (id=1,v=10), updating to v=20 and full-compacting, the same incremental diff returns [1,20] with the full schema but no rows when projected to id; the merge-base implementation returns [1] for the latter. The existing IncrementalTableTest.testTagIncremental also fails locally and in both exact-head JDK 8/11 CI jobs (expected [2,6], actual [6]). Please retain all values needed to decide whether the row changed and project only after computing the diff, while adding metadata dependencies separately.
| return null; | ||
| } | ||
| KeyValue kv = serializer.fromRow(internalRow).setLevel(level); | ||
| if (metadataFallbackRow != null) { |
There was a problem hiding this comment.
[P2] Populate ordinary-file fallback metadata after merging values
This copies metadata from each input file before the merge engine runs. For an aggregation table, the synthetic field does not inherit its source's aggregation function, so it is merged using the default last_non_null_value. With legal lookup/event-metadata=amount, fields.amount.aggregate-function=sum and write-only=true, two commits of amount 10 and 20 return an ordinary snapshot row amount=30, __internal__amount=20. Full compaction changes only the file layout and then the same row reads 30,30; the last_non_null_value control remains 20,20 throughout. Independent real-file runs also reproduce it with metadata-only projection. Please derive fallback values from the final merged physical row (or preserve the source merge semantics), while retaining the actual event metadata stored in changelog files.
| // pushdown reads a ROW. Such a type cannot be merged with the metadata field's type. | ||
| if (left.getTypeRoot() != fullType.getTypeRoot() | ||
| || right.getTypeRoot() != fullType.getTypeRoot()) { | ||
| return left; |
There was a problem hiding this comment.
[P2] Retain a full MAP dependency beside selected-key physical reads
Returning the physical selected-key ROW here leaves a full MAP metadata field backed by that ROW. With a shared-shredding payload MAP and event-metadata-fields=payload, real Spark 3.5 SQL SELECT payload['a'], __internal__payload FROM T pushes PushedMapSelectedKeys:[payload=[a]] and then fails with ClassCastException (CastedRowColumnVector to MapColumnVector) through FallbackMappingRow.getMap. Selecting the full physical map plus metadata, or only payload['a'], both succeeds. I reproduced this on real compacted Parquet files using the exact current Spark 3.5 optimizer implementation, in addition to a Core read-type probe. Please retain an independent full source-map dependency or prevent this selected-key pruning when full metadata needs that source, rather than reusing the incompatible ROW fallback.
| val baseRowType = table.rowType() | ||
| val extendedRowType = table match { | ||
| case fst: FileStoreTable if !coreOptions.changelogEventMetadataFields().isEmpty => | ||
| ChangelogEventMetadataTable.computeExtendedRowType(fst, baseRowType) |
There was a problem hiding this comment.
[P2] Keep synthetic metadata out of MERGE target assignment alignment
Exposing these read-only fields through schema also adds them to DataSourceV2Relation.output, but the MERGE resolver still expands and aligns assignments against that full output. Under the default merge-schema=false, an ordinary physical-only source now fails both WHEN MATCHED THEN UPDATE SET * and WHEN NOT MATCHED THEN INSERT * with "source is missing" __internal__event_ts. Actual Spark SQL controls show that explicit physical assignments succeed on the same metadata-enabled table, and both stars succeed on the same lookup table without this option. The new INSERT/V2 write filtering happens later and does not cover MERGE analysis. Please align MERGE stars against physical target fields while continuing to expose metadata for reads.
Preserve correct before-images in changelog retraction records and append event values as configurable metadata columns, accessible via Flink SupportsReadingMetadata and Spark table schema.
Purpose
<prefix><field>metadata columnssuch as Cassandra
WRITETIME.Tests
paimon-docs verifypasses on Java 8 and Java 11.