Repository navigation
Core: Merge concurrently added DVs for RowDelta pure delete commits - #17754
raunaqmorarka wants to merge 1 commit into
Conversation
d23c1c3 to
3a19bce
Compare
3a19bce to
ecd1277
Compare
ecd1277 to
7d26b96
Compare
|
Reviewed this end to end. The concurrent-DV merge is correct — the union preserves all deleted positions given the one-DV-per-file invariant, and restricting merges to DELETE-sourced snapshots ( @uros-b the FileIO-resolved cleanup-location comment looks addressed — would you mind another look? This is a clean base to extend row-level concurrency to UPDATE/MERGE. |
7d26b96 to
8de3a47
Compare
|
cc @huaxingao — you pointed to this PR on the dev@ thread on row-level concurrency for deletion vectors. I've reviewed it end to end and it looks ready to land: the one open review comment (tracking the FileIO-resolved cleanup location) is addressed, and the test coverage is thorough. The position-granular extension to UPDATE/MERGE I'm working on builds directly on this, so it'd be great if you could do a review pass to help it move forward. |
4d95c47 to
99592bf
Compare
|
@amogh-jahagirdar @nastra could you please take a look? This relaxes the "Found concurrently added DV" failure for pure RowDelta deletes and merges the concurrent DV instead. I've reviewed it and it looks correct to me, but a second pair of eyes on the commit-path semantics would be good. Thanks! |
99592bf to
92340c1
Compare
|
Gentle nudge on this one 🙏 @nastra @amogh-jahagirdar — as the primary maintainers of the commit-path code here ( |
uros-b
left a comment
There was a problem hiding this comment.
Makes sense to me too, but yeah let's wait for additional feedback from committers!
|
@nastra @amogh-jahagirdar — gentle follow-up on the request for committer feedback. @uros-b noted in the September 29 review that the change makes sense but should wait for additional feedback from committers: #17754 (review) Could either of you take a look at the concurrent-DV merge for pure RowDelta deletes, particularly the commit/retry semantics, when you have a chance? Thanks! |
|
@rdblue @RussellSpitzer @szehon-ho could one of you take a look at this when you have a chance? It merges a concurrently added DV into the operation's DV for pure RowDelta deletes, instead of failing with @huaxingao has reviewed it end to end and asked for a second committer's view on the commit/retry semantics. All open comments are addressed, including the rollback case where the concurrent DV disappears between attempts. It is also the first step of the row-granular DV concurrency proposal on dev@ (#18020). Thanks! |
4706c39 to
c9e694c
Compare
Concurrent DELETE commits producing DVs for the same data file always fail validation even though position deletes are commutative.
c9e694c to
0371a49
Compare
Concurrent DELETEs on V3 tables fail with
Found concurrently added DVwhen they add DVs for the same data file, even though position deletes are commutative.This change merges the concurrently added DV into the operation's DV instead of failing, when all of the following hold:
validateNoConflictingDeleteFilesis not enabledThe newest concurrent DV for a data file contains the content of all older ones, since the V3 spec requires a DV to include all positions of the DV it replaces. That DV is removed from the table and its content is merged into the new DV, and pending removals of delete files for that data file are dropped. Merged puffin files from invalidated commit attempts are deleted on retry and on commit failure.