feat: add bulk insertion to deletion vector by dentiny · Pull Request #1578 · apache/iceberg-rust

dentiny · 2025-08-03T01:06:22Z

What changes are included in this PR?

In this PR, I added a bulk insertion API to deletion vector and roaring bitmap.

Context:

I'm working on iceberg-related feature on daily basis, and I'm implementing own deletion vector and puffin blob myself
- Code reference: https://github.com/Mooncake-Labs/moonlink/blob/main/src/moonlink/src/storage/iceberg/deletion_vector.rs
I would like to leverage upstream's implementation to reduce re-inventing the wheels, then I noticed some differences
- My impl supports bulk insertion, because append provides better perf
- In my use case, all deleted rows are fetched in ascending order

Are these changes tested?

Yes, unit tests added.

liurenjie1024

Thanks @dentiny for this pr, generally looks good!

liurenjie1024 · 2025-08-04T10:17:46Z

crates/iceberg/src/delete_vector.rs

+    pub fn append_positions(&mut self, positions: &[u64]) -> bool {
+        let expected_num = positions.len();
+        let appended_num = self.inner.append(positions.iter().copied()).unwrap();
+        appended_num as usize == expected_num


This definition seems odd to me, which treats insertation with duplication as a failure. I would suggest to return nothing.

Thanks for the comment!
Yeah I think it's too specific for my use case (which requires no duplicates and ordering).

In the latest commit, I updated the return value to # of items inserted:

It matches the interface for roaring bitmap

For my own usage, I do want to assert no duplicate insertions

Let me know if you're fine with it, thank you for the quick review and constructive comments!

crates/iceberg/src/delete_vector.rs

liurenjie1024 · 2025-08-06T09:53:58Z

crates/iceberg/src/delete_vector.rs

+    /// Precondition: The values of the iterator must be ordered and strictly greater than the greatest value in the set.
+    /// If a value in the iterator doesn’t satisfy this requirement, it is not added and the append operation is stopped.
+    #[allow(dead_code)]
+    pub fn insert_positions(&mut self, positions: &[u64]) -> u64 {


There are some misunderstading here. What I mean is:

If the inner append fail, we should also return this error.

If we succeeded, we should return the actual number inserted, rather the len of positions. The difference is, if the inner already contains some value in positions, the returned length should substract the common part.

Please add detailed doc for the behavior.

Also please add some tests for the case where there are intersections.

If the inner append fail, we should also return this error.

I considered it:

I met some problems to wrap it inside of iceberg error, for example, I don't want to add another error category for this particular error type and may lose information (successfully inserted rows)

Returning the number of successfully inserted rows is a well-defined behavior, and it's always caller's responsibility to check whether row insertion works

Another consideration here is whether we want users to keep insertion upon failure:

Returning an error, without inserted rows being specified, means deletion vector could be at a broken state, and users are not supposed to keep insertion;

Returning the number of rows inserted indicates clearly which part gets inserted and which parts not, so it's actually legal to keep insertion after updating the input arguments.

Also please add some tests for the case where there are intersections.

I actually already have certain cases:

iceberg-rust/crates/iceberg/src/delete_vector.rs

Lines 176 to 181 in b297231

// Perform another bulk insertion after an incomplete one.

assert_eq!(dv.insert_positions(&[2, 4]), 0);

let mut collected: Vec<u64> = dv.iter().collect();

collected.sort();

assert_eq!(collected, expected_positions);

Do you think that's enough?

I updated the doc and impl to return Result, which I wrap iceberg::Result around the returned result;
let me know if you think it makes sense :)
Thank you so much for the explanation and quick review!

crates/iceberg/src/delete_vector.rs

liurenjie1024

Thanks @dentiny for this pr, LGTM! Just one nit about error handling.

dentiny · 2025-08-07T14:11:48Z

Just one nit about error handling.

Thank you so much for the careful review! I learnt a lot.

liurenjie1024

Thanks @dentiny for this pr!

## What changes are included in this PR? In this PR, I added a bulk insertion API to deletion vector and roaring bitmap. Context: - I'm working on iceberg-related feature on daily basis, and I'm implementing own deletion vector and puffin blob myself + Code reference: https://github.com/Mooncake-Labs/moonlink/blob/main/src/moonlink/src/storage/iceberg/deletion_vector.rs - I would like to leverage upstream's implementation to reduce re-inventing the wheels, then I noticed some differences + My impl supports bulk insertion, because `append` provides better perf + In my use case, all deleted rows are fetched in ascending order ## Are these changes tested? Yes, unit tests added.

dentiny added 2 commits August 3, 2025 01:02

feat: add bulk insertion to deletion vector

2d837df

simplify test code

9097cd2

liurenjie1024 reviewed Aug 4, 2025

View reviewed changes

update interface

c54d670

dentiny requested a review from liurenjie1024 August 4, 2025 17:28

liurenjie1024 reviewed Aug 6, 2025

View reviewed changes

crates/iceberg/src/delete_vector.rs Outdated Show resolved Hide resolved

crates/iceberg/src/delete_vector.rs Outdated Show resolved Hide resolved

doc, no error and unit test

b297231

dentiny requested a review from liurenjie1024 August 6, 2025 09:25

liurenjie1024 reviewed Aug 6, 2025

View reviewed changes

dentiny added 3 commits August 6, 2025 18:26

return error status

33e6d66

doc

ce36af0

revert back to unused

aefed79

dentiny force-pushed the hjiang/bulk-insertion-deletion-vector branch from 1b1127f to aefed79 Compare August 6, 2025 18:37

dentiny requested a review from liurenjie1024 August 6, 2025 18:38

liurenjie1024 reviewed Aug 7, 2025

View reviewed changes

crates/iceberg/src/delete_vector.rs Outdated Show resolved Hide resolved

crates/iceberg/src/delete_vector.rs Show resolved Hide resolved

liurenjie1024 reviewed Aug 7, 2025

View reviewed changes

error with source

4ad3e60

clippy

e6e1a74

dentiny requested a review from liurenjie1024 August 7, 2025 14:43

liurenjie1024 approved these changes Aug 8, 2025

View reviewed changes

liurenjie1024 merged commit 50cd2ab into apache:main Aug 8, 2025
17 checks passed

	// Perform another bulk insertion after an incomplete one.
	assert_eq!(dv.insert_positions(&[2, 4]), 0);

	let mut collected: Vec<u64> = dv.iter().collect();
	collected.sort();
	assert_eq!(collected, expected_positions);

Conversation

dentiny commented Aug 3, 2025

What changes are included in this PR?

Are these changes tested?

Uh oh!

liurenjie1024 left a comment

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

dentiny Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liurenjie1024 Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

liurenjie1024 Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

dentiny Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dentiny Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

dentiny Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liurenjie1024 left a comment

Choose a reason for hiding this comment

Uh oh!

dentiny commented Aug 7, 2025

Uh oh!

liurenjie1024 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dentiny Aug 4, 2025 •

edited

Loading

dentiny Aug 6, 2025 •

edited

Loading