Repository navigation
Core, Flink: Share inserted-row trackers across equality key promotions in the dynamic sink - #20
abrarsher23 wants to merge 8 commits into
Conversation
A schema change that promotes an equality field (int to long, float to double, or a decimal's precision) makes a sink open a writer whose key type differs from the earlier writer's. The two could not share an inserted-row tracker, because keys of different classes never match in a StructLikeMap. A re-write of a key within one commit then went out as an equality delete at the same sequence number, and both rows stayed visible. InsertedRowTracker.canShareWith accepts such key types. A writer whose keys hold the same value classes as the tracker gets the tracker itself, so the common path does no extra work. A promoted writer gets a view that converts its keys to the tracker's key type. A key without an exact value in that type (a long outside the int range, or a double that is not a float) can only equal keys of other promoted writers, so it is kept in a second map in the widest type of each key field. Changes of optionality or position, and promotions next to a nested key field, are still not shared. acceptsKeyType keeps its strict meaning. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hema changes DynamicWriter kept a single tracker key type per table, the one of the first writer. After a key promotion every later writer was compared with that original key type. So within one checkpoint, inserting a key, promoting it, updating it, adding a column and updating it again left the last two writers on private trackers, although their key types were equal. A writer of a table now joins the first group of trackers whose key type it can share, also through a type promotion, and otherwise opens a new group that later writers with the same key type share. The WARN about another writer for the table is logged only when the new writer shares a tracker; a writer that cannot share logs both key types instead. Promoting the source column of an identity or truncate partition field turns its partition values from Integer to Long, or from Float to Double. Data files of one spec with both value types fail to commit together (PartitionData rejects the other class), with or without a shared tracker. Such partitions keep separate trackers and a WARN says why. Bucket partitions keep their values and share. Applied to flink/v2.0 and flink/v2.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…trackers Writers of one checkpoint that cannot share an inserted-row tracker, here because the key was made optional, keep both rows of a key that is re-written across them: the second writer's equality delete has the same sequence number as the first writer's data file. An upsert of the key in a later checkpoint writes an equality delete with a higher sequence number, which applies to both rows and leaves one. Applied to flink/v2.0 and flink/v2.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DynamicWriter no longer answers a writer it cannot share with by returning null; it opens another group of trackers. A resolver that returns null still gives the writer a private tracker, which is now documented and tested. Applied to flink/v2.0 and flink/v2.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
InsertedRowTracker.acceptsKeyType has had no caller outside tests since canShareWith replaced it, and the class is not released yet, so it goes. canShareWith now validates its argument, and the converting view notes that the state it inherits is never used. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…racker The message that writers share an inserted-row tracker was logged as soon as a writer joined a group, before its partitions were looked up. When a promoted partition source column gave the writer's partitions values of another type, the message promised position deletes and the next one took it back. It is now logged, at INFO, the first time a writer is handed a tracker that another writer created. The partition message now states the actual consequence: data files of one spec with both value types of a partition cannot be committed together. The message for a writer that cannot share also names a change of the identifier fields used as equality fields as a cause. A group keeps its first tracker to decide which key types can join, and the tests now check which partitions keep separate trackers and which writers were handed another writer's tracker. The three tests of the partition type change are renamed for what they check: the commit fails. Applied to flink/v2.0 and flink/v2.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6b9afec to
74a09ed
Compare
DynamicWriter kept a list of tracker groups per table and opened another group for a writer whose key type could not share the existing ones. A table now has one set of inserted-row trackers per checkpoint, keyed by the key type of its first writer. A writer whose key type the first tracker can share with, including a promoted one, uses them. Any other writer keeps a private tracker, as a resolver that returns null already provides, and the warning for it is logged once per schema ID per checkpoint. The test of a key optionality change now checks that the writers after the change keep private trackers. Applied to flink/v2.0 and flink/v2.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rayhondo
left a comment
There was a problem hiding this comment.
Reviewed at 5b4df4f. The shared-tracker design looks coherent, and the 207 new TestTaskEqualityDeltaWriter cases pass in the core-tests (17) CI log. Two items below, one question and one follow-up request. Separately: core-tests and kafka-connect-tests are red in CI (S3/MinIO tests and testcontainers launch errors, none in the files this PR touches). The description says the core suite passed locally, so it would help to confirm whether the same failures occur on the base branch.
A writer whose key type is a promotion of the shared tracker's stores its keys through PromotedKeyView, which converts them into a new holder. Add a test where that writer is handed one record changed in place for every row, with a string key field next to the promoted one and a key outside the int range, and check that the int-keyed writer and another promoted writer still find each key and retire its row with a position delete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Affirm-Skill: thor-control
|
On CI: the same failures are on the base branch. On the |
Change Management
This PR is the change-management record for two changes: this PR (released as
1.11.0-affirm-patch.4), and the downstream PR that bumps our CDC sink's Iceberg version to that release. Approvals for both are collected here.What this changes
#17 lets the writers of one table share an inserted-row tracker within a checkpoint. When a schema change opens a new writer mid-checkpoint, the new writer can then retire the old writer's row with a position delete, so the row isn't left visible twice. But #17 only shares when the key types match exactly. If a key column is widened (
int→long) in the same checkpoint, the row is still left visible twice.This PR closes that gap:
int↔long,float↔doubleand same-scale decimals. A promoted writer reads the first writer's tracker through a view that converts each key exactly (7L→7). A key that has no exact equivalent, such as alongabove theintrange, goes into a small side map that only other promoted writers check. Nointwriter can have written that key, so nothing is missed.The changes are in
core/.../io/BaseTaskWriter.java(canShareWith, the converting view) andflink/v2.0andv2.1(DynamicWriter, plus small changes inBaseDeltaTaskWriterandRowDataTaskWriterFactory). The two Flink versions get identical changes, andflink/v1.20is untouched.Not fixed here. Each of these happens within one checkpoint:
identity/truncatepartition column. The commit fails because of a separate upstream bug (Core: Committing data files fails after promoting the source column of an identity partition field apache/iceberg#18355), so no duplicate is left. The behaviour is pinned by tests.The writers log a WARN for these. Any duplicate left behind is removed by the key's next upsert or delete.
Risk
Low. Checkout impact: none. This is a library change.
-0.0, keys outside theintrange, null keys, two promoted fields, and a reused mutable row.canShareWithis checked againstTypeUtil.isPromotionAllowedfor all 289 type pairs..3.canShareWithreplaces Core, Flink: Share the inserted-row tracker across DynamicIcebergSink writers of one table #17'sacceptsKeyType, which had no callers.revapireports 0 differences.Release and rollback
version.txt,AFFIRM_VERSIONS.md), tag1.11.0-affirm-patch.4and publish.Rollback: pin back to
1.11.0-affirm-patch.3and redeploy. The deletes this version writes are standard Iceberg files that.3reads, so nothing needs migrating.Observability
iceberg-version1.11.0-affirm-patch.4.… share an inserted-row tracker …: sharing worked.Not sharing inserted-row tracker …: a case listed above. Investigate if it appears after a promotion.Partition … in other types …: the commit will fail (Core: Committing data files fails after promoting the source column of an identity partition field apache/iceberg#18355).Testing
Unit tests (JDK 21). The full
core,data,flink-2.0andflink-2.1suites pass, with 0 failures. Those runs were at5b4df4ffa. The head,0f0e6b9c0, adds one test and no production code. At the head,TestTaskEqualityDeltaWriterpasses 216 of 216.TestTaskEqualityDeltaWriterTestDeltaTaskWriter(each Flink module)TestDynamicWriter(each Flink module)intrange, a key becoming optional, partition promotion failing to commit, a later upsert removing a leftover duplicateCoverage. JaCoCo shows 166 of 166 changed lines and 104 of 105 branches exercised. The one uncovered branch only skips repeating a WARN.
Mutation checks. Breaking the change makes tests fail in each case:
TestDynamicWriterTestTaskEqualityDeltaWriterTestTaskEqualityDeltaWriterTestTaskEqualityDeltaWriterStage testing checklist. A dev build of
5b4df4ffaran in our staging environment against a live CDC stream, with anINT-keyed test table. Each session landed in one checkpoint:intkeys (one negative), widenidtoBIGINT, update, delete, insert keys above theintrange, add a column, update again. All in one commit: every writer shared, and every key's count matched the source.Only
int→longon format v2 was exercised in stage. The other promotions are covered by unit tests only. No benchmark was run.CI.
core-testsandkafka-connect-testsfail the same way on the base branch: the MinIO and Docker setups can't start. Neither failure is in code this PR touches. The Flink, Spark and Hive checks pass.🤖 Generated with Claude Code