Repository navigation
Bound decimal storage by precision - #10305
joseph-isaacs wants to merge 10 commits into
Conversation
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
0fff52c to
ec2b1b5
Compare
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Merging this PR will improve performance by 25.57%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | mul_u64_nonnull_neon |
39.5 µs | 28.4 µs | +39.44% |
| ⚡ | WallTime | multiply_shapes_neon[(32768, PerRowPerRow)] |
38.5 µs | 32.3 µs | +19.28% |
| ⚡ | WallTime | mul_i64_nonnull_neon |
38.5 µs | 32.3 µs | +19.03% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ji/decimal-storage-contract (7b6da81) with develop (c901cf7)
Footnotes
-
367 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. ↩
-
1 benchmark was run, but is now archived. If it was deleted in another branch, consider rebasing to remove it from the report. Instead if it was added back, click here to restore it. ↩
…-contract Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
develop now initialises ArrayParts with their slots directly, so the precision-bound regression test passes the slots to the constructor instead of calling the removed with_slots helper. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF
Storage is now bounded by precision, so tests that stored values wider than the precision-derived width can no longer be constructed. Move the numeric overflow tests to inputs that overflow within bounded storage, expect i8 storage for a decimal(2, 0) byte-parts split, and drop the row key-width test whose narrowing path is unreachable from a valid array. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF
The checked constructors now fail when storage is wider than the type required by the precision. Importers that receive wider storage, the Arrow decimal conversions and legacy serialized arrays, narrow explicitly through DecimalData::narrow_to_precision before constructing the array. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF
Polar Signals Profiling ResultsLatest Run
Previous Runs (1)
Powered by Polar Signals Cloud |
Benchmarks: PolarSignals Profiling 📖Commits: PR datafusion / vortex-file-compressed / ns (1.003x ➖, 0↑ 0↓)
No file size changes detected. |
Benchmarks: String Encoding 📖Commits: PR vortex / vortex-file-compressed / ms (0.999x ➖, 0↑ 0↓)
vortex / vortex-file-compressed / % (1.000x ➖, 0↑ 0↓)
|
Benchmarks: FineWeb NVMe 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.026x ➖, 0↑ 1↓)
datafusion / vortex-compact / ns (1.006x ➖, 1↑ 1↓)
datafusion / parquet / ns (1.005x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.902x ➖, 3↑ 1↓)
duckdb / vortex-compact / ns (0.961x ➖, 1↑ 0↓)
duckdb / parquet / ns (0.997x ➖, 0↑ 0↓)
No file size changes detected. |
Benchmarks: TPC-H SF=1 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.997x ➖, 0↑ 0↓)
datafusion / vortex-compact / ns (1.003x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.000x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.004x ➖, 0↑ 1↓)
duckdb / vortex-compact / ns (0.993x ➖, 1↑ 1↓)
duckdb / parquet / ns (1.003x ➖, 0↑ 0↓)
No file size changes detected. |
Benchmarks: Clickbench Sorted on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.007x ➖, 0↑ 0↓)
datafusion / vortex-compact / ns (0.987x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.996x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.001x ➖, 2↑ 1↓)
duckdb / vortex-compact / ns (1.002x ➖, 1↑ 1↓)
duckdb / parquet / ns (1.031x ➖, 0↑ 2↓)
File Size Changes (200 files changed, -0.0% overall, 96↑ 104↓)
Totals:
|
Benchmarks: FineWeb S3 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.903x ➖, 2↑ 0↓)
datafusion / vortex-compact / ns (1.020x ➖, 0↑ 1↓)
datafusion / parquet / ns (1.003x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.981x ➖, 0↑ 0↓)
duckdb / vortex-compact / ns (1.094x ➖, 0↑ 1↓)
duckdb / parquet / ns (1.037x ➖, 0↑ 0↓)
|
Benchmarks: Appian on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-compact / ns (0.997x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.996x ➖, 0↑ 0↓)
duckdb / vortex-compact / ns (0.990x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.004x ➖, 0↑ 0↓)
No file size changes detected. |
Benchmarks: Clickbench on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.999x ➖, 0↑ 0↓)
datafusion / vortex-compact / ns (0.994x ➖, 1↑ 0↓)
datafusion / parquet / ns (0.999x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.013x ➖, 2↑ 4↓)
duckdb / vortex-compact / ns (1.012x ➖, 0↑ 2↓)
duckdb / parquet / ns (1.017x ➖, 1↑ 1↓)
No file size changes detected. |
Benchmarks: TPC-DS SF=1 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.001x ➖, 0↑ 1↓)
datafusion / vortex-compact / ns (1.001x ➖, 0↑ 1↓)
datafusion / parquet / ns (1.002x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.994x ➖, 4↑ 2↓)
duckdb / vortex-compact / ns (1.001x ➖, 1↑ 1↓)
duckdb / parquet / ns (0.998x ➖, 2↑ 3↓)
No file size changes detected. |
Benchmarks: TPC-H SF=10 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.002x ➖, 0↑ 0↓)
datafusion / vortex-compact / ns (0.992x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.996x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.993x ➖, 1↑ 0↓)
duckdb / vortex-compact / ns (0.995x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.996x ➖, 0↑ 0↓)
No file size changes detected. |
Benchmarks: Statistical and Population Genetics 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.993x ➖, 0↑ 0↓)
datafusion / vortex-compact / ns (0.998x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.994x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.002x ➖, 0↑ 0↓)
duckdb / vortex-compact / ns (1.002x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.006x ➖, 0↑ 0↓)
No file size changes detected. |
Benchmarks: Compression 📖Commits: PR vortex / vortex-file-compressed / ns (0.976x ➖, 0↑ 0↓)
vortex / vortex-file-compressed / bytes (1.000x ➖, 0↑ 0↓)
vortex / vortex-file-compressed / ratio (0.984x ➖, 0↑ 0↓)
vortex / parquet / ns (1.000x ➖, 0↑ 0↓)
vortex / parquet / bytes (1.000x ➖, 0↑ 0↓)
vortex / arrow-ipc / ns (0.987x ➖, 0↑ 0↓)
vortex / arrow-ipc / bytes (1.000x ➖, 0↑ 0↓)
|
Tests that built decimals with storage wider than their precision allows now use the bounded width, or a precision that admits the width they exercise. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF
Benchmarks: TPC-H SF=1 on S3 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.032x ➖, 0↑ 3↓)
datafusion / vortex-compact / ns (1.091x ➖, 1↑ 4↓)
datafusion / parquet / ns (0.973x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed / ns (1.058x ➖, 0↑ 1↓)
duckdb / vortex-compact / ns (1.069x ➖, 0↑ 1↓)
duckdb / parquet / ns (1.011x ➖, 0↑ 0↓)
|
Benchmarks: Random Access (S3) 📖Commits: PR How to read Verdict and Engines
random-access / vortex-file-compressed / ns (1.024x ➖, 0↑ 1↓)
random-access / parquet / ns (1.019x ➖, 0↑ 0↓)
random-access / lance / ns (1.215x ➖, 0↑ 4↓)
|
Benchmarks: TPC-H SF=10 on S3 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-compact / ns (1.075x ➖, 0↑ 2↓)
datafusion / parquet / ns (0.958x ➖, 1↑ 0↓)
duckdb / vortex-compact / ns (1.041x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.083x ➖, 0↑ 3↓)
|
Decimal byte parts written before storage was bounded by precision may be wider than it allows; host assembly narrows them explicitly while the GPU kernel, which cannot narrow device buffers, now validates instead of constructing unchecked. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF
Benchmarks: Random Access 📖Commits: PR How to read Verdict and Engines
vortex / arrow-ipc / ns (0.965x ➖, 2↑ 0↓)
random-access / vortex-file-compressed / ns (0.994x ➖, 0↑ 0↓)
random-access / parquet / ns (0.991x ➖, 0↑ 0↓)
random-access / lance / ns (0.983x ➖, 0↑ 0↓)
|
Byte-parts, sparse, row and fuzz tests that built decimals with storage wider than their precision allows now use the bounded width, or a precision that admits the width they exercise. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF
The GPU filter test stores its i256 case at a precision that admits i256, and the Arrow export test for narrowing i256 storage below its precision is removed since such storage can no longer be constructed. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zQPnFWRi7CWXHpgBE8CiF
Decimal arrays currently permit physical storage wider than the smallest type required by their precision. Establish
values_type <= DecimalType::smallest_decimal_value_type(decimal_dtype)as the storage contract so compute kernels can rely on this upper bound.try_new,try_new_handle) rather than narrowing it; storage within the bound is accepted without copying.DecimalData::narrow_to_precisionandDecimalArray::try_new_narrowedfor importers that receive wider storage than the precision requires. Arrow decimal imports, legacy serialized arrays, andDecimalBytePartsassembly narrow explicitly through them; device buffers cannot be narrowed and are rejected. The GPU byte-parts kernel validates its storage instead of constructing unchecked.Stack 1/2: based on
develop. The dispatch macros and kernel migrations are in #10314, based on this PR.Validation:
cargo clippy -p vortex-array -p vortex-arrow --all-targets --all-features -- -D warnings,cargo fmt --all --checkwith the pinned nightly, and the tests ofvortex-array,vortex-arrow,vortex-row,vortex-sparse,vortex-fuzzandvortex-decimal-byte-partspass locally with the CI profile. CI is green on the current head, including the CUDA, sanitizer and miri jobs.Compatibility: constructing a decimal array with storage wider than its precision requires is now an error (a panic from the infallible
newconstructors). Arrow imports of low-precisionDecimal128/Decimal256columns allocate to narrow their storage. Legacy files with oversized decimal storage still decode, with narrowing on the host; on the GPU they are rejected. As before, constructors require non-null values to fit their declared precision; this change does not add per-value precision validation.