Repository navigation
Conversation
|
@bryanck I'd appreciate your review, since you raised the read-performance concern that stopped #14797. This PR is part 1 of 4. It adds Nothing calls the converter yet, so there is no behavior change here. If the direction looks wrong to you, I'd rather hear it on this PR than after the writer lands. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
still alive |
|
@laskoviymishka Could you take a look at this PR, or point me to the right reviewer? Some context: #14797 (CDC/upsert for the sink) was blocked because it committed equality deletes, and it was closed as stale. Since then, the v4 spec forbids writing new equality deletes (#17783), and Flink converts them to deletion vectors with This series follows the same direction for Kafka Connect. The coordinator resolves the equality deletes of a commit to row positions and commits deletion vectors instead, so the table never contains an equality delete. This PR (part 1 of 4) only adds the converter class. Nothing calls it yet, so there is no behavior change. Before I continue with the coordinator wiring, is this direction acceptable for the sink, or should I start a Thanks! |
EqualityDeleteConverter takes the equality delete files of one commit and returns deletion vectors for the rows they delete in a base snapshot, so a writer can commit vectors instead of the equality deletes. It groups the files by equality field ids, partition spec and partition, loads their keys, and asks a KeyPositionResolver for the data file positions of those keys. The default ScanKeyPositionResolver plans the base snapshot with a filter that pins every partition field of the delete's partition through its transform and narrows the keys with IN lists or, beyond a budget sized from the table's data file count, with ranges of the leading key column (KeyFilters). It then reads only the key columns and _pos of the candidate files in parallel, skipping positions an existing deletion vector already covers. Vectors are written with BaseDVFileWriter, which merges existing vectors and reports them as rewritten. The result also carries the base snapshot id and a conflict detection filter for the RowDelta that commits the vectors. Nothing in the connector uses the converter yet; the coordinator wiring and the CDC writer follow in separate changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A partitioned equality delete applies only to data files with the same spec ID and partition values. The partition filter matched rows through transforms, so after partition evolution the converter also deleted rows of data files written with other specs. Check the spec and partition of every candidate, and document that the commit must validate the data files referenced by the deletion vectors. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
laskoviymishka
left a comment
There was a problem hiding this comment.
Thanks for the patience on this one, and for ordering the series so no merged state writes equality deletes. On your direction question: converting in the coordinator follows where the project already went with #17783 and Flink's ConvertEqualityDeletes (#17142), so it looks right to me for the sink. I'd still like @bryanck to confirm, since his read-cost concern is what stopped #14797.
For this part, the one thing I'd want before merge is the empty-branch path: it returns an alwaysFalse conflict filter, so a concurrent first commit can keep rows the equality delete would have removed. The version check and the test gaps are inline.
Deletion vectors require format version 3. On an older table the converter scanned the data files and wrote Puffin files that the commit then rejected, leaving the files behind. Reject such a table when the converter is created. Format versions only go up, so a converter created for a version 3 table stays valid. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only a package-private constructor accepts a resolver, so code outside the package cannot provide one yet. Make the interface package-private and drop the Javadoc that described plugging in another resolver. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A candidate data file with a format version 2 position delete file failed the conversion before any key was matched, so one legacy delete file blocked every commit touching its partition. A deletion vector replaces all position deletes of its data file, so the resolver now returns the existing deletes of a matched file, whether a deletion vector or position delete files, and the new vector merges them. BaseDVFileWriter reports only file-scoped deletes as rewritten, so a partition-scoped file stays in the table for its other data files. Existing deletes are read only for files that have a matching key. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The converter merges the position deletes a data file already has only when the resolver returns them. A resolver that returned null for a file with a deletion vector would leave the file with two vectors, and reads would fail. For a file with position delete files left from format version 2, readers would ignore those files once the new vector exists, and the rows they deleted would come back. State in FileMatches.existingDeletes that null means the file has no position deletes. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The count was an instance field reset by every convert call, so two calls on one converter overwrote each other's count. Return it on Result instead, which also removes the test-only accessor. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Without a snapshot the converter returned an alwaysFalse conflict filter, so the commit's validation passed whatever was committed concurrently. If another writer made the first commit to the branch, its rows had a lower sequence number than ours and the equality delete would have removed them, but with the conversion they survived. Load the keys and build the conflict filter also without a snapshot, and document that a null base snapshot still requires validating the added data files against the filter, without a starting snapshot. The converter also treated a branch that does not exist yet as empty, but a commit to that branch builds on the current snapshot of main, so the rows of main that the deletes match survived in the new branch. Resolve such a branch against the current snapshot of main. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The partition tests counted the files left after the spec and partition check, and their key filters already pruned the other partitions: the identity test keyed on the partition column, and IN lists of ids project through bucket(4, id). Both would pass with a partition filter of alwaysTrue. Count the files the plan returns before the check, and change the tests so that only the partition filter can prune: the identity test keys on the id alone, and the bucket test deletes more keys than IN lists may hold, so ids are matched with ranges that bucket(4, id) cannot project. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every converter test stayed within the IN list limit, so value ranges were only checked against file metrics in TestKeyFilters. Delete two clusters of keys past the limit, which planning matches with ranges, and check the planned files and the surviving rows. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test called KeyFilters.splitIntoRanges directly, so it belongs with the other KeyFilters tests. Split it into one test per case. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The key filter sized itself from a comparison budget derived from the table's data file count and split value ranges at the largest gaps. Its constants are hard to review without a benchmark to rerun, and the converter has no caller yet. Match up to 1,000 keys with IN lists of at most 200 values, and more keys with the minimum and maximum of the leading key column. The row-level match keeps the result exact either way. A size that adapts to the table can follow together with a benchmark once the coordinator calls the converter. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
21ba9f7 to
286bc46
Compare
|
@laskoviymishka The last commit, 286bc46, drops the comparison budget and the range splitting, so it is mostly deletions in While fixing the empty-branch path, I also found that a branch that does not exist yet was treated as empty, although a commit to it builds on the current snapshot of main. 92ee01a handles both cases. |
laskoviymishka
left a comment
There was a problem hiding this comment.
The round-1 threads all landed, thanks. This is a second pass; I'll dive deeper once this round is fixed. Code-wise this is close, but the direction should settle before merge.
That direction is bigger than this stack: v4 drops equality deletes (#17783), and this should eventually be how Kafka Connect does CDC. Could you open the dev@ [DISCUSS] thread you offered? Flink's ConvertEqualityDeletes (#15996, #17142) converts after commit as a stateful maintenance task; this converts before commit with a stateless scan, worth aligning there. @bryanck, @mxm, could you weigh in?
One proposal for that thread: make the CDC path v4-only. On v3, equality deletes are still allowed, and converting them costs a scan on every commit that gets expensive for scattered keys (see the KeyFilters thread). The conversion is worth that cost where equality deletes aren't an option, which is v4. A v4 gate would also let part 3 take keys from the primary-key constraint (#17822) instead of identifier-field-ids, and the format check here would move from >= 3 to >= 4. v3 tables would stay append-only, and CDC would wait until v4 is usable end to end. wdyt?
@pvary, could you check the resolver seam in the existingDeletes() thread?
| * committed, so the commit should pass their referenced data files to {@link | ||
| * org.apache.iceberg.RowDelta#validateDataFilesExist(Iterable)}. | ||
| */ | ||
| public List<DeleteFile> dvFiles() { |
There was a problem hiding this comment.
Could the tests commit a Result through a small helper, covering a concurrent compaction of a DV's data file and a concurrent in-key append on a non-null base? Conflict validation only covers append and overwrite, so only validateDataFilesExist catches the compaction, and the commit() helper never calls it.
There was a problem hiding this comment.
Added in b02bb42.
The tests now commit a Result through a new helper, commit(Table, Result, DataFile...). It applies the validations the Result Javadoc asks the commit for:
conflictDetectionFilter(result.conflictFilter())withvalidateNoConflictingDataFiles()andvalidateNoConflictingDeleteFiles()validateDataFilesExist(...)for the data files the new deletion vectors point tovalidateFromSnapshot(result.baseSnapshotId())when the result has a base snapshot
testConflictFilterCoversKeysAppendedAfterBase covers the concurrent in-key append on a non-null base: another writer appends a row with a deleted key after the conversion, and the commit fails with a ValidationException. testConflictFilterCoversKeysWithoutSnapshot is the same case without a base snapshot, and now uses the same helper.
The other tests keep the old helper. Nothing commits concurrently there, so these validations cannot fail.
As you suggested, the compaction case waits for part 2, where it can be tested against the coordinator's real RowDelta.
| * returning null for a file that has some leaves it with two vectors or brings back rows the | ||
| * older deletes removed. | ||
| */ | ||
| PositionDeleteIndex existingDeletes(); |
There was a problem hiding this comment.
@pvary, does this obligation fit the index in #16961? Every resolver has to return the file's existing DV and v2 position deletes, and once the new DV exists, a null for v2 files silently brings their deleted rows back. The draft spec doesn't define whether an entry points back to its row, so an index-backed resolver may have no positions to give.
| * hold no deleted key. | ||
| */ | ||
| static Expression keyFilter(Schema keySchema, StructLikeSet keys) { | ||
| if (keys.size() <= MAX_LISTED_KEYS) { |
There was a problem hiding this comment.
Above 1,000 keys this falls back to one min/max range on the first key column. For scattered keys (UUIDs, hashes, updates across history) that means every commit reads the key columns of every file in the partition, or the whole table if unpartitioned, with no cap on planned files. The 1,200-key test already plans all 30 of 30 files. The same range is the conflict filter, so a retry repeats the scan. What envelope (keys per commit, files per partition, key distribution) should the part-2 benchmark cover, and what happens when plannedDataFiles() goes past it?
There was a problem hiding this comment.
What envelope (keys per commit, files per partition, key distribution) should the part-2 benchmark cover?
- Keys per commit: from a few keys to well beyond the IN-list limit, so that both the IN lists and the min/max range are covered.
- Data files per partition: from small partitions to ones where reading the key columns of every file takes longer than the commit interval.
- Key distribution, in two parts:
- Which rows a commit changes: recent rows, rows scattered across history, and new keys, since upsert mode also deletes the key of every insert.
- How keys lie in data files: increasing IDs, UUIDs after a sorting compaction, and unsorted UUIDs.
- Layout: unpartitioned, identity-partitioned, and bucketed by the key, each with and without bloom filters on the key columns.
For each case, the benchmark measures conversion time against the commit interval, planned data files, and bytes and manifests read, and compares read latency with the same commits written as equality deletes. The envelope is where the conversion still finishes within the commit interval.
What happens when
plannedDataFiles()goes past it?
The conversion still runs. Going past the envelope costs commit time, not correctness. In part 2, the coordinator will:
- Report the planned data files and the conversion time of each commit.
- Warn when a conversion takes longer than the commit interval.
- On a conflict, re-run the conversion (and its scan) against the new snapshot, and fail the commit if it keeps conflicting.
The benchmark results also say how to stay inside the envelope or widen it:
- Layouts that stay inside it: sorting or bucketing by the key, bloom filters, compaction. These go into the docs.
- The first pruning change to make: several ranges instead of one min/max range, one manifest pass for all partition groups, or skipping row groups with bloom filters.
- Beyond that, the resolver seam is where an index (Spec: Secondary index specification #16961) can take over.
As you asked, I'll add a short version of this plan to the description.
|
I dug into the v3 part a bit more after the review, and I think I was too strict there. There is a good reason to support this on v3 too, not only v4. Ryan’s point in the thread behind #17783 is that DV-based writes on v3 give writers a clean upgrade path before the table itself moves to v4 (thread). Flink’s converter also targets v3. v4 can come later once the converter writes colocated DVs. The cost is on the write side. An equality-delete writer just writes keys. Here we have to read key columns from candidate files before commit. But after that, readers only need the DV instead of doing the equality-delete join on every read, and clients without equality-delete support can still read the table. For Kafka Connect I think that is probably the right trade. The cases I’d want to understand better in the dev@ thread are mostly the bad key distributions:
For part 2, I’d benchmark conversion time vs commit interval, plus bytes/manifests read. Recent/scattered/random keys are probably enough to show where this starts getting expensive. I’d also compare that with read latency after accumulating equality deletes for a while. Bloom filters on the key columns are worth trying too. For this part, I think I only need the concurrent in-key append test on a non-null base, plus a short benchmark plan in the description. The compaction case can wait for part 2 when there is a real The >1,000-key fallback is what I asked for in round 1, so I’m fine with it here. |
The deletion vectors of a result only cover rows of the snapshot they were resolved against. When another writer commits a row with a deleted key after that snapshot, the equality delete would remove the row but the deletion vectors miss it, so the commit has to fail. Commit results through a helper that applies the validations the result asks for, and check that such an append fails the commit on an existing base snapshot. The test without a base snapshot now uses the same helper. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@laskoviymishka Thanks for the second pass and the follow-up. Agreed on v3: CDC writes target v3 now, and v4 follows once the converter writes colocated DVs. I started the dev@ discussion on the direction: https://lists.apache.org/thread/gmkj55oc0k1k72gobsylgfc9ngk9lyxn |
|
@koodin9 thread went to spam for me, but i did reply onto this, i think for me now main question is from strategic perspective - do we want to disrupt current status quo or no. |
|
@laskoviymishka Thanks, I replied on the thread. @bryanck @rdblue, the dev@ thread is now on whether the sink should support CDC. You were part of the original decision to leave it out because of equality deletes, so your view would help. In short: the coordinator converts each commit's equality deletes into deletion vectors before committing, so the table never gets an equality delete. CDC would be opt-in, and append-only stays the default. |
The coordinator has to convert a commit's equality deletes within the commit interval, so the conversion time should be known before the converter is merged. Measure one conversion against an unpartitioned table, where the key filter alone decides which data files are read. The parameters vary the table size, how keys lie in the data files (increasing IDs, unsorted and sorted UUID strings), which rows change (recent, scattered, and new keys as from upsert), and the number of keys around the IN-list cap. The counters report the planned data files, the deletion vectors, and the deleted rows. The tables are kept in a local Hadoop catalog by default. Reading from local disk costs less per file than an object store, so another catalog can be given as a properties file in BENCHMARK_CATALOG_PROPERTIES. One for the REST catalog and the RustFS object store of the integration test compose file is included, with the AWS modules that S3FileIO needs at runtime. Generated-by: Claude Code Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I added the benchmark to this PR (2c593a4), since Péter asked on the dev@ thread to measure the conversion before anything is merged. Here are the first results. Setup: a 14-core Mac, an unpartitioned v3 table with 1,000 rows per data file, and 1,000 deleted keys per commit. The table is in RustFS behind toxiproxy, which delays every S3 response by a fixed latency. The worker pool has 14 threads unless noted. Each time is one conversion. How to run it, including the toxiproxy setup, is in the PR description.
Next, I'd like to add partitioned layouts and larger data files, and run compaction alongside repeated conversion commits. |
|
What are the expectation of the time for the commits? Is a worst case full read acceptable? |
Well below the commit interval. Our target is about 3 minutes from a change to its visibility. With a commit interval of 60 to 90 s, the conversion should take at most about 80 s, or about 40 s so that one retry still fits.
Not on large tables, I think. When the changes are spread over the key space, every commit reads the key column of the whole table. Our two largest CDC tables are unpartitioned, with about 150 to 160 data files and 1.7 and 2.4 billion rows. Amoro already reads all of these files after each commit to turn the equality deletes into position deletes. It runs one task per data file in parallel, and still takes 14 s and 43 s. I haven't measured the conversion on our storage yet, but doing that in one coordinator on every commit is too expensive. So I'd document in part 2 that the conversion suits tables where the key filter or the partition spec prunes most data files, such as increasing keys with changes on recent rows. Tables with changes spread over many rows need an index behind |
|
Thanks for the answers @koodin9! These were my main questions. I leave the decision to people with more Kafka knowledge to decide how to proceed. |
This is part 1 of 4 in a series that adds CDC (upsert/delete) writes to the Kafka Connect sink
without committing equality deletes to the table.
Why
#14797 was blocked because the equality deletes committed by a streaming sink slow down every
reader until compaction runs. The v4 spec now forbids writing new equality deletes (#17783).
The sink's coordinator sees every delete file of a commit before the commit happens. So it can
resolve the deleted keys to row positions and commit deletion vectors (DVs) instead. This is the
same idea as Flink's
ConvertEqualityDeletes(#15996, #17142), but it needs no staging branch,no index state and no separate maintenance job.
flowchart LR W["Worker<br/>delta writer (part 3)"] -->|"data files +<br/>equality delete files"| C["Coordinator (part 2)"] C -->|"equality delete files"| X["EqualityDeleteConverter<br/>(this PR)"] X -->|"DVs"| C C -->|"RowDelta: data files + DVs"| T[("Iceberg v3 table<br/>no equality deletes")]Series
EqualityDeleteConverterand its benchmark. No callers, no behavior change.(
iceberg.tables.convert-equality-deletes-enabled, validation, retries, docs).iceberg.tables.cdc-field,iceberg.tables.upsert-mode-enabled).Each part is opened after the previous one is merged, so no merged state can write equality
deletes without the conversion.
What this PR adds
EqualityDeleteConverterResultfor the commit.KeyPositionResolverScanKeyPositionResolver_posof the candidate files. Keeps no state.KeyFiltersINlists or the bounds of the leading key column.Resultmaps directly onto theRowDeltathat part 2 builds:baseSnapshotIdforvalidateFromSnapshot,dvFilesforaddDeletes(and their referenced data files forvalidateDataFilesExist, so a concurrent compaction fails the commit instead of resurrecting rows),rewrittenDvFilesforremoveDeletes, andconflictFilterfor conflict detection.The conversion gives the same rows as committing the equality deletes. A partitioned delete only
applies to data files with the same spec ID and partition values, also after partition evolution.
The pruning filters below only narrow the candidates.
Format version 3 only. A data file can have only one DV, so the new DV also carries the data file's
existing DV or v2 position deletes.
How the candidate files are pruned
The key filter uses
INlists (200 values per list, the evaluator limit) for up to 1,000 keys.Above that it uses the bounds of the leading key column, which can keep most files of the
partition when the keys are scattered; the benchmark below covers this.
Benchmark
EqualityDeleteConverterBenchmark(JMH) times one conversion on an unpartitioned table, where thekey filter alone decides which data files are read. It varies the table size, how keys lie in the
data files (increasing IDs, unsorted or sorted UUIDs), which rows change (recent, scattered, new
keys from upsert), and keys per commit across the
IN-list limit. It was planned inthis thread, and the results
are in this comment.
The tables are on local disk by default.
BENCHMARK_CATALOG_PROPERTIESnames a catalog propertiesfile instead, and
kafka-connect/kafka-connect-runtime/docker/benchmark-catalog.propertiesusesthe REST catalog and RustFS of the integration tests.
Adding object store latency with toxiproxy
Only S3 requests go through the proxy. The REST catalog is reached directly.
Not covered yet: partitioned layouts, bloom filters, larger data files, and read latency compared
with the same commits written as equality deletes. The envelope is where the conversion finishes
within the commit interval. Past it, the conversion still runs and costs commit time, not
correctness.
Testing
TestEqualityDeleteConverter(17 tests) on a real v3 table: conversion of earlier commits,merging with an existing DV or v2 position deletes, partition pruning (identity and
bucket),file pruning for clustered and scattered keys and past the
IN-list limit, a custom resolver,branches including one that does not exist yet, a concurrent append of a deleted key that the
commit's validation rejects, and rejection of non-equality deletes and format version 2.
TestKeyFilters(2 tests): the files kept byINlists and by the bounds../gradlew -DkafkaVersions=3 :iceberg-kafka-connect:iceberg-kafka-connect:checkpasses.AI Disclosure