Add `ByteBoolArray` type and fixe a bug in `BoolArray` #383

AdamGS · 2024-06-17T17:34:32Z

The main two contributions in this PR are:

A new ByteBoolArray type (closes Add a ByteBoolArray #335)
An important fix for slicing and initialization of BoolArray

gatesn · 2024-06-17T17:47:45Z

vortex-array/src/array/bool/compute/scalar_at.rs

@@ -9,7 +9,12 @@ use crate::ArrayDType;
 impl ScalarAtFn for BoolArray {
    fn scalar_at(&self, index: usize) -> VortexResult<Scalar> {
        if self.is_valid(index) {
-            Ok(self.boolean_buffer().value(index).into())
+            let value = self.boolean_buffer().value(index);
+            let s = Scalar::new(


I wonder if we should wrap the native type -> scalar conversions in #cfg(test), they're just too dangerous

Honestly already pretty hard to work with scalars IMO, but I don't have a much better idea here.

Maybe an improved API is Scalar::bool(...) and Scalar::nullable_bool(...) and we just explicitly write out the constructors.

gatesn · 2024-06-18T07:39:41Z

vortex-array/src/array/bool/mod.rs

+        let buffer_len = buffer.len();
+        let buffer_offset = buffer.offset();
+
+        let inner = buffer.into_inner().bit_slice(buffer_offset, buffer_len);


So I think this only partially fixes the bug. The problem is that the BoolArray itself doesn't hold an offset, meaning the first bit is always considered to be byte aligned.

Maybe in another PR can fix for real by adding a bit_offset (from 0..8) field to BoolMetadata. The SliceFn compute function and maybe others must also respect it.

bit_slice actually copies the buffer if the offset isn't byte aligned so this operation isn't as cheap as it used to be. Whether that's a reasonable tradeoff is up to you.
I do agree with the second point, I'm obviously not familiar with every part of the codebase here but taking the offset logic management into vortex will probably be valuable.

Yeah, the general idea is that construction and SliceFn should be constant time and not re-allocate. Which is why lots of arrays hold offsets.

Reminds me, we should have some logic to slice out those offsets during serialization!

gatesn · 2024-06-18T07:40:24Z

vortex-array/src/array/byte_bool/compute/mod.rs

+};
+
+impl ArrayCompute for ByteBoolArray {
+    fn as_arrow(&self) -> Option<&dyn crate::compute::as_arrow::AsArrowArray> {


Nit: move the fully qualified names to imports

gatesn · 2024-06-18T07:42:56Z

vortex-array/src/array/byte_bool/compute/mod.rs

+
+        let scalar = match self.is_valid(index).then(|| self.buffer()[index] == 1) {
+            Some(b) => Scalar::new(
+                vortex_dtype::DType::Bool(self.validity().nullability()),


This can also be self.dtype().clone() right?

gatesn · 2024-06-18T07:43:07Z

vortex-array/src/array/byte_bool/compute/mod.rs

+        let scalar = match self.is_valid(index).then(|| self.buffer()[index] == 1) {
+            Some(b) => Scalar::new(
+                vortex_dtype::DType::Bool(self.validity().nullability()),
+                vortex_scalar::ScalarValue::Bool(b),


Remove fully qualified

gatesn · 2024-06-18T07:53:15Z

vortex-array/src/array/byte_bool/mod.rs

+                length,
+            },
+            Some(buffer),
+            validity.into_array_data().into_iter().collect_vec().into(),


I think this should just be vec![validity.into_array_data()]?

No, took this snippet from BoolArray, it can probably be simplified but I suspect that's going to take a bigger change

gatesn · 2024-06-18T07:54:13Z

vortex-array/src/array/byte_bool/mod.rs

+impl From<Vec<bool>> for ByteBoolArray {
+    fn from(value: Vec<bool>) -> Self {
+        let value_len = value.len();
+        Self::try_with_validity(value, NullBuffer::new_valid(value_len)).unwrap()


Just a note, this will create a nullable bool array I think? Not sure if that was intended?

gatesn · 2024-06-18T07:54:59Z

vortex-array/src/array/byte_bool/mod.rs

+    }
+}
+
+impl AsRef<[bool]> for ByteBoolArray {


Yeah, this is the one I don't like. These damn nulls!

gatesn · 2024-06-18T07:55:07Z

vortex-array/src/array/byte_bool/mod.rs

+
+impl ArrayTrait for ByteBoolArray {
+    fn len(&self) -> usize {
+        self.metadata().length


self.buffer().len()

gatesn · 2024-06-18T07:56:29Z

vortex-array/src/array/byte_bool/stats.rs

+        }
+
+        match self.logical_validity() {
+            LogicalValidity::AllValid(len) => Ok(all_true_bool_stats(len)),


I don't think this works? Just because all values are non-null, doesn't mean all values are true.

gatesn · 2024-06-18T08:24:01Z

The other high-level comment, we should move this encoding into a crate. The main vortex-array encodings are (mostly) only those that convert 1:1 to Arrow. Anything else is an "extension"

AdamGS · 2024-06-24T10:21:34Z

Pushed fixes for most comment, I think stats are the last thing I need to fix here

robert3005

Only note about changing constructor though could be convinced that we should keep it as is

robert3005 · 2024-06-24T18:03:38Z

encodings/byte_bool/src/compute/mod.rs

+            validity: validity.to_metadata(length)?,
+        });
+
+        ArrayData::try_new(


You generally want to go through the typed constructor where possible. So you want a
try_new(buffer: Buffer, validity: Validity) -> Self and then rename existing try_with_validity to from_vec?

That makes sense, introduced try_new and renamed to try_from_vec

robert3005 · 2024-06-24T18:22:06Z

encodings/byte_bool/src/stats.rs

+        }
+
+        let bools = self.as_array_ref().clone().into_canonical()?.into_bool()?;
+        bools.compute_statistics(stat)


note for future this could be optimized to avoid copy but for now this is enough

added a TODO comment

AdamGS · 2024-06-24T18:48:43Z

~~Seems like one of my recent changes broke tests, looking into~~ Fixed

AdamGS added 19 commits June 14, 2024 12:48

Initial work on ByteBoolArray

49b9911

Some initial compute traits for ByteBoolArray

56ddb16

fix slice bug

3b73add

Fix bug in AsArrowaArray conversion for ByteBoolArray

ab85b89

Add 'Take' implementation and some other changes and fixes

90c77b5

Some refactoring and tests

bc9dae1

Small change

2e27c79

Filling up the ArrayFlatten implementation

69ba0ae

this is actually correct

c930191

Some more details

163ae67

.

4bc1584

Merge branch 'spiraldb:develop' into develop

af0e8bf

I was trying to be too smart for my own good

db969f6

This is more idiomatic

1c8398f

Add CompareFn

8e89a4d

Add 'FillForwardFn' implementation

a4ebeff

Cleanup test

cdee0e2

temp

cd8fd9b

Fix bug in BoolArray slicing

0bb9bb3

AdamGS changed the title ~~[WI{Add ByteBoolArray type and fixes a bug in BoolArray slicing~~ [WIP] Add ByteBoolArray type and fixes a bug in BoolArray slicing Jun 17, 2024

AdamGS changed the title ~~[WIP] Add ByteBoolArray type and fixes a bug in BoolArray slicing~~ [WIP] Add ByteBoolArray type and fixe a bug in BoolArray slicing Jun 17, 2024

AdamGS changed the title ~~[WIP] Add ByteBoolArray type and fixe a bug in BoolArray slicing~~ [WIP] Add ByteBoolArray type and fixe a bug in BoolArray Jun 17, 2024

AdamGS added 2 commits June 17, 2024 20:41

Cleanup

3f1e8d6

Stats for ByteBoolArray

7097776

gatesn reviewed Jun 18, 2024

View reviewed changes

AdamGS added 3 commits June 24, 2024 12:10

Merge branch 'develop' into develop

9552f65

Some CR notes

7b417ce

More CR notes and moving things around

0f3b281

AdamGS added 3 commits June 24, 2024 13:23

fix From impl

0c5b203

Fallback to BoolArray stats and add some other tests

7a1a434

Merge branch 'develop' into develop

78d0f37

AdamGS requested a review from gatesn June 24, 2024 15:46

Resolve merge conflits and utilize new APIs

4d8ee5f

robert3005 reviewed Jun 24, 2024

View reviewed changes

More CR notes

751a153

robert3005 approved these changes Jun 24, 2024

View reviewed changes

Fix tests, was missing a buffer

0110c80

AdamGS changed the title ~~[WIP] Add ByteBoolArray type and fixe a bug in BoolArray~~ Add ByteBoolArray type and fixe a bug in BoolArray Jun 24, 2024

AdamGS marked this pull request as ready for review June 24, 2024 19:41

robert3005 approved these changes Jun 24, 2024

View reviewed changes

robert3005 merged commit e43ec57 into spiraldb:develop Jun 24, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ByteBoolArray` type and fixe a bug in `BoolArray` #383

Add `ByteBoolArray` type and fixe a bug in `BoolArray` #383

AdamGS commented Jun 17, 2024 •

edited

Loading

gatesn Jun 17, 2024

AdamGS Jun 18, 2024

gatesn Jun 18, 2024

gatesn Jun 18, 2024

AdamGS Jun 18, 2024

gatesn Jun 18, 2024

gatesn Jun 18, 2024

gatesn Jun 18, 2024

gatesn Jun 18, 2024

gatesn Jun 18, 2024

AdamGS Jun 24, 2024

gatesn Jun 18, 2024

gatesn Jun 18, 2024

AdamGS Jun 24, 2024

gatesn Jun 18, 2024

gatesn Jun 18, 2024

AdamGS Jun 18, 2024

gatesn commented Jun 18, 2024

AdamGS commented Jun 24, 2024

robert3005 left a comment

robert3005 Jun 24, 2024

AdamGS Jun 24, 2024

robert3005 Jun 24, 2024

AdamGS Jun 24, 2024

AdamGS commented Jun 24, 2024 •

edited

Loading

Add ByteBoolArray type and fixe a bug in BoolArray #383

Add ByteBoolArray type and fixe a bug in BoolArray #383

Conversation

AdamGS commented Jun 17, 2024 • edited Loading

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

Choose a reason for hiding this comment

gatesn commented Jun 18, 2024

AdamGS commented Jun 24, 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

AdamGS commented Jun 24, 2024 • edited Loading

Add `ByteBoolArray` type and fixe a bug in `BoolArray` #383

Add `ByteBoolArray` type and fixe a bug in `BoolArray` #383

AdamGS commented Jun 17, 2024 •

edited

Loading

AdamGS commented Jun 24, 2024 •

edited

Loading