feat: mask by danking · Pull Request #1900 · vortex-data/vortex

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

+            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

+        };
+
+        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

+        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

+
+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

+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

+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

+            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

+                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

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


Same problem here

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

    }

-    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:	`041e913`
Status:	✅ Deploy successful!
Preview URL:	https://72b23c2a.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.

danking · 2025-02-18T16:14:46Z

+    (0.01, 0.1),
+    (0.01, 0.01),
+])]
+fn bench_dict_mask(bencher: Bencher, (fraction_valid, fraction_masked): (f64, f64)) {


This is a useful microbenchmark but dict currently lacks a MaskFn so this just measures decompression time. I intend to follow up with a real MaskFn.

gatesn mentioned this pull request Jan 11, 2025

Expression partitioning to respect scope validity #1907

Closed

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

Closed

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

joseph-isaacs removed their request for review February 18, 2025 09:41

danking added 3 commits February 18, 2025 10:26

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

041e913

fixes

3ec13c0

test mask on dict array

554f492

danking commented Feb 18, 2025

View reviewed changes

danking added 2 commits February 18, 2025 11:23

revert change

3445488

StructArray children includes validity: must use fields()

5e2f415

gatesn approved these changes Feb 19, 2025

View reviewed changes

danking merged commit eadc1fe into develop Feb 19, 2025

danking deleted the dk/mask branch February 19, 2025 09:31

miniex mentioned this pull request Jun 26, 2026

fix(layout): preserve nullability of structs nested in structs #8590

Open

Uh oh!

Conversation

danking commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Specializations

Naming

Follow ups:

Uh oh!

danking commented Jan 13, 2025

Uh oh!

gatesn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloudflare-workers-and-pages Bot commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying vortex-bench with Cloudflare Pages

Uh oh!

danking commented Feb 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danking commented Jan 10, 2025 •

edited

Loading

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

edited

Loading