Repository navigation
Conversation
|
@JingsongLi Given the amount of changes in the other PR, I feel like this PR is a better approach. I have tried clearly mentioning that the fields shouldn't be used as aggregated values nor as filtering keys. I would love to get your thoughts on this. |
29ba5e3 to
20582fd
Compare
JingsongLi
left a comment
There was a problem hiding this comment.
The event-timestamp preservation use case has end-to-end value, but the production review found two correctness gaps below. The ordinary deduplicate update/delete path was also checked through real table commits and persisted changelog reads.
20582fd to
ba2edd9
Compare
Both of the comments have been addressed. |
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS.
The earlier partial-update and rename concerns are addressed: validation now restricts the option to deduplicate + lookup, and persisted rename/update/delete tests cover rewritten option references. The documented external sink restrictions are exercised by the Flink integration test.
A new Spark postpone-write interaction remains below. I reproduced it with a real filesystem table and the actual SparkPostponeStagedCommitter constructor using this head's Core/API classes. The unchanged Spark coordinator source is identical to the compiled class used by the probe. Removing the preserve option, clearing it on the runtime copy, or overlaying only the exact baseline SchemaValidation all make the same coordinator initialize successfully.
Verification: normal JDK 8 Maven verify passed 120 focused tests (116 Core, 4 real Flink external-upsert-sink cases), and current CI is green. The additional integration probe fails on this head as described inline.
| "'%s' is only supported with 'merge-engine' = 'deduplicate'.", | ||
| CHANGELOG_PRODUCER_PRESERVE_FIELD_ON_RETRACT.key()); | ||
| checkArgument( | ||
| changelogProducer == ChangelogProducer.LOOKUP, |
There was a problem hiding this comment.
[P2] Clear preserved fields from internal rescale copies that disable changelog
A valid table with bucket=-2, postpone.batch-write-fixed-bucket=true, changelog-producer=lookup and preserve-field-on-retract=event_ts fails Spark batch-write coordinator initialization. SparkPostponeStagedCommitter eagerly builds rescaleWriteBuilder by copying the table with changelog-producer=none, retaining the new preserve option; AbstractFileStoreTable.copy runs this validation and throws before commit. The real table creation succeeds, and the actual coordinator constructor succeeds without the preserve option or with the exact baseline validation. Keep the CREATE/ALTER restriction, but clear the preserve option on internal copies that deliberately disable changelog production, and cover a Spark postpone fixed-bucket write with preservation enabled.
There was a problem hiding this comment.
The internal rescale copy in SparkPostponeStagedCommitter now has changelog-producer.preserve-field-on-retract removed. That copy forces changelog-producer=none and writes no changelog, so the option has no effect there. The CREATE/ALTER restriction is unchanged, and the table itself keeps the option. This is the only place that forces changelog-producer=none on a table copy, so no other internal copies need the same change.
I added a Spark test to PostponeBucketTableTest. It uses a postpone fixed-bucket table with changelog-producer=lookup and preserve-field-on-retract enabled. The test writes enough rows to trigger a load-factor rescale from 1 to 16 buckets. It then upserts an existing key and checks the table contents and options. Without the fix, the test fails with the reported validation error from the SparkPostponeStagedCommitter constructor. With the fix, the whole suite passes on Spark 3.
Validate the option during table creation and alteration. Add regression coverage for rejected configurations and successful deduplicate operations, and document the supported combination and failure behavior.
Rewrite preservation field names on column rename and reject schemas that reference missing preservation fields. Cover persisted update/delete changelogs after renames, nested renames, and rejection of dropping columns referenced by preservation or sequence options. Document the rename and DROP behavior.
Test preserved retract fields after schema changes and with complex types in Parquet and Avro. Verify nested paths are rejected on CREATE/ALTER and failed ALTER operations leave the schema unchanged. Cover Flink external upsert sink materialization and document the consuming-job setting. Share fixtures and remove duplicate validation tests.
The rescale copy disables changelog, so drop the option to pass validation.
ba2edd9 to
67946aa
Compare
Purpose
Tests
Manual verification