Repository navigation
fix: avoid fill_null crashes on lazy fill values - #10576
robert3005 merged 2 commits into
Conversation
Execution now evaluates a fill that is not already a constant until it becomes one, then reads the scalar. The parent reduce and execute kernels return no match when the fill child is not constant, leaving that case to execution. A fill that is still not a scalar afterward still fails with the constant-or-scalar error. Tests cover a lazy literal fill and a varying fill that the kernels decline. Fixes vortex-data#10543 Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Merging this PR will degrade performance by 6.37%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ |
Simulation | random_i128[0.01] |
3.8 µs | 4.7 µs | -19.08% |
| ❌ | Simulation | sum_v2_i64 |
194.2 µs | 224.9 µs | -13.66% |
| ❌ | Simulation | sum_i64 |
194.7 µs | 225.1 µs | -13.52% |
| ❌ | WallTime | bitpack_blocked_compress_avx2 |
6.7 µs | 7.6 µs | -11.69% |
| ⚡ | WallTime | starts_with_avx2[8, VarBin] |
46.6 µs | 41.1 µs | +13.4% |
| ⚡ | Simulation | isolated_runs[u32, (16, 0)] |
87.5 µs | 78.6 µs | +11.33% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing mvanhorn:fix/10543-fill-null-lazy-fill-values (3aa9df0) with develop (5fc5cb8)
Footnotes
-
12 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. ↩
When the fill value was lazy, execute it to a constant and hand the rebuilt FillNull node back through the optimizer, so the input's own FillNull kernels get a chance before the input is decoded. A test checks that a chunked input stays chunked. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Summary
Filling nulls panics when the replacement value is a lazy scalar. The failure reports that the fill value must be a constant or scalar. Execution required the fill argument to already be a constant and rejected it when that check found nothing. A lazy scalar, such as an unevaluated literal, becomes a constant only after it runs, so it took that failure path. The parent kernels demanded the same thing with vortex_expect, which panics when the fill child is not already constant.
Execution now evaluates a fill that is not already a constant until it becomes one, then reads the scalar. The parent reduce and execute kernels return no match when the fill child is not constant, leaving that case to execution. A fill that is still not a scalar afterward still fails with the constant-or-scalar error. Tests cover a lazy literal fill and a varying fill that the kernels decline.
cargo test -q --manifest-path vortex-array/Cargo.toml -p vortex-array passed the vortex-array library tests with 4351 passed, 0 failed, and 1 ignored.
That command passed the next test target with 77 passed, 0 failed, and 21 ignored.
That command passed the remaining test target with 1 passed and 0 failed.
That command finished the doctests.
Fixes #10543
Changes
Covered in the summary above.
API Changes
Uncomment this section if there are user-facing changes, such as a broken public API or a change in the behavior of an engine integration. A breaking change also needs the
changelog/breaklabel.-->
AI was used for assistance.