feat: teach DeltaArray slice and scalar_at #927

danking · 2024-09-25T14:59:15Z

More subtle than I expected.

DeltaArray is a sequence of chunks. All chunks except the last must be "full", i.e. containing 1,024 values. The last chunk may contain as few as one value and is encoded differently from the rest.

In this PR, I introduced an "offset" and a "limit". Together they enable logical/lazy slicing while preserving full chunks for later decompression. The offset is a value, less than 1024, which offsets into the first chunk. The limit is either None or less than 1024. None represents no limit which allows callers to avoid computing the length of the last chunk [1]. Internally, the limit is converted to a "right-offset": trailing_garbage which is sliced away when decompression happens.

I also added support for signed values. I verify they're non-negative in delta_compress.

[1] Which is, a bit annoyingly, this:

match deltas.len() % 1024 {
    0 => 1024,
    n => n
}

danking · 2024-09-25T15:06:06Z

If you're curious how using DeltaEncoding affects things, this comment benchmarks the use of DeltaEncoding for the offsets of the varbin array inside an FSST array.

robert3005

There's couple places where we are not using compute functions the right way

robert3005 · 2024-09-25T15:01:37Z

encodings/fastlanes/src/delta/compress.rs

+    let array = if array.ptype().is_signed_int() {
+        match_each_signed_integer_ptype!(array.ptype(), |$T| {
+            assert!(array.statistics().compute_min::<$T>().map(|x| x >= 0).unwrap_or(false));
+        });
+        &array.reinterpret_cast(array.ptype().to_unsigned())
+    } else {
+        array
+    };
+


Generally can handle this in the compressor where you would say you will only delta compress unsigned types and then we FoR compressor should kick in to convert things into unsigned and you will only ever see unsinged integers

The compressor actually already does this.

robert3005 · 2024-09-25T15:09:04Z

encodings/fastlanes/src/delta/compute.rs

+
+impl ScalarAtFn for DeltaArray {
+    fn scalar_at(&self, index: usize) -> VortexResult<Scalar> {
+        let decompressed = SliceFn::slice(self, index, index + 1)?.into_primitive()?;


just slice(self, index, index+1) is there a reason you want to do self.slice(index, index+1)?

I can't do self.slice because both ArrayCompute and SliceFn are in scope, but I could do slice(self, ...), the function. I'll do that.

I guess it does seem a bit silly (in terms of generated code) to do the whole rigamarole of getting an option, asserting its not None, just to call a function that I can statically resolve to SliceFn::slice. Thoughts on that?

hm fair, I haven't thought about arraycompute and individual functions colliding in scope. Fair to keep it as is

robert3005 · 2024-09-25T15:09:43Z

encodings/fastlanes/src/delta/compute.rs

+            physical_start % 1024,
+            limit,
+        )?;
+        if arr.len() != stop - start {


This is a weird assert in this place...

As I was developing it was helpful to ensure applying the offset & limit produced the correct length but I'll remove it now.

robert3005 · 2024-09-25T15:11:57Z

encodings/fastlanes/src/delta/compute.rs

+            new_deltas,
+            new_validity,
+            physical_start % 1024,
+            limit,


This should just be a number, and 0 should be perfectly fine value for it

Okay, done! The code does seem simpler now.

Can we explicitly specify length here instead of last chunk length?

robert3005 · 2024-09-25T15:14:11Z

encodings/fastlanes/src/delta/compute.rs

+        })?;
+
+        let new_deltas = deltas.with_dyn(|a| {
+            a.slice()


you want slice(deltas ...)

robert3005 · 2024-09-25T15:14:51Z

encodings/fastlanes/src/delta/compute.rs

+        let validity = self.validity();
+        let lanes = self.lanes();
+
+        let new_bases = bases.with_dyn(|a| {


you want slice(bases, ...)

robert3005 · 2024-09-25T15:18:58Z

encodings/fastlanes/src/delta/compress.rs

            array.validity()
        )
    });
-    Ok(decoded)
+    decoded
+        .slice(array.offset(), decoded.len() - array.trailing_garbage())?


You can use the original array length here that will be only the valid values, i.e. offset + len

danking

All addressed.

danking · 2024-09-30T13:40:52Z

encodings/fastlanes/src/delta/compress.rs

            array.validity()
        )
    });
-    Ok(decoded)
+    decoded
+        .slice(array.offset(), decoded.len() - array.trailing_garbage())?


danking · 2024-09-30T13:58:51Z

encodings/fastlanes/src/delta/compute.rs

