Repository navigation
Run Widen arithmetic, cast, fill null, zip and between on the values - #10891
Conversation
Merging this PR will regress 2 benchmarks3 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
4c17aca to
cfa0805
Compare
| let cast_ptype = match (values.is_signed_int(), target.is_signed_int()) { | ||
| // `validate_dtypes` keeps the values narrower than 64 bits. | ||
| (false, true) => match values { | ||
| PType::U8 => PType::I16, | ||
| PType::U16 => PType::I32, | ||
| _ => PType::I64, | ||
| }, | ||
| (true, false) => values.to_unsigned(), | ||
| _ => values, | ||
| }; |
There was a problem hiding this comment.
This is basically trying to be clever, where we try to cast into a smaller bit width than we need to while remaining under Narrow.
Is this fine? We save 2x or more memory by doing this, but it could potentially cause even more work in the future, assuming some other operation cannot be pushed down into Narrow.
| } | ||
|
|
||
| /// Returns `array` executed to its canonical form. | ||
| fn canonical(array: &ArrayRef, ctx: &mut ExecutionCtx) -> VortexResult<ArrayRef> { |
robert3005
left a comment
robert3005
left a comment
There was a problem hiding this comment.
Apart from the some weird test code this looks fine
dffc937 to
6b937fd
Compare
connortsui20
changed the title
Narrow arithmetic, cast, fill null, zip and between on the valuesWiden arithmetic, cast, fill null, zip and between on the values
f114a89 to
16047c4
Compare
Checked add, subtract, multiply and divide of a Widen and another Widen or a constant now run at the smallest integer type that holds both operands and the result interval. When only the logical type holds the interval, the operation runs at the logical type, so overflow and division by zero fail exactly as canonical execution does. Any other operand declines. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
The cast rule now sends a float target to the values, and handles an integer target of the other signedness through a storage type of the target signedness. Fill null widens the values to the smallest type that holds the fill value. Zip runs on the values when the other branch is a Widen or a constant, with the Widen in either branch, through a ZipKernel and a second execute-parent kernel for the if_false branch. Between runs on the values when both bounds are constants inside their range, and as two compares of Widen when the bounds are constants or Widen arrays. The compare, zip and between kernels share the helpers for the wider value type and the constant cast in widen/compute/mod.rs, and the test modules share stored_ptype and canonical. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
…ernels Share one helper that casts values to a type and keeps their nullability, move the width choice of the arithmetic kernel into arithmetic_ptype, and give integer_storage_type and storage_ptype names that say what they return. Shorten the docs to the reasons. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Rename the shared helpers and the arithmetic helpers so that each name says what it returns, move constant_fits_values next to the other shared helpers now that between also calls it, and document where the arithmetic kernel relies on min and max statistics. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
ZipExecuteAdaptor dispatches only on the if_true branch, so the second kernel needed its own execute-parent impl and a branch enum. A Widen in the if_false branch now takes the default path, as every other encoding does. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Compare the complete error message without its captured backtrace. The canonical and Widen paths have different call stacks when CI enables backtraces, even when they return the same error. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
16047c4 to
419b196
Compare
Summary
Widenencoding for integers and decimals #10733This finishes the integer kernels for
Widen. Checked arithmetic on twoWidenarrays, or on aWidenand a constant, now runs at the smallest integer type that holds both operands and the result, instead of at the logical type. Cast, fill null, zip and between also run on the values.Based on #10890.
Changes
The arithmetic kernel reads the extrema of each operand and computes an interval that holds every result of add, subtract, multiply or divide. It runs the operation at the smallest type of the logical signedness that holds the operands and that interval. When only the logical type holds it, the operation runs at the logical type, so an overflow or a division by zero fails with the same error as canonical execution. Any other operand declines, so the default path widens a wide or lazy operand once. A brute-force test checks the interval against every result over small ranges.
Cast now also sends a float target, and an integer target of the other signedness, to the values. Fill null widens the values only as far as the fill value needs. Zip runs on the values when the
Widenis theif_truebranch and theif_falsebranch is aWidenor a constant.ZipExecuteAdaptordispatches only onif_true, so aWideninif_falsetakes the default path, as for every other encoding. Between runs on the values when both bounds are constants inside the range of the values, and as two compares on theWidenwhen the bounds are other constants orWidenarrays.Open question: since #10864, cast sends narrowing casts to the values, and this PR also sends sign-changing and float casts there. A failing cast fails on the same rows as canonical execution, but its message names the storage type, for example
i8tou8instead ofi64tou32. Is that acceptable, or should those casts decline?For review, read the commits one at a time.