Repository navigation
Execute interleaved decimal arrays - #10940
Conversation
Merging this PR will regress 5 benchmarks2 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
2fbe1e5 to
98dea22
Compare
98dea22 to
bd15c87
Compare
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Prepare each decimal source as one typed gather input, with an early return for constants. Keep buffer normalization separate from selector dispatch. Parameterize bounds-check cases and report assertion failures at their callers. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
The module imports `Interleave`, so rustdoc rejects the explicit link target as redundant. The source comment now covers both the widening and the narrowing copy, and the tests share one module session. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
…dispatch Primitive and decimal execution both repeated the dispatch over the two selector types. `gather` now owns it, and the slice kernel becomes `gather_rows`. `Source::constant` and `Source::decoded` set `row_mask` and the row count, so callers no longer write these fields by hand. The unit test now also covers the row check of a decoded source. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
The decimal tests repeated cases that other tests already cover. The scale and nullability dimensions do not reach decimal code, the selector bounds checks belong to the shared `gather`, and `copy_to_canonical_width` has tests for each width. The remaining tests cover each canonical width, constants stored at other widths, sources stored at other widths, empty input and a value outside the precision. The lazy-children test in `interleave/mod.rs` now covers the decimal kernel too. With fewer tests, the kernel is a flat `decimal.rs` like `bool.rs` and `primitive.rs`. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
7d99ff3 to
c0eb004
Compare
| let Some(source) = values.get((*branch).as_()) else { | ||
| vortex_bail!("interleave array index out of bounds"); | ||
| }; |
There was a problem hiding this comment.
Im pretty sure we want to panic here instead of erroring, that would be consistent with how dict take works.
I'll have a followup PR on this later (and I think that runend and chunked take should also panic on OOB instead of erroring)
There was a problem hiding this comment.
ok nvm this is actually the correct behavior, we want dict take to error instead
There was a problem hiding this comment.
this is mostly a refactor of the current primitive.rs in this module
Once every `DecimalArray` stores its values at the canonical width of its dtype (#10701), all sources of a decimal `Interleave` share that width and the copy branch cannot run. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
WD1 to WD4, CD3, CD3b and the decimal Interleave kernel merged, and CD4, CD5 and WD6 are open drafts. The plan now lists CD3b (#10939) and the Interleave kernel (CD4p, #10940) as dependencies of CD4, and CD3b as a dependency of CD6 and CD7. CD7 merges right after CD5 (#10905). D26 moves to CD6, because CD4 (#10946) adds no serialization lowering: the default writer executes a Widen, and reads of old files are the first ordinary source of one. The CD6 and WR1 briefs now carry that choice. D35 no longer makes WD6 wait for WD5. #10908 joins the follow-up issues, and the plan suggests settling the mechanism of Signed-off-by: Connor Tsui <connor.tsui20@gmail.com> #10748 before WD5.
Summary
Interleaveaccepts decimal values but panics when it executes them, because only the bool and primitive kernels exist. DecimalWiden(CD4) falls back to canonical decimals for interleave, so it needs a decimal kernel first.Changes
Most of the diff is a refactor of the primitive kernel: its gather moves into a
fixed_widthmodule next to the kernels, so decimals reuse it instead of getting a second copy. Canonical decimals are fixed-width buffers just like primitives, so one set of selector bounds checks and one tuned gather loop can serve both. The gather stays ininterleaveand not inarrays/fixed_widthbecause it owns the selector contract ofInterleaveand has no other caller.Decimal sources stored at a non-canonical width are copied to the canonical width before the gather. That copy goes away once CD8a stores every decimal at its canonical width.
API Changes
Interleaveexecutes decimal values and returns them at the canonical width of their dtype, where it used to panic.