perf(arrow): build the delete row selection from deleted ranges by JeonDaehong · Pull Request #3454 · apache/iceberg-rust

JeonDaehong · 2026-10-14T14:29:25Z

Which issue does this PR close?

Closes DeleteVectorIterator::advance_to mishandles positions at the 2^32 boundary #3400.

What changes are included in this PR?

build_deletes_row_selection walks the delete vector one position at a time, so a run of deleted rows costs one iteration per row. On files with many contiguous deletes this is a large part of the read.

This builds the selection from deleted ranges instead (DeleteVector::ranges):

Short runs are still walked with next. A run switches to roaring's next_range after 4 consecutive positions, because next_range binary-searches the rest of an array container on each call and is slower than next when most runs are single rows.
Row groups that are not selected are skipped with skip_to instead of walking their deletes.
The range iterator is crate-private, so the public API is unchanged. The reader no longer calls DeleteVectorIterator::advance_to (public since feat(delete-vector): make the delete_vector module public #3461), but it is kept and now fixed for the cases in DeleteVectorIterator::advance_to mishandles positions at the 2^32 boundary #3400, using the same check as the new skip_to.

Results are identical to main (checked in the benchmark below and in the tests).

Benchmark (c7i.2xlarge, release, 7 runs alternating old/new, median)

Reading a 20M-row Parquet file (4 numeric columns, 20 row groups, local disk), selection build + read:

deletes	main	this PR
1000 large runs, 50%	190 ms	125 ms
runs of 100, 50%	223 ms	159 ms
runs of 20, 50%	279 ms	224 ms
99% deleted	260 ms	135 ms
5% scattered single rows	270 ms	271 ms
pairs, 10%	270 ms	269 ms
0.01%	163 ms	163 ms

The last three are within run-to-run noise. Selection build alone on 100M rows: 329 ms → 0.05 ms (large runs), 689 ms → 59 ms (99%), 172 ms → 174 ms (5% scattered, ranges overlap). The share of the build in a read is smaller with more or wider columns or remote storage.

I also tried switching after 8, 16 and 32 positions; 4 was best for runs and showed no slowdown on scattered deletes.

I did the same kind of change for Java's DV iteration in apache/iceberg#18027.

Are these changes tested?

Yes. Model tests in delete_vector.rs check ranges()/skip_to and advance_to on mixed sets (single rows, pairs, runs around the switch point, runs across 2^16 and 2^32, gaps between bitmaps), with forward and backward targets, plus one test per #3400 case and a u64::MAX test. A randomized test checks build_deletes_row_selection against a per-position model over random row group layouts and selections, and another crosses 2^32 with and without row group selection. Existing positional delete tests pass unchanged. Breaking skip_to or dropping a buffered run in four different ways makes the new tests fail. cargo test -p iceberg --lib, clippy and fmt are clean.

AI Disclosure

I used Claude Code to write the change, the tests and the benchmark, and to run them. I reviewed it before opening the PR.

JeonDaehong · 2026-10-14T14:39:13Z

@laskoviymishka @mbutrovich could you take a look when you have time? This builds the delete row selection from deleted ranges instead of per position: reads with contiguous deletes get 20-48% faster, scattered deletes are unchanged (numbers in the description). cc @Baunsgaard, since #2292 looked at range handling for delete vectors.

build_deletes_row_selection walked the delete vector one position at a time, so a run of deleted rows cost one iteration per row. It now reads the deleted positions as ranges, which makes a run one step. Short runs are still walked with `next`, because roaring's `next_range` binary-searches the rest of an array container on every call and is slower than `next` for single positions. Row groups that are not selected are skipped with `skip_to` instead of walking their deletes. The new range iterator is crate-private; the public `DeleteVector::iter` and `DeleteVectorIterator::advance_to` are unchanged.

JeonDaehong · 2026-10-15T15:04:09Z

Rebased on #3461, which made delete_vector public. This PR now keeps DeleteVector::iter, DeleteVectorIterator and advance_to as they are and adds the range iterator as crate-private, so public-api.txt is unchanged. delete_vector and positional_deletes tests, clippy, fmt and cargo doc with -D warnings pass.

laskoviymishka

Building the selection from ranges is the right shape for this, and I couldn't find a path where it keeps a deleted row or drops a live one. Carrying a range into the next row group, cutting into a skipped group and splitting at 2^32 all line up with the old per-position walk. The probe before next_range adds state, but the scattered-delete numbers show why it's there.

What I'd still like in this PR is test coverage for the reader that matches the iterator's. ranges() and skip_to get a model test, but build_deletes_row_selection was rewritten wholesale and only has hand-picked cases. A randomized check against a per-row oracle, over random row-group layouts and selected subsets, would cover it. Also, the + 1 in full_range shouldn't be able to panic at u64::MAX.

Separately, skip_to now has the logic that fixes both #3400 cases, but DeleteVectorIterator::advance_to still has both bugs. It's been public since #3461 and has no caller inside the crate after this. Porting the fix over here would let #3400 close. A follow-up is fine too, as long as the comment above it stops pointing at a roaring PR that has already merged.

laskoviymishka · 2026-10-15T18:09:25Z

+    /// Row groups and deleted runs that cross the 2^32 position boundary, with and without row
+    /// group selection.
+    #[test]
+    fn test_build_deletes_row_selection_across_2_pow_32() {


I'd add a randomized test for build_deletes_row_selection next to this one: random row-group sizes, a random selected_row_groups subset and the DVs from mixed_delete_vectors, compared against a per-row oracle that run-length-encodes the selected groups.

The iterator gets a model test, but this loop was rewritten wholesale and only has hand-picked cases. The branch I'd least trust to stay correct is a range carried from a selected row group into a skipped one. mixed_delete_vectors would need to be pub(crate) under cfg(test) for this.

Added test_build_deletes_row_selection_matches_model: random row-group cuts (half placed next to a deleted position so they land inside runs), a random selected_row_groups subset or None, over every DV from mixed_delete_vectors (now pub(crate) under cfg(test)), compared with a selection built one deleted position at a time. Dropping the "range continues into the next row group" break, or skipping one row too far in a skipped group, makes it fail.

laskoviymishka · 2026-10-15T18:09:25Z


+    /// Returns the deleted positions as ascending, non-overlapping half-open ranges.
+    ///
+    /// Adjacent ranges are not always merged: a run that crosses a 2^32 boundary comes out as


I don't think 2^32 is the only split. The probe calls next_range once, so if that stops at a 2^16 container boundary, the rest of the run comes back as a separate range right next to it. I'd just say adjacent ranges may not be merged and callers shouldn't assume they're maximal. The reader is fine either way because RowSelection merges them.

Agreed, reworded to say adjacent ranges may not be merged and callers should not assume a range is maximal.

laskoviymishka · 2026-10-15T18:09:25Z

 // Ideally, we'd just wrap `roaring::RoaringTreemap`'s iterator, `roaring::treemap::Iter` here.
 // But right now, it does not have a corresponding implementation of `roaring::bitmap::Iter::advance_to`,
-// which is very handy in ArrowReader::build_deletes_row_selection.
+// which `advance_to` below needs.


This block is still stale after the edit: roaring-rs#314 has since merged (April 2025). After this PR, advance_to also has no caller inside the crate.

The bigger issue is that advance_to went public in #3461 with both #3400 bugs still in it. skip_to here already has the three-way high_bits vs hi handling that fixes them. We could port that into advance_to here so #3400 can close, or do it as an immediate follow-up and reword this comment to say advance_to is only kept for the public API. Which would you rather?

Ported it here, so this now closes #3400. advance_to uses the same three-way check as skip_to, which also fixes the "before the first next()" case from the issue. Removed the stale roaring-rs#314 and git-dependency notes. Each #3400 case has its own advance_to test, plus a model test like the one for skip_to.

laskoviymishka · 2026-10-15T18:09:25Z

+/// `roaring`'s `next_range` binary-searches the rest of an array container on every call, which
+/// costs more than `next` when most runs are a single position, so it only pays off for long
+/// runs.
+const RUN_PROBE_LEN: u32 = 4;


I'd move the 4 vs 8/16/32 comparison from the PR description into this doc comment, so whoever bumps roaring later knows the value was tuned, and on which workload.

Moved it into the doc comment, with the roaring version and the workloads.

laskoviymishka · 2026-10-15T18:09:25Z

+    /// A position already taken from `bitmap_iter` that starts the next range.
+    pending: Option<u32>,
+    /// A complete range already taken from `bitmap_iter`, returned next.
+    pending_range: Option<(u32, u32)>,


pending and pending_range are never both set, because each exit path in next() sets only one. But skip_to trims them separately and nothing enforces that. I'd fold them into one field so the type carries the invariant:

enum Buffered { /// Start of the next range. Pos(u32), /// A complete range, inclusive end. Range(u32, u32), }

That also documents the inclusive u32 end. The end < lo check in skip_to depends on it, and the current field doc doesn't say so.

Done, buffered: Option<Buffered> with Pos(u32) and Range(u32, u32) (inclusive end, documented on the variant).

laskoviymishka · 2026-10-15T18:09:25Z

+        let base = u64::from(self.high_bits) << 32;
+        (
+            base | u64::from(start),
+            (base | u64::from(end_inclusive)) + 1,


This + 1 overflows when the last deleted position is u64::MAX. In debug that's a panic; in release it wraps to (start, 0), which breaks the start < end contract the reader relies on.

It isn't a realistic row position, but the treemap can hold it and DVs come from external blobs, so this shouldn't be able to panic. Either use checked_add here and end the iterator at the edge, or reject positions above Java's MAX_POSITION at decode time and keep a debug_assert! here.

Fixed with saturating_add: a run ending at u64::MAX ends the range at u64::MAX, and a range holding only u64::MAX ends the iterator, since that position is past any row. test_ranges_end_at_u64_max covers both.

laskoviymishka · 2026-10-15T18:09:25Z

+        )
+    }
+
+    /// Skips every deleted position below `pos`. A range that starts below `pos` and ends after


The doc should say this only applies to ranges still buffered in the iterator. A range already returned by next() is the caller's to cut, which the reader does in its start < next_row_group_base_idx arm.

Added that to the doc.

laskoviymishka · 2026-10-15T18:09:25Z

+            let mut target = 0u64;
+            loop {
+                if rand() % 3 == 0 {
+                    target = target.max(remaining.first().copied().unwrap_or(max));


Because target only ever moves forward, the high_bits > hi early return and the "already past lo" path are only hit by the hand-written test below. I'd let the model sometimes pick a backward target, and add a treemap with gaps between bitmaps (high bits {0, 3, 7}) to mixed_delete_vectors so the loop that skips empty bitmaps gets exercised. Both paths also assume Iter::advance_to never rewinds, which is better checked by the randomized test than by reading the code.

The model now also skips backwards (near the current position and anywhere below it), and mixed_delete_vectors has a DV with high bits {0, 3, 7}. Removing the Buffered::Pos trim in skip_to fails the model test.

laskoviymishka · 2026-10-15T18:09:25Z

+        assert_eq!(ranges.next(), None);
+    }
+
+    // The two cases from https://github.com/apache/iceberg-rust/issues/3400.


I'd split this into two tests, e.g. ..._to_missing_later_bitmap and ..._below_current_bitmap, so a failure says which case broke. Since #3400 stays open, I'd also say these are the cases for skip_to; as written it reads as if the issue is fixed.

Split into test_ranges_skip_to_missing_later_bitmap and test_ranges_skip_to_below_current_bitmap, with the matching advance_to tests next to them.

- DeleteVectorIterator::advance_to now handles a target in a missing later bitmap, a target below the current bitmap, and a call before the first next (the cases in apache#3400), with the same three-way check as skip_to. Dropped the stale comments about roaring-rs#314 and the git dependency. - Randomized test for build_deletes_row_selection: random row group layouts and selections over mixed_delete_vectors, against a selection built one deleted position at a time. - full_range no longer overflows at u64::MAX. - One Buffered enum instead of pending/pending_range. - Model tests also skip backwards, mixed_delete_vectors has gaps between bitmaps, a model test for advance_to, the apache#3400 cases split per test, and doc updates for ranges, skip_to and RUN_PROBE_LEN.

JeonDaehong · 2026-10-16T06:20:21Z

Thanks @laskoviymishka. Addressed everything in 1820b84, including the advance_to fix, so this now closes #3400. delete_vector and positional_deletes tests, clippy, fmt and cargo doc with -D warnings pass.

akshat-lakhera

I read through the latest version against the roaring 0.11.4 source, and I couldn't find a case where a deleted row is kept or a live one is selected either. A few things that aren't covered in the earlier review:

In roaring 0.11.4, next_range already joins a run across 2^16 containers. next_range_impl in bitmap/iter.rs keeps pulling the next container while the range ends at 0xFFFF and the next container starts at 0. So within one 32-bit bitmap the ranges this returns are maximal, and the only split is at 2^32. The doc wording that ranges may not be merged is still the safe thing to say. A small test asserting a run across 2^16 comes back as one range would catch a future roaring change that starts splitting there again. That would be a silent slowdown rather than a correctness bug, so it's optional.
The randomized reader test always picks num_rows above the last deleted position, so it never generates deletes past the end of the file. The old loop ignored those, and the new one should too, both when the last row group is selected and when it is skipped. Letting num_rows sometimes land at or below the last deleted position would cover that path.
The note that a range holding only u64::MAX is dropped sits on full_range, which is private. It would read better on ranges() itself, next to the note about ranges not being merged, since that's what callers read.
The doc comment on RUN_PROBE_LEN now says it was tuned on a specific workload with roaring 0.11.4, but the benchmark isn't in the tree, so re-tuning after a roaring bump means rebuilding it. A follow-up that adds it as a bench would make that easier. Not needed for this PR.

… docs

JeonDaehong · 2026-10-17T10:39:35Z

Thanks @akshat-lakhera for checking this against the roaring source. Addressed in 0bc58a0:

Added test_ranges_join_runs_across_2_16, which checks that a run across 2^16 comes back as one range, both when next_range picks it up and when the run starts within the positions walked first. I also changed the ranges() doc to say a run is split at each 2^32 boundary.
The randomized reader test now ends the file at or just after a deleted position half the time, so deletes past the end of the file are covered with the last row group both selected and skipped.
Moved the u64::MAX note to ranges().
Agreed. I'll add the benchmark as a follow-up.

JeonDaehong force-pushed the perf-dv-range-row-selection branch from 2d09acc to 46ff84f Compare October 15, 2026 15:03

JeonDaehong mentioned this pull request Oct 15, 2026

DeleteVectorIterator::advance_to mishandles positions at the 2^32 boundary #3400

Open

laskoviymishka approved these changes Oct 15, 2026

View reviewed changes

JeonDaehong force-pushed the perf-dv-range-row-selection branch from f952232 to 1820b84 Compare October 16, 2026 06:19

This was referenced Oct 16, 2026

ci: Limit open, non-draft PRs to 3 for users without write access #3465

Merged

[core] Build deletion-vector row ranges from roaring runs apache/paimon-rust#1146

Open

akshat-lakhera reviewed Oct 17, 2026

View reviewed changes

Address review: deletes past the file end, runs across 2^16, ranges()…

0bc58a0

… docs

Conversation

JeonDaehong commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

JeonDaehong commented Oct 14, 2026

Uh oh!

JeonDaehong commented Oct 15, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeonDaehong commented Oct 16, 2026

Uh oh!

akshat-lakhera left a comment

Choose a reason for hiding this comment

Uh oh!

JeonDaehong commented Oct 17, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JeonDaehong commented Oct 14, 2026 •

edited

Loading