Skip to content

Add wide decimal byte-part splitting and assembly logic - #9808

Open
mhk197 wants to merge 7 commits into
mk/dbp-v2-featurefrom
mk/dbp-parts
Open

Add wide decimal byte-part splitting and assembly logic#9808
mhk197 wants to merge 7 commits into
mk/dbp-v2-featurefrom
mk/dbp-parts

Conversation

@mhk197

@mhk197 mhk197 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Add support for splitting decimals stored as i128 or i256 into primitive parts and assembling them back into decimal arrays. Decimals stored as i8, i16, i32, or i64 continue to use a single part.

Splitting divides each value into 64-bit words ordered most significant first:

Decimal storage Split representation
i8, i16, i32, i64 Reuse the original signed buffer as the most significant part (MSP), with no lower parts.
i128 An i64 MSP holding the high 64 bits, followed by one u64 lower part.
i256 An i64 MSP holding the high 64 bits, followed by three u64 lower parts.

Assembly reconstructs the value according to the number of lower parts:

  • None: reuse the MSP buffer as decimal storage, without copying.
  • One: combine the MSP and lower part into an i128.
  • Two: the lower parts form the low 128 bits of an i256. Sign-extend the MSP to form the high 128 bits.
  • Three: the MSP and first lower part form the high 128 bits of an 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] to i256's Shl<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 current I256 assembly benchmarks built with --profile release took 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.

@codspeed-hq

codspeed-hq Bot commented Sep 8, 2026

Copy link
Copy Markdown

Merging this PR will regress 3 benchmarks

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 6 improved benchmarks
❌ 3 regressed benchmarks
✅ 2178 untouched benchmarks
🆕 18 new benchmarks
⏩ 224 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

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

Open in CodSpeed

Footnotes

  1. 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.

  2. No successful run was found on mk/dbp-v2-feature (bffdca1) during the generation of this report, so develop (e3b8eb2) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@mhk197 mhk197 changed the title Add decimal byte-part splitting and assembly helpers Add decimal part splitting and assembly helpers Sep 8, 2026
@mhk197 mhk197 changed the title Add decimal part splitting and assembly helpers Add decimal byte-part splitting and assembly helpers Sep 8, 2026
@mhk197 mhk197 changed the title Add decimal byte-part splitting and assembly helpers Add decimal byte-part splitting and assembly logic Sep 9, 2026
@mhk197 mhk197 changed the title Add decimal byte-part splitting and assembly logic Add wide decimal byte-part splitting and assembly logic Sep 9, 2026
@mhk197
mhk197 marked this pull request as ready for review September 9, 2026 03:37
Comment on lines +137 to +138
let mut msp = BufferMut::<i64>::zeroed(values.len());
let mut lower = BufferMut::<u64>::zeroed(values.len());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of this please

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I can get rid of this case, but curious why? speeds up compression of arrays with significant # nulls (see benches)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

talked offline

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zeroed < using with_capacity

Comment on lines +124 to +149
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())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not use const generics here

@mhk197 mhk197 Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@mhk197
mhk197 removed this pull request from stack #9811 September 9, 2026 15:09
@mhk197
mhk197 changed the base branch from mk/compressor-serialized-ids to mk/dbp-v2-feature September 9, 2026 15:09
@mhk197
mhk197 added this pull request to stack #9813 September 9, 2026 15:09
@mhk197 mhk197 added the changelog/skip Do not list PR in the changelog label Sep 9, 2026
Comment on lines +128 to +131
for value in values.iter() {
msp.push((value >> LOWER_PART_BITS) as i64);
lower.push(*value as u64);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +186 to +196
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;
}
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks very slow with this iter

Comment on lines +200 to +214
/// 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,
]
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why cast?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +118 to +122
/// 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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move this to the lines it matches

Comment on lines +262 to +282
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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@mhk197
mhk197 requested a review from joseph-isaacs September 9, 2026 19:20
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>
@mhk197
mhk197 requested a review from robert3005 September 9, 2026 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/skip Do not list PR in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants