feat: mask #1900

danking · 2025-01-10T23:04:39Z

Mask sets entries of an array to null. I like the analogy to light: the array is a sequence of lights (each value might be a different wavelength). Null is represented by the absence oflight. Placing a mask (i.e. a piece of plastic with slits) over the array causes those values where the mask is present (i.e. "on", "true") to be dark.

An example in pseudo-code:

a = [1, 2, 3, 4, 5]
a_mask = [t, f, f, t, f]
mask(a, a_mask) == [null, 2, 3, null, 5]

Specializations

I only fallback to Arrow for two of the core arrays:

Sparse. I was skeptical that I could do better than decompressing and applying it.
Constant. If the mask is sparse, SparseArray might be a good choice. I didn't investigate.
Extension. (Now a follow up due to comments)

For the non-core arrays, I'm missing the following. I'm not clear that I can beat decompression forrun end. The others are easy enough but some amount of typing and testing.

fastlanes
fsst
roaring
runend
runend-bool
zigzag
ALP (Now a follow up due to comments)

Naming

Pandas also calls this operation mask but accepts an optional second argument which is an array of values to use instead of null (which makes Pandas' mask more like an if_else).

Arrow-rs calls this nullif.

Arrow-cpp has if_else(condition, consequent, alternate) and replace_with_mask(array, mask, replacements) both of which can implement our mask by passing a NullArray as the third argument.

Follow ups:

Extension array masking.
ALP array masking.
Add an expression for masking.
Assess the situation with nested structs in files and make sure we're preserving validity correctly.

Mask sets entries of an array to null. I like the analogy to light: the array is a sequence of lights (each value might be a different wavelength). Null is represented by the absence of light. Placing a mask (i.e. a piece of plastic with slits) over the array causes those values where the mask is present (i.e. "on", "true") to be dark. An example in pseudo-code: ```rust a = [1, 2, 3, 4, 5] a_mask = [t, f, f, t, f] mask(a, a_mask) == [null, 2, 3, null, 5] ``` Specializations --------------- I only fallback to Arrow for two of the core arrays: - Sparse. I was skeptical that I could do better than decompressing and applying it. - Constant. If the mask is sparse, SparseArray might be a good choice. I didn't investigate. For the non-core arrays, I'm missing the following. I'm not clear that I can beat decompression for run end. The others are easy enough but some amount of typing and testing. - fastlanes - fsst - roaring - runend - runend-bool - zigzag Naming ------ Pandas also calls this operation [`mask`](https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.mask.html) but accepts an optional second argument which is an array of values to use instead of null (which makes Pandas' mask more like an `if_else`). Arrow-rs calls this [`nullif`](https://arrow.apache.org/rust/arrow/compute/fn.nullif.html). Arrow-cpp has [`if_else(condition, consequent, alternate)`](https://arrow.apache.org/docs/cpp/compute.html#cpp-compute-scalar-selections) and [`replace_with_mask(array, mask, replacements)`](https://arrow.apache.org/docs/cpp/compute.html#replace-functions) both of which can implement our `mask` by passing a `NullArray` as the third argument.

danking · 2025-01-13T17:43:40Z

Nick, dict array no longer uses mul. It just iterates the indices of the mask.

gatesn

It would have been really helpful if this was done one PR per encoding.

gatesn · 2025-01-14T09:30:43Z

encodings/alp/src/alp/compute/mask.rs

+            array
+                .patches()
+                .map(|patches| {
+                    patches.map_values(|values| try_cast(&values, &values.dtype().as_nullable()))


This doesn't seem to apply the mask to the patches?

This is gone. When #1951 merges, I'll revisit ALP mask.

gatesn · 2025-01-14T09:33:05Z

encodings/datetime-parts/src/compute/cast.rs

+        };
+
+        Ok(DateTimePartsArray::try_new(
+            array.dtype().clone().as_nullable(),


This logic seems wrong? You're check for eq_ignore_nullability, but you don't check which direction the cast is, yet you always return a nullable DType.

Maybe pull this into a PR and add tests

Done: #1946

And now removed from this PR.

gatesn · 2025-01-14T09:36:30Z

encodings/dict/src/compute/mask.rs

+        FilterIter::Slices(slices) => {
+            for slice in slices {
+                for index in slice.0..slice.1 {
+                    codes[index] = T::zero();


I think this works?
codes[slice].fill(T::zero())

this works: codes[slice.0..slice.1].fill(T::zero()) (it needed to be a Range). Thanks, pushed!

gatesn · 2025-01-14T09:37:06Z

encodings/dict/src/compute/mask.rs

+
+impl MaskFn<DictArray> for DictEncoding {
+    fn mask(&self, array: &DictArray, mask: FilterMask) -> VortexResult<ArrayData> {
+        let new_codes = match_each_integer_ptype!(array.codes_ptype(), |$T| {


Don't you also have to check that the array was nullable before? Otherwise zero may not be the null value.

Good point. This is fixed in 373ae02 and a test was added.

gatesn · 2025-01-14T09:38:07Z

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

+impl CastFn<BoolArray> for BoolEncoding {
+    fn cast(&self, array: &BoolArray, dtype: &DType) -> VortexResult<ArrayData> {
+        let DType::Bool(new_nullability) = dtype else {
+            vortex_bail!(MismatchedTypes: "bool type", dtype);


I think an error message saying "cannot cast from X to Y" is probably better.

Done in e6bf8f0

gatesn · 2025-01-14T09:42:46Z

vortex-array/src/array/extension/compute/mask.rs

+impl MaskFn<ExtensionArray> for ExtensionEncoding {
+    fn mask(&self, array: &ExtensionArray, filter_mask: FilterMask) -> VortexResult<ArrayData> {
+        let DType::Extension(ext_dtype) = array.dtype() else {
+            vortex_bail!("extension array must have extension dtype");


Doesn't ExtensionArray have a ext_dtype() function? If not, it should. And it should panic, rather than return a result.

I reverted the extension changes in 757d1b8. I will revisit when #1965 (casting) merges.

gatesn · 2025-01-14T09:46:45Z

vortex-array/src/array/struct_/compute.rs

+            array.names().clone(),
+            array
+                .children()
+                .zip_eq(sdtype.dtypes())


This seems like wrong (or at the very least, undocumented) behavior. So we cast each field to the positionally equivalent DType in the destination type? But we preserve the names of the original array.

I would expect cast(array, dtype)?.dtype() == dtype. In fact, can you add this assertion in the cast entry-point function.=

I now require the names to match and added a test that shuffling the name order causes the cast to fail.

gatesn · 2025-01-14T09:48:07Z

vortex-array/src/array/varbin/compute/cast.rs

+                VarBinArray::try_new(
+                    array.offsets(),
+                    array.bytes(),
+                    array.dtype().with_nullability(*nullability),


You match on the target DType, but then you just change the nullability of the source DType. So VarBin(Binary).cast(UTF8?) => VarBin(Binary?) ??

gatesn · 2025-01-14T09:51:21Z

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

+                VarBinViewArray::try_new(
+                    array.views(),
+                    array.buffers().collect(),
+                    array.dtype().with_nullability(*nullability),


Same problem here

gatesn · 2025-01-14T10:22:20Z

vortex-array/src/compute/filter.rs

@@ -251,7 +275,7 @@ impl FilterMask {
        self.boolean_buffer().cloned()
    }

-    fn boolean_buffer(&self) -> VortexResult<&BooleanBuffer> {
+    pub fn boolean_buffer(&self) -> VortexResult<&BooleanBuffer> {


Why is this pub now?

This is now already public in develop.

cloudflare-workers-and-pages · 2025-02-03T20:03:59Z

Deploying vortex-bench with Cloudflare Pages

Latest commit:	`49d4a0d`
Status:	✅ Deploy successful!
Preview URL:	https://1cd8da2e.vortex-bench.pages.dev
Branch Preview URL:	https://dk-mask.vortex-bench.pages.dev

View logs

danking · 2025-02-03T20:50:36Z

Alright @gatesn , this is ready for another look.

gatesn mentioned this pull request Jan 11, 2025

Expression partitioning to respect scope validity #1907

Open

danking force-pushed the dk/mask branch from 814b4ab to 6b3b101 Compare January 13, 2025 17:41

danking changed the title ~~Dk/mask~~ feat: mask Jan 13, 2025

danking requested review from gatesn and joseph-isaacs January 13, 2025 17:43

danking marked this pull request as ready for review January 13, 2025 17:43

joseph-isaacs mentioned this pull request Jan 13, 2025

[wip] Add struct layout nullability #1922

Open

remove unnecssary clone

c25b668

danking mentioned this pull request Jan 13, 2025

feat: StructArrayTrait::field_by_idx #1925

Closed

clippy again

26c4613

danking enabled auto-merge (squash) January 13, 2025 20:18

gatesn requested changes Jan 14, 2025

View reviewed changes

danking mentioned this pull request Jan 14, 2025

fix: teach column and getitem to respect validity #1927

Closed

danking added 11 commits January 14, 2025 13:05

revert datetime-parts changes

469f401

extension arrays are neither castable nor maskable

757d1b8

Merge remote-tracking branch 'origin/develop' into dk/mask

e7de227

revert ALP mask for now

4bf1e0c

use slice::fill instead of a loop

2110ee2

fix: non-nullable dict array needs a leading empty element

373ae02

better error message when casting BoolArray to non-bool dtype.

e6bf8f0

test that changing name order is not allowed

bf2bbd2

remove DictArray mask for now

5b98893

fix varbin and varbinview casts

6d5af14

Merge remote-tracking branch 'origin/develop' into dk/mask

732171e

danking requested a review from gatesn January 21, 2025 12:15

danking added 3 commits January 21, 2025 12:18

clippy

af00e5c

clippy

5c43574

clippy

fe393c0

Merge remote-tracking branch 'origin/develop' into dk/mask

169fe33

danking added 6 commits February 3, 2025 15:15

remove cruft

64b714d

revert unused changes

cb0fabd

cleanups

77740e8

validity array not supporting min is an error

4d909eb

lift validity failure before boolean buffer creation

ac41d69

final fixups

efb9881

danking added 3 commits February 4, 2025 10:59

Merge remote-tracking branch 'origin/develop' into dk/mask

94c731c

Merge remote-tracking branch 'origin/develop' into dk/mask

49d4a0d

use new idioms for true count

256f013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: mask #1900

feat: mask #1900

danking commented Jan 10, 2025 •

edited

Loading

danking commented Jan 13, 2025

gatesn left a comment

gatesn Jan 14, 2025

danking Jan 16, 2025

gatesn Jan 14, 2025

gatesn Jan 14, 2025

danking Jan 14, 2025

danking Jan 14, 2025

gatesn Jan 14, 2025

danking Jan 16, 2025

gatesn Jan 14, 2025

danking Jan 16, 2025

gatesn Jan 14, 2025

danking Jan 16, 2025

gatesn Jan 14, 2025

danking Jan 16, 2025

gatesn Jan 14, 2025

danking Jan 21, 2025

gatesn Jan 14, 2025

danking Jan 21, 2025

gatesn Jan 14, 2025

danking Jan 21, 2025

gatesn Jan 14, 2025

danking Jan 21, 2025

cloudflare-workers-and-pages bot commented Feb 3, 2025 •

edited

Loading

danking commented Feb 3, 2025

feat: mask #1900

Are you sure you want to change the base?

feat: mask #1900

Conversation

danking commented Jan 10, 2025 • edited Loading

Specializations

Naming

Follow ups:

danking commented Jan 13, 2025

gatesn 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

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

cloudflare-workers-and-pages bot commented Feb 3, 2025 • edited Loading

Deploying vortex-bench with Cloudflare Pages

danking commented Feb 3, 2025

danking commented Jan 10, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 3, 2025 •

edited

Loading