Attach RowFn validity directly to canonical output - #9694
Conversation
Merging this PR will degrade performance by 17%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | arrow_checked_add_u32_avx2[16384] |
17.7 µs | 21.3 µs | -17% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ct/row-fn-canonical-validity (8208687) with ct/primitive-comparison-simd (6bb6d93)3
Footnotes
-
206 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
4 benchmarks were run, but are now archived. If they were deleted in another branch, consider rebasing to remove them from the report. Instead if they were added back, click here to restore them. ↩
-
No successful run was found on
ct/primitive-comparison-simd(d7f1683) during the generation of this report, so 4e4e412 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
84cde9e to
c72fbbe
Compare
c72fbbe to
8df4c19
Compare
|
generally not a fan of optimisations like these... |
|
I think it shows a gap in our abstraction, ideally this is not required for this optimization? |
8df4c19 to
b464dd5
Compare
b464dd5 to
f17be3d
Compare
f17be3d to
89e5590
Compare
| Validity::Array(validity_array) => { | ||
| let values = if let Some(canonical) = values.as_opt::<AnyCanonical>() { | ||
| mask_validity_canonical( | ||
| Canonical::from(canonical), | ||
| Validity::Array(validity_array), | ||
| ctx, | ||
| )? | ||
| .into_array() | ||
| } else { | ||
| values.mask(validity_array)? | ||
| }; | ||
|
|
||
| self.finalize_output(values, self.row_count) | ||
| } |
There was a problem hiding this comment.
There's something sketchy in the profile here and I would have to trace more of the code to figure out what's wrong. In the develop version we seem to execute the mask while in the new one we don't. Ah... I think this is because the benchmark is executing to AnyCanonical and this change here immediately flattens the mask into value instead of doing another execution round. Ultimately I think this is not useful benchmaxxing.
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
89e5590 to
8208687
Compare
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Summary
Attaches input-derived validity directly when a RowFn kernel already returns a canonical array, avoiding an extra lazy
vortex.maskexecution. Non-canonical output keeps the existing lazy mask path.Changes
Reuses
mask_validity_canonicalwithout copying canonical value buffers and preserves kernel-output validation, output relabeling, and outer nullability. Structural Boolean and primitive tests verify that nullable dense execution no longer runsvortex.mask.