Add wide decimal byte-part splitting and assembly logic - #9808
Conversation
Merging this PR will regress 3 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | arrow_checked_add_u32_neon[16384] |
12.2 µs | 20.3 µs | -39.77% |
| ❌ | Simulation | random_i16[0.95] |
77.5 µs | 94.3 µs | -17.82% |
| ❌ | WallTime | deferred_i64_avx2[PerRowPerRow] |
9.9 µs | 11.5 µs | -13.9% |
| ⚡ | Simulation | random_i8[0.5] |
91.4 µs | 66.7 µs | +37.01% |
| ⚡ | WallTime | arrow_checked_add_u32_avx2[16384] |
21.4 µs | 17.6 µs | +21.26% |
| ⚡ | Simulation | allocate_drop_arrow[0] |
456.9 ns | 402.7 ns | +13.45% |
| ⚡ | WallTime | filtered_owned_i64_avx512[OneNullInEight] |
26.2 µs | 23.2 µs | +12.91% |
| ⚡ | Simulation | chunked_bool_canonical_into[(1000, 10)] |
30.6 µs | 27.2 µs | +12.49% |
| ⚡ | WallTime | mul_u32_nonnull_avx512 |
6.2 µs | 5.6 µs | +10.37% |
| 🆕 | Simulation | dbp_assemble[(I128, 1024)] |
N/A | 47.9 µs | N/A |
| 🆕 | Simulation | dbp_assemble[(I128, 8192)] |
N/A | 266.7 µs | N/A |
| 🆕 | Simulation | dbp_assemble[(I256, 1024)] |
N/A | 80.5 µs | N/A |
| 🆕 | Simulation | dbp_assemble[(I256, 8192)] |
N/A | 530.3 µs | N/A |
| 🆕 | Simulation | dbp_assemble[(I64, 1024)] |
N/A | 9.2 µs | N/A |
| 🆕 | Simulation | dbp_assemble[(I64, 8192)] |
N/A | 7.3 µs | N/A |
| 🆕 | Simulation | dbp_split_all_valid[(I128, 1024)] |
N/A | 51.5 µs | N/A |
| 🆕 | Simulation | dbp_split_all_valid[(I128, 8192)] |
N/A | 286.7 µs | N/A |
| 🆕 | Simulation | dbp_split_all_valid[(I256, 1024)] |
N/A | 88.6 µs | N/A |
| 🆕 | Simulation | dbp_split_all_valid[(I256, 8192)] |
N/A | 549.7 µs | N/A |
| 🆕 | Simulation | dbp_split_all_valid[(I64, 1024)] |
N/A | 7.6 µs | N/A |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing mk/dbp-parts (41fbd80) with develop (e3b8eb2)2
Footnotes
-
224 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. ↩
-
No successful run was found on
mk/dbp-v2-feature(bffdca1) during the generation of this report, sodevelop(e3b8eb2) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
| let mut msp = BufferMut::<i64>::zeroed(values.len()); | ||
| let mut lower = BufferMut::<u64>::zeroed(values.len()); |
There was a problem hiding this comment.
Sure I can get rid of this case, but curious why? speeds up compression of arrays with significant # nulls (see benches)
There was a problem hiding this comment.
zeroed < using with_capacity
| fn split_i128(values: &Buffer<i128>, validity: &Mask) -> (Buffer<i64>, Buffer<u64>) { | ||
| if validity.all_true() { | ||
| let mut msp = BufferMut::<i64>::with_capacity(values.len()); | ||
| let mut lower = BufferMut::<u64>::with_capacity(values.len()); | ||
| for value in values.iter() { | ||
| msp.push((value >> LOWER_PART_BITS) as i64); | ||
| lower.push(*value as u64); | ||
| } | ||
| return (msp.freeze(), lower.freeze()); | ||
| } | ||
|
|
||
| // Lower parts are stored as non-nullable arrays, so use zeros at null positions instead | ||
| // of copying their garbage values. | ||
| let mut msp = BufferMut::<i64>::zeroed(values.len()); | ||
| let mut lower = BufferMut::<u64>::zeroed(values.len()); | ||
|
|
||
| if let Mask::Values(valid) = validity { | ||
| let msp = msp.as_mut_slice(); | ||
| let lower = lower.as_mut_slice(); | ||
| valid.bit_buffer().for_each_set_index(|i| { | ||
| let value = values[i]; | ||
| msp[i] = (value >> LOWER_PART_BITS) as i64; | ||
| lower[i] = value as u64; | ||
| }); | ||
| } | ||
| (msp.freeze(), lower.freeze()) |
There was a problem hiding this comment.
can we not use const generics here
There was a problem hiding this comment.
yes, what did you have in mind?
Introduce typed splitting and reassembly for i128 and i256 decimals, including sign extension and boundary tests. Route existing single-part canonicalization through the same assembly helper without changing its wire representation. Signed-off-by: "Matt Katz" <mhkatz97@gmail.com>
Resolve validity once for wide storage and populate only valid rows in zero-initialized part buffers, so arbitrary null payloads do not inflate lower-part compression. Preserve the all-valid loops and narrow zero-copy path. Cover sliced, empty, nullable, and wider-than-precision storage. Signed-off-by: "Matt Katz" <mhkatz97@gmail.com>
Keep i256 words in most-significant-first order and assemble the two 128-bit halves directly. Dispatch on lower-part count, validate signed MSPs and equal child lengths, and cover word order and sign extension. Signed-off-by: "Matt Katz" <mhkatz97@gmail.com>
Keep the splitting and assembly helpers independent of the array changes in the next PR. Signed-off-by: "Matt Katz" <mhkatz97@gmail.com>
| for value in values.iter() { | ||
| msp.push((value >> LOWER_PART_BITS) as i64); | ||
| lower.push(*value as u64); | ||
| } |
There was a problem hiding this comment.
push is very slower. likely we want want to zip values and all buffers and write each one by one https://github.com/vortex-data/vortex/pull/9796/changes#diff-a8d2d08ea97410ea3fe9c897077ccc5871bddd10600b762be4ff36b75e043d53R151
| if let Mask::Values(valid) = validity { | ||
| let msp = msp.as_mut_slice(); | ||
| let mut lower = lower.each_mut().map(BufferMut::as_mut_slice); | ||
| valid.bit_buffer().for_each_set_index(|i| { | ||
| let [msp_word, lower_words @ ..] = i256_to_words(values[i]); | ||
| msp[i] = msp_word.cast_signed(); | ||
| for (part, word) in lower.iter_mut().zip(lower_words) { | ||
| part[i] = word; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
looks very slow with this iter
| /// Split an `i256` into four `u64` words, most significant first. | ||
| #[inline] | ||
| const fn i256_to_words(value: i256) -> [u64; 4] { | ||
| let (low, high) = value.to_parts(); | ||
| #[expect( | ||
| clippy::cast_possible_truncation, | ||
| reason = "each cast takes the low 64 bits of a word pair by construction" | ||
| )] | ||
| [ | ||
| (high >> LOWER_PART_BITS) as u64, | ||
| high as u64, | ||
| (low >> LOWER_PART_BITS) as u64, | ||
| low as u64, | ||
| ] | ||
| } |
There was a problem hiding this comment.
because we're splitting the i256 to 64 bit words -- high i64 and lows u64? Just changed to return (i64, [u64]) tho so we dont cast msp
| /// For each valid row, the original value is `msp * 2^64 + lower`. Invalid rows are zeroed. | ||
| #[expect( | ||
| clippy::cast_possible_truncation, | ||
| clippy::cast_sign_loss, | ||
| reason = "splitting a wide integer into 64-bit windows truncates by construction" |
There was a problem hiding this comment.
please move this to the lines it matches
| Ok(match lower.as_slice() { | ||
| [first] => DecimalArray::new(assemble_i128(msp, first), decimal_dtype, validity), | ||
| [first, second] => { | ||
| DecimalArray::new(assemble_i256(msp, [first, second]), decimal_dtype, validity) | ||
| } | ||
| [first, second, third] => DecimalArray::new( | ||
| assemble_i256(msp, [first, second, third]), | ||
| decimal_dtype, | ||
| validity, | ||
| ), | ||
| _ => vortex_bail!( | ||
| "at most {MAX_LOWER_PARTS} lower parts are supported, got {}", | ||
| lower.len() | ||
| ), | ||
| }) | ||
| } | ||
|
|
||
| /// Reassemble a signed MSP and one `u64` lower part into `i128` values. | ||
| /// | ||
| /// For each row, the result is `msp * 2^64 + lower`. | ||
| #[expect( |
There was a problem hiding this comment.
seems like we can have a single assemble::<1>, 2 so on and move that loop to a compile time one
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Measure split_decimal and assemble_decimal directly across storage widths and input sizes, with fixtures outside the timed calls. Cover all-valid, all-null, random-null, and clustered-null split inputs. Signed-off-by: "Matt Katz" <mhkatz97@gmail.com>
DCO Remediation Commit for Matt Katz <mhkatz97@gmail.com> I, Matt Katz <mhkatz97@gmail.com>, hereby add my Signed-off-by to this commit: 3877465 I, Matt Katz <mhkatz97@gmail.com>, hereby add my Signed-off-by to this commit: 966977b I, Matt Katz <mhkatz97@gmail.com>, hereby add my Signed-off-by to this commit: 643f4b1 I, Matt Katz <mhkatz97@gmail.com>, hereby add my Signed-off-by to this commit: d93444d I, Matt Katz <mhkatz97@gmail.com>, hereby add my Signed-off-by to this commit: 1f2143a Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Add support for splitting decimals stored as
i128ori256into primitive parts and assembling them back into decimal arrays. Decimals stored asi8,i16,i32, ori64continue to use a single part.Splitting divides each value into 64-bit words ordered most significant first:
i8,i16,i32,i64i128i64MSP holding the high 64 bits, followed by oneu64lower part.i256i64MSP holding the high 64 bits, followed by threeu64lower parts.Assembly reconstructs the value according to the number of lower parts:
i128.i256. Sign-extend the MSP to form the high 128 bits.i256. The remaining two parts form the low 128 bits.The MSP carries the sign and null mask. Lower parts are non-nullable, and splitting writes zeroes at null positions so arbitrary null-slot bytes do not affect their compression.
Added
#[inline]toi256'sShl<usize>because disassembly of the release benchmark otherwise shows three out-of-line shift calls per row when assembling three lower parts; with it, the shifts simplify to word loads and stores. On local ARM64, the currentI256assembly benchmarks built with--profile releasetook 0.917 µs with inline versus 6.207 µs without for 1,024 rows, and 6.332 µs versus 48.540 µs for 8,192 rows. These are medians of five alternating runs per version, corresponding to approximately 6.8× and 7.7× faster assembly with inline.