feat: better ways to use deletion vectors #215

hntd187 · 2024-05-22T19:04:33Z

This is meant to be some utils to help make DV usage easier. You can

Get a deletion/selection vector
Get a DV or SV in bits or bools depending
Get either DV or SV in batches or a full vector specified by the caller
Pass an array, slice or vec to use for the container
Implement the traits to provide your own container to operate on

TODO:

Docs
More tests
FFI integration for this. I'd prefer to get the rust side right first and then I can extend for FFI usage.
Correctness, is this what we want?

nicklan

Took a first pass. Overall I think this is the right idea, but I'm not sold on the batch thing

kernel/src/actions/deletion_vector.rs

scovich

Flushing first pass. Seems like an interesting start, but not sure I have a "big picture" view of the idea quite yet?
How selection vs. deletion vectors would work, how engine could provide its own bit vector implementation, etc.

kernel/src/actions/deletion_vector.rs

scovich

Lots of comments but not much substance in the end... rust pointers are just messy no matter how you "slice" it :(

scovich · 2024-06-08T20:23:36Z

ffi/src/lib.rs

@@ -122,6 +126,12 @@ mod private {
        len: usize,
    }

+    #[repr(C)]
+    pub struct KernelRowIndexArray {
+        ptr: *mut u64,


is this accurate?

Suggested change

ptr: *mut u64,

ptr: NonNull<u64>,

update: this mirrors the existing KernelBoolSlice, which uses a null pointer to represent the empty slice.

However, according to std::slice::from_raw_parts:

You can obtain a pointer that is usable as data for zero-length slices using NonNull::dangling().

... that way, we wouldn't need any null check in the as_ref() method below.

The docs for Vec don't specifically mention NonNull::dangling, but Vec::as_ptr does return

a dangling raw pointer valid for zero sized reads if the vector didn’t allocate.

The docs for Vec::from_raw_slice are not clear tho:

ptr must have been allocated using the global allocator, such as via the alloc::alloc function. ... These requirements are always upheld by any ptr that has been allocated via Vec<T> [which presumably includes the pointer returned by as_ptr which we already trust for the non-empty case?]. Other allocation sources are allowed if the invariants are upheld.

Further, both Unique::dangling and NonNull::dangling state that they are

useful for initializing types which lazily allocate, like Vec::new does.

That said, it's probably safest of all to just use Vec::new().as_mut_ptr() as pointer for an empty index slice.

However, the null pointer approach is simpler if the engine might ever need to create one of these to pass into kernel? Actually, given how messy all the non-null/unique/etc constraints are, nullable pointers are probably the simplest even if engine doesn't need to create one.

Sorry it's not clear to me what changes should be made? Should be switch both slice types over to a new way in a subsequent PR?

Yeah sorry that was a lot of rambling/exploring. Basically, we have two options for representing an empty slice:

Pointer is always non-null, length 0 and dangling pointer means empty

Null pointer means empty

Rust and its library classes don't have a really nice way to handle either case, so IMO we should choose the least-bad approach (= least code and least error prone) and use that consistently. Maybe the current code is already the correct choice.

Actually, the current code does both -- the slice -> vec code assumes nullable pointer, while the vec -> slice (impl From) uses a dangling pointer. We should pick one and stick with it.

Any opinion on which you prefer @scovich ? I'd probably go with the always non-null length zero.

I updated both bool and row id slice to use non-zero length as empty.

ffi/src/lib.rs

scovich · 2024-06-08T22:55:26Z

ffi/src/lib.rs

+            let mut vec = value.row_indexes();
+            vec.shrink_to_fit();


Suggested change

let mut vec = value.row_indexes();

vec.shrink_to_fit();

let vec = value.row_indexes();

Vec::into_boxed_slice states:

Before doing the conversion, this method discards excess capacity like shrink_to_fit.

Aside: This feels like a very messy part of rust. There are so many ways to convert to and from pointers, and it's not clear that any of them is the "best" -- especially when bits don't fit together nicely. Like here -- into_raw returns a pointer that satisfies NonNull, but there's no "safe" way to actually create a NonNull from it.

ffi/src/lib.rs

kernel/src/actions/deletion_vector.rs

nicklan

generally fine, but we need to augment the actual ffi apis. So today we have selection_vector_from_dv (in ffi/src/scan.rs), which always returns a bool slice.

we should probably add a row_indexes_from_dv to mirror that.

Alternately, we could have a single function that can return whatever the user asks for, but while that could be clean in rust (return an enum), I think over ffi it's probably easier/cleaner to just have multiple functions each with its own return type.

ffi/src/lib.rs

kernel/src/actions/deletion_vector.rs

nicklan · 2024-06-12T16:59:59Z

ffi/src/lib.rs

@@ -4,12 +4,14 @@
 #[cfg(any(feature = "default-engine", feature = "sync-engine"))]
 use std::collections::HashMap;
 use std::default::Default;
+use std::ffi::{c_long, c_longlong, c_ulonglong};


Suggested change

use std::ffi::{c_long, c_longlong, c_ulonglong};

not used

nicklan · 2024-06-12T17:00:32Z

ffi/src/lib.rs

 use std::os::raw::{c_char, c_void};
 use std::ptr::NonNull;
 use std::sync::Arc;
 use tracing::debug;
 use url::Url;

+use delta_kernel::actions::deletion_vector::DeletionVector;


Suggested change

use delta_kernel::actions::deletion_vector::DeletionVector;

not used

nicklan

mostly good! I think there's still some stuff ryan pointed out that needs to be fixed, and I had one more thing.

kernel/src/actions/deletion_vector.rs

kernel/src/scan/state.rs

kernel/src/actions/deletion_vector.rs

ffi/src/lib.rs

kernel/src/actions/deletion_vector.rs

…d of nullable pointer

nicklan

thanks for iterating on this. just a couple of doc comments and we're good to go (i think)

ffi/src/lib.rs

kernel/src/scan/state.rs

…d of nullable pointer

zachschuermann

just a few nits - and can we update the PR description to accurately represent the changes?

zachschuermann · 2024-07-25T22:00:57Z

ffi/src/lib.rs

@@ -153,10 +163,10 @@ mod private {
        /// The slice must have been originally created `From<Vec<bool>>`, and must not have been
        /// already been consumed by a previous call to this method.
        pub unsafe fn as_ref(&self) -> &[bool] {
-            if self.ptr.is_null() {
+            if self.len == 0 {


safety nit: should we add a comment saying that the pointer is assumed valid when len > 0 and is invalid when len == 0?

zachschuermann · 2024-07-25T22:23:00Z

ffi/src/lib.rs

@@ -190,13 +204,57 @@ mod private {
    /// memory, but must only free it by calling [super::free_bool_slice]. Since the global
    /// allocator is threadsafe, it doesn't matter which engine thread invokes that method.
    unsafe impl Send for KernelBoolSlice {}
+    unsafe impl Send for KernelRowIndexArray {}


safety comment

zachschuermann · 2024-07-25T23:17:51Z

kernel/src/actions/deletion_vector.rs

@@ -382,4 +392,16 @@ mod tests {
        expected[4294967300] = false;
        assert_eq!(bools, expected);
    }
+
+    #[test]
+    fn test_dv_wrapper() {


Suggested change

fn test_dv_wrapper() {

fn test_dv_row_indexes() {

feat: better ways to use deletion vectors

9cd41f2

hntd187 requested review from rtyler, nicklan, scovich, zachschuermann and roeap May 22, 2024 19:04

hntd187 self-assigned this May 22, 2024

Merge branch 'main' into dv-utils

9edcce9

nicklan requested changes May 22, 2024

View reviewed changes

scovich reviewed May 22, 2024

View reviewed changes

hntd187 and others added 2 commits May 22, 2024 19:27

Merge branch 'main' into dv-utils

6acb5f8

feat: option to pass DVs over as just array of row indexes

6c726f5

scovich reviewed Jun 8, 2024

View reviewed changes

nicklan requested changes Jun 12, 2024

View reviewed changes

hntd187 and others added 4 commits June 21, 2024 08:43

Merge branch 'main' into dv-utils

0c20f6b

Updated and added some plumbing for the row index vector creation

213c519

chore: fmt

0da2dfb

Merge branch 'main' into dv-utils

ea36d70

hntd187 requested a review from nicklan June 21, 2024 21:19

nicklan requested changes Jun 21, 2024

View reviewed changes

kernel/src/actions/deletion_vector.rs Outdated Show resolved Hide resolved

hntd187 and others added 2 commits June 27, 2024 08:26

Merge branch 'main' into dv-utils

1c897c6

More deleting, a few doc comments, some cleanup

b33d621

hntd187 requested a review from nicklan June 27, 2024 12:57

scovich reviewed Jul 1, 2024

View reviewed changes

hntd187 and others added 3 commits July 5, 2024 13:36

Merge branch 'main' into dv-utils

c621634

Address PR feedback

4b8e18f

Address PR feedback

63bdbc6

nicklan mentioned this pull request Jul 11, 2024

Better docs about selection/deletion vectors #277

Open

hntd187 and others added 2 commits July 20, 2024 15:04

Merge branch 'main' into dv-utils

b92fcc0

Address PR feedback, use dangling pointer for init/empty array instea…

bfc7486

…d of nullable pointer

nicklan requested changes Jul 22, 2024

View reviewed changes

ffi/src/lib.rs Show resolved Hide resolved

kernel/src/scan/state.rs Show resolved Hide resolved

Merge branch 'main' into dv-utils

59c150b

hntd187 requested a review from scovich July 23, 2024 00:04

hntd187 added 2 commits July 25, 2024 13:42

Address PR feedback, use dangling pointer for init/empty array instea…

8e19022

…d of nullable pointer

Merge remote-tracking branch 'mine/dv-utils' into dv-utils

265d1f1

zachschuermann reviewed Jul 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: better ways to use deletion vectors #215

feat: better ways to use deletion vectors #215

hntd187 commented May 22, 2024

nicklan left a comment

scovich left a comment

scovich left a comment

scovich Jun 8, 2024

scovich Jun 8, 2024

scovich Jun 8, 2024

scovich Jun 8, 2024

scovich Jun 8, 2024

hntd187 Jul 5, 2024

scovich Jul 8, 2024

scovich Jul 8, 2024

hntd187 Jul 20, 2024

hntd187 Jul 20, 2024

scovich Jun 8, 2024

scovich Jun 8, 2024

nicklan left a comment

nicklan Jun 12, 2024

nicklan Jun 12, 2024

nicklan left a comment

nicklan left a comment

zachschuermann left a comment

zachschuermann Jul 25, 2024

zachschuermann Jul 25, 2024

zachschuermann Jul 25, 2024

	let mut vec = value.row_indexes();
	vec.shrink_to_fit();
	let vec = value.row_indexes();

feat: better ways to use deletion vectors #215

Are you sure you want to change the base?

feat: better ways to use deletion vectors #215

Conversation

hntd187 commented May 22, 2024

nicklan left a comment

Choose a reason for hiding this comment

scovich left a comment

Choose a reason for hiding this comment

scovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

nicklan left a comment

Choose a reason for hiding this comment

zachschuermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment