Repository navigation
Decode DecimalByteParts at the canonical decimal width - #10905
connortsui20 wants to merge 3 commits into
Conversation
Merging this PR will degrade performance by 46.28%2 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
59eca0e to
881fe73
Compare
Every lower part of a DecimalByteParts array now has the dtype `u64`. Only the MSP may use a narrower signed integer dtype. The constructor, `assemble_decimal` and the v2 reader reject `u8`, `u16` and `u32` lower parts, and the error names `u64`. The constructor also bounds the lower-part count, so the parts never assemble wider than the canonical width of the decimal dtype. Precision up to 18 allows no lower parts, 19 to 38 at most one, and 39 to 76 up to three. `assemble_decimal` runs the same validation as the constructor. `DecimalByteParts::encode` now uses the checked constructor, because a decimal stored wider than its canonical width splits into more lower parts than its dtype allows. The unchecked constructor had no other caller, so it is removed. Assembly no longer casts lower parts to `u64`. The docs, tests and the `dbp_assemble_narrowed` bench now use `u64` lower parts. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
`assemble_decimal` now returns a decimal array at the canonical width of its dtype, whatever the width of the MSP. With no lower parts, an MSP at the canonical width is reused with no copy and no scan. A narrower MSP is widened into a copy, and a TODO marks where a later change can return a `Narrow` instead. A wider MSP is narrowed with `copy_to_canonical_width`, which checks every valid value against the precision and writes zeros in the null slots. One lower part assembles into `i128`, or straight into `i256` at a precision above 38. Two or three lower parts assemble into `i256`. The constructor bounds the lower parts by the canonical width, so assembly never narrows. Every result is built with the unchecked constructor, because its width is the canonical width and reads trust the precision. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
The CUDA DecimalByteParts decoder now returns a decimal array at the canonical width of its dtype, as the CPU decoder does. An MSP at the canonical width is reused with no copy. A narrower MSP is widened on the device with the decimal_cast kernels. An MSP wider than the canonical width returns an error: narrowing it must check the precision, and that check on the device needs an overflow flag that synchronizes with the host. The btrblocks scheme never writes such an MSP. Lower parts still return an error on the GPU. The device widening moves out of the Arrow export into a crate-private kernel module, so the export and the decoder share one entry point. That entry point rejects a narrowing cast. The decimal_cast kernels gain an i16 output, because an i8 MSP at precision 3 or 4 widens to i16. The GPU tests now assert the canonical width, and cover an i8 MSP at precision 3, an i64 MSP at precision 40, an i32 MSP at precision 9 that is reused, and an i64 MSP at precision 4 that is rejected. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
881fe73 to
fca52f8
Compare
|
So the regressions are definitely real, but without narrow decimal encoding there's not much we can do. I will note though that I think it is good we regress these benchmarks because now it actually shows how expensive a very large decimal is in its fully decoded representation. |
Summary
Decoding a DecimalByteParts array reuses the MSP buffer at whatever width the MSP has, so a
Decimal(18, 2)column with ani32MSP decodes to ani32-backedDecimalArray. Later steps of #10701 make the canonical width from #10897 (DecimalDType::canonical_values_type()) an invariant ofDecimalArray, so decoding has to produce it.This must not merge before a release publishes the compat fixtures from #10753, because only those fixtures let CI check that old files with narrow MSPs still decode. A local check of fixtures written by the base of this PR passes all 43, including
decimal_byte_parts_narrow_msp.vortex.Changes
assemble_decimalnow returns the canonical width. It reuses the MSP buffer when the MSP already has that width, widens a narrower MSP with a copy, and narrows a wider MSP withcopy_to_canonical_width, which fails on a valid value outside the precision. One lower part at precision 39 or more assembles straight intoi256. The widening copy is a pass that the old reuse didn't make, and thedbp_computebenches from #10752 will show its cost. ATODO(connor)at that branch notes the alternative of returning aNarrowover the MSP if that cost matters.Every lower part must now be
u64, and the number of lower parts is bounded by the precision (none up to precision 18, at most one up to 38), so assembly never needs to narrow. Nothing writes other lower parts today:split_decimalonly emitsu64parts, and no edition permits v2.DecimalByteParts::encodenow goes through the checked constructor, so a decimal stored wider than its canonical width can't produce more lower parts than its precision allows.The CUDA decoder follows the same rule. It widens a narrow MSP on the device with
decimal_cast, which moves out of the Arrow export into a shared module and gains ani16output, and it returns an error for an MSP wider than the canonical width. The CUDA code was not built locally, so it needs a review from someone who knows it.This is stacked on #10903, which is stacked on #10897. For review, read the commits one at a time.
API Changes
DecimalByteParts constructors and the v2 reader reject lower parts that are not
u64, and more lower parts than the precision allows. Decoding returns aDecimalArrayat the canonical width of its dtype.