+
+impl ScalarAtFn for DeltaArray {
+    fn scalar_at(&self, index: usize) -> VortexResult<Scalar> {
+        let decompressed = SliceFn::slice(self, index, index + 1)?.into_primitive()?;


I can't do self.slice because both ArrayCompute and SliceFn are in scope, but I could do slice(self, ...), the function. I'll do that.

I guess it does seem a bit silly (in terms of generated code) to do the whole rigamarole of getting an option, asserting its not None, just to call a function that I can statically resolve to SliceFn::slice. Thoughts on that?

danking · 2024-09-30T15:41:45Z

encodings/fastlanes/src/delta/compute.rs

+            physical_start % 1024,
+            limit,
+        )?;
+        if arr.len() != stop - start {


As I was developing it was helpful to ensure applying the offset & limit produced the correct length but I'll remove it now.

danking · 2024-09-30T15:50:08Z

encodings/fastlanes/src/delta/compute.rs

+            new_deltas,
+            new_validity,
+            physical_start % 1024,
+            limit,


Okay, done! The code does seem simpler now.

danking · 2024-09-30T16:14:59Z

encodings/fastlanes/src/delta/compress.rs

+    let array = if array.ptype().is_signed_int() {
+        match_each_signed_integer_ptype!(array.ptype(), |$T| {
+            assert!(array.statistics().compute_min::<$T>().map(|x| x >= 0).unwrap_or(false));
+        });
+        &array.reinterpret_cast(array.ptype().to_unsigned())
+    } else {
+        array
+    };
+


robert3005 · 2024-09-30T16:51:13Z

encodings/fastlanes/src/delta/mod.rs

@@ -68,7 +162,7 @@ impl DeltaArray {
    #[inline]
    pub fn deltas(&self) -> Array {
        self.as_ref()
-            .child(1, self.dtype(), self.len())
+            .child(1, self.dtype(), self.metadata().deltas_len)


you could use deltas_len() function here

robert3005 · 2024-09-30T16:51:34Z

encodings/fastlanes/src/delta/mod.rs

@@ -19,11 +22,86 @@ impl_encoding!("fastlanes.delta", ids::FL_DELTA, Delta);
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct DeltaMetadata {
    validity: ValidityMetadata,
-    len: usize,
+    deltas_len: usize,


As a followup we could get rid of this one, it always have to be ~~len rounded up to 1024~~ len + offset rounded up to 1024

This isn't true; in fact, this is what took me so long to wrap my head around this PR. Before this PR, if the logical length is not 0 mod 1024, then we encode the last chunk as one vector using scalar operations. After this PR, the above is still true except it depends on deltas.len() (which previously was always equal to the logical length).

oh, I somehow assumed it was the same as fastlanes bitpacking

More subtle than I expected. DeltaArray is a sequence of chunks. All chunks except the last must be "full", i.e. containing 1,024 values. The last chunk may contain as few as one value and is encoded differently from the rest. In this PR, I introduced an "offset" and a "limit". Together they enable logical/lazy slicing while preserving full chunks for later decompression. The offset is a value, less than 1024, which offsets into the first chunk. The limit is either `None` or less than 1024. `None` represents no limit which allows callers to avoid computing the length of the last chunk [1]. Internally, the limit is converted to a "right-offset": `trailing_garbage` which is sliced away when decompression happens. [1] Which is, a bit annoyingly, this: ``` match deltas.len() % 1024 { 0 => 1024, n => n } ```

1. logical_len is already stored as the array length. 2. trailing_garbage duplicates the information stored in the logical length.

fixup limit

danking requested a review from robert3005 September 25, 2024 14:59

danking marked this pull request as ready for review September 25, 2024 14:59

robert3005 requested changes Sep 25, 2024

View reviewed changes

danking force-pushed the dk/teach-delta-array-scalar-at-and-slice branch 2 times, most recently from ac4d207 to a0fd137 Compare September 30, 2024 15:49

danking commented Sep 30, 2024

View reviewed changes

danking requested a review from robert3005 September 30, 2024 16:17

danking enabled auto-merge (squash) September 30, 2024 16:18

robert3005 approved these changes Sep 30, 2024

View reviewed changes

danking added 14 commits September 30, 2024 13:07

fix docs

c09369c

remove logical_len and trailing_garbage from metadata

c3d1056

1. logical_len is already stored as the array length. 2. trailing_garbage duplicates the information stored in the logical length.

use the slice function rather than the trait method

f33192f

eliminate the option for limit

1ae75c0

fixup limit

remove length assertion

4b72272

remove the support for unsigned types in Delta

82a8fae

revert to origin/develop

6ee5b6c

revert to origin/develop

85ea4b0

revert to origin/develop

6c63957

restore origin/develop

836da47

directly use logical_len rather than limit

087b09d

fix up the comment

d17e3ec

prefer deltas_len() to metadata().deltas_len

1ed2261

danking force-pushed the dk/teach-delta-array-scalar-at-and-slice branch from 611e124 to 1ed2261 Compare September 30, 2024 17:08

danking merged commit 826cdf1 into develop Sep 30, 2024
5 checks passed

danking deleted the dk/teach-delta-array-scalar-at-and-slice branch September 30, 2024 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: teach DeltaArray slice and scalar_at #927

feat: teach DeltaArray slice and scalar_at #927

danking commented Sep 25, 2024

danking commented Sep 25, 2024

robert3005 left a comment

robert3005 Sep 25, 2024

robert3005 Sep 25, 2024

danking Sep 30, 2024

robert3005 Sep 25, 2024

danking Sep 30, 2024

robert3005 Sep 30, 2024

robert3005 Sep 25, 2024

danking Sep 30, 2024

robert3005 Sep 25, 2024

danking Sep 30, 2024

robert3005 Sep 30, 2024

robert3005 Sep 25, 2024

robert3005 Sep 25, 2024

robert3005 Sep 25, 2024

danking Sep 30, 2024

danking left a comment

danking Sep 30, 2024

danking Sep 30, 2024

danking Sep 30, 2024

danking Sep 30, 2024

danking Sep 30, 2024

robert3005 Sep 30, 2024

danking Sep 30, 2024

robert3005 Sep 30, 2024 •

edited

Loading

danking Sep 30, 2024

robert3005 Sep 30, 2024

feat: teach DeltaArray slice and scalar_at #927

feat: teach DeltaArray slice and scalar_at #927

Conversation

danking commented Sep 25, 2024

danking commented Sep 25, 2024

robert3005 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danking 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

robert3005 Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert3005 Sep 30, 2024 •

edited

Loading