ESQL: Method to convert BooleanBlock to a "mask" #112253
Conversation
This adds a method, `BooleanBlock#toMask` to convert `BooleanBlock`s into a "mask" for use with `keepMask`.
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| $elseif(boolean)$ | ||
| /** | ||
| * Convert this to a {@link BooleanVector "mask"} that's appropriate for | ||
| * passing to {@link #keepMask}. |
There was a problem hiding this comment.
Nit: let's describe what to expect in case of null values and multivalues.
| /** | ||
| * Result from calling {@link BooleanBlock#toMask}. {@link #close closing} this will | ||
| * close the contained {@link #mask()}. If you want to keep a reference to it then you'll | ||
| * have to {@link Block#incRef()} it. |
There was a problem hiding this comment.
I think this doesn't implement incref/RefCounted/AbstractNonThreadSafeRefCounted.
There was a problem hiding this comment.
Block's ref counted - this is sort of like a single reference to it. Not sure if that's clear though.
x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/data/ToMask.java
Show resolved
Hide resolved
| /** | ||
| * Convert this to a {@link BooleanVector "mask"} that's appropriate for | ||
| * passing to {@link #keepMask}. | ||
| */ |
There was a problem hiding this comment.
Nit: This is duplicated in most of the templates, but having this javadoc on the boolean block/vector interfaces should suffice. (Just tested this for ConstantNullBlock, which doesn't have this javadoc comment, and when hovering over toMask I get the correct javadoc from the interface.)
| } | ||
| } | ||
| assertThat(blockFactory.breaker().getUsed(), equalTo(0L)); | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: there's 2 ways of having null values in an array block:
- the nullsmask is explicitly set at the position
- the value count is just 0.
I'm not sure if the added tests cover toMask in the second case.
There was a problem hiding this comment.
The test never makes 0 value count. Also! Those should mean the same thing.
There was a problem hiding this comment.
That's my point though: for good measure, I think the a test should make a block with a 0 value count at p (probably without setting the nulls mask at p); toMask should still work. (I'm pretty sure it does rn, but tests are better proof than my word!)
Last time I went digging in these parts of the code (when I added the invariant check for array blocks), there were rare instances where we actually made 0 value counts (by setting the first index for p and p+1 to the same value). Not sure if that's still the case, but I think we at least allow it.
There was a problem hiding this comment.
I'll open an PR. I think at this point not setting the null mask but setting the value count is an error. All kinds of things will blow up.
This adds a method, `BooleanBlock#toMask` to convert `BooleanBlock`s into a "mask" for use with `keepMask`.
This adds a method, `BooleanBlock#toMask` to convert `BooleanBlock`s into a "mask" for use with `keepMask`.
This adds a method,
BooleanBlock#toMaskto convertBooleanBlocks into a "mask" for use withkeepMask.