fix: defer blank rows when a fully deleted batch has no row to copy - #8612
Open
LuciferYang wants to merge 1 commit into
Open
fix: defer blank rows when a fully deleted batch has no row to copy#8612LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
Adding or merging a column fails with "Missing too many rows in merge, run compaction to materialize deletions first" when a fragment's deleted rows line up with its first read batch. Every data file in a fragment must hold the same physical row count, so the updater restores the deleted rows before writing. `add_blanks` materializes a placeholder by copying the batch's first row, which means the batch needs at least one live row. A deleted run that trails live rows is greedily appended to the preceding batch and always has one, but a run starting at physical row 0 arrives as an empty batch carrying every one of its offsets, and the copy has nothing to copy from. Defer those blanks instead of inventing values for an arbitrary schema: `DeletionRestorer` remembers how many rows it owes and prepends them to the next batch that does have a live row. Deleted rows sort before the live rows that follow them, so the physical row order is unchanged. Copying a real row also keeps the placeholder valid for a non-nullable column, which null placeholders would not. Legacy files must reproduce their original row group size, which deferring would break, so they keep reporting the existing error and `add_blanks` keeps rejecting an empty batch.
Contributor
Author
Contributor
Author
|
cc @Xuanwo @wjones127 @hamersaw FYI |
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The change addresses the leading fully deleted-batch failure at the deletion-restoration boundary: current-format rewrites carry the owed physical rows into the next live batch, preserving row order and non-nullable values, while legacy fixed row-group behavior remains unchanged. The state-machine and end-to-end Rust regressions cover the relevant positions and failure boundaries.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adding or merging a column fails with
NotSupported: Missing too many rows in merge, run compaction to materialize deletions firstwhen a fragment's deleted rows cover its first read batch.This supersedes #7318, which goes after the same failure. Parameterizing #7233's test by where the fully deleted batch sits shows the middle and trailing cases already pass on
main, for both nullable and non-nullable new columns. Soadd_blanksrejecting an empty batch is not a middle-of-fragment problem, and the oversized-batch slicing in that PR does not change any observable behavior. Only a run starting at physical row 0 fails.Root cause
Every data file in a fragment has to hold the same physical row count, so the updater restores the deleted rows before writing.
add_blanksmaterializes a placeholder by copying the batch's first row, which means the batch needs at least one live row.A deleted run that trails live rows gets greedily appended to the preceding batch, so it always has one. A run starting at physical row 0 arrives as an empty batch carrying every one of its offsets, and the copy has nothing to copy from.
Fix
Defer those blanks rather than invent values for an arbitrary schema:
DeletionRestorerremembers how many rows it owes and prepends them to the next batch that does have a live row. Deleted rows sort before the live rows that follow them, so the physical row order is unchanged. Copying a real row also keeps the placeholder valid for a non-nullable column, which a null placeholder would not.Legacy files have to reproduce their original row group size, which deferring would break, so they keep reporting the existing error and
add_blankskeeps rejecting an empty batch.Two hardening changes in
add_blankscame with it. Offsets that are not strictly increasing used to underflowu32silently, and an offset past the batch's live rows used to reacharrow::compute::takewith bounds checking off. Both now returnInternalnaming the offending offset.Tests
updater.rs: sixDeletionRestorercases covering the deferral, a second empty batch carrying the debt through, the offset shift,is_exhaustedwhile blanks are owed, and no double counting of deferred rows. Plusadd_blanksrejection cases for non-increasing, equal, and out-of-range offsets, and a blank landing exactly at the end of a batch.schema_evolution.rs:test_add_columns_with_fully_deleted_batchparameterized over leading/middle/trailing by nullable/non-nullable, and a legacy case asserting the error still comes fromadd_blanks.fragment.rs: a deletion vector naming a row past the end of the fragment must fail the stream at its end instead of writing a short data file.test_dataset.py:merge_columnsplusLanceOperation::Mergeover a dataset with a deletion file, parameterized the same three ways, with the merged column declared non-nullable.