feat(vortex-onpair): update to the onpair 0.1 API - #8675
Merged
Conversation
gargiulofrancesco
added
the
action/benchmark-sql
github-actions
Bot
removed
the
action/benchmark-sql
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
Polar Signals Profiling ResultsLatest Run
Previous Runs (5)
Powered by Polar Signals Cloud |
Contributor
Benchmarks: PolarSignals ProfilingVortex (geomean): 1.019x ➖ How to read Verdict and Engines
datafusion / vortex-file-compressed (1.019x ➖, 1↑ 1↓)
File Size Changes (1 files changed, +0.0% overall, 1↑ 0↓)
Totals:
|
Contributor
Benchmarks: FineWeb NVMeVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.063x ➖, 0↑ 3↓)
datafusion / parquet (0.995x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.976x ➖, 3↑ 2↓)
duckdb / parquet (0.992x ➖, 0↑ 0↓)
File Size Changes (3 files changed, -46.2% overall, 1↑ 2↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=1 on NVMEVerdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.049x ➖, 0↑ 0↓)
datafusion / parquet (1.068x ➖, 0↑ 3↓)
datafusion / arrow (1.074x ➖, 0↑ 7↓)
duckdb / vortex-file-compressed (1.042x ➖, 0↑ 0↓)
duckdb / parquet (1.029x ➖, 1↑ 3↓)
File Size Changes (17 files changed, -43.3% overall, 5↑ 12↓)
Totals:
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.064x ➖, 1↑ 13↓)
datafusion / parquet (1.066x ➖, 1↑ 16↓)
duckdb / vortex-file-compressed (1.060x ➖, 0↑ 18↓)
duckdb / parquet (1.039x ➖, 0↑ 5↓)
File Size Changes (42 files changed, -43.5% overall, 15↑ 27↓)
Totals:
|
Contributor
Benchmarks: FineWeb S3Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.927x ➖, 0↑ 0↓)
datafusion / parquet (1.001x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed (1.007x ➖, 0↑ 1↓)
duckdb / parquet (1.009x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Clickbench Sorted on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.057x ➖, 0↑ 2↓)
datafusion / parquet (1.051x ➖, 0↑ 2↓)
duckdb / vortex-file-compressed (1.029x ➖, 2↑ 3↓)
duckdb / parquet (1.037x ➖, 0↑ 1↓)
File Size Changes (201 files changed, -42.2% overall, 91↑ 110↓)
Totals:
|
Contributor
Benchmarks: Statistical and Population GeneticsVerdict: No clear signal (medium confidence) How to read Verdict and Engines
duckdb / vortex-file-compressed (1.012x ➖, 0↑ 1↓)
duckdb / parquet (1.099x ➖, 0↑ 5↓)
File Size Changes (3 files changed, -32.3% overall, 1↑ 2↓)
Totals:
|
Contributor
Benchmarks: Clickbench on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.983x ➖, 3↑ 2↓)
datafusion / parquet (0.960x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed (0.931x ➖, 7↑ 0↓)
duckdb / parquet (0.996x ➖, 0↑ 0↓)
File Size Changes (201 files changed, -38.6% overall, 99↑ 102↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=10 on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.014x ➖, 1↑ 5↓)
datafusion / parquet (0.934x ➖, 4↑ 0↓)
datafusion / arrow (0.898x ✅, 14↑ 0↓)
duckdb / vortex-file-compressed (0.928x ➖, 4↑ 0↓)
duckdb / parquet (1.005x ➖, 0↑ 0↓)
File Size Changes (47 files changed, -43.4% overall, 20↑ 27↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=1 on S3Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.044x ➖, 0↑ 2↓)
datafusion / parquet (1.072x ➖, 3↑ 4↓)
duckdb / vortex-file-compressed (1.051x ➖, 0↑ 0↓)
duckdb / parquet (1.013x ➖, 0↑ 0↓)
|
Comment on lines
+98
to
+100
| // SAFETY: `out_bytes` has capacity for `total_size + DECODE_PADDING`, and | ||
| // `total_size` is the decoded length from Vortex's uncompressed_lengths | ||
| // child. |
Contributor
There was a problem hiding this comment.
can we trust uncompressed_lengths?
Contributor
Author
There was a problem hiding this comment.
Swapped the decode to the safe try_decode_into + a length cross-check, so uncompressed_lengths is no longer trusted for sizing
gargiulofrancesco
force-pushed
the
fg/onpair-integration
branch
3 times, most recently
from
July 16, 2026 13:55
37bd58d to
d2e55d0
Compare
gargiulofrancesco
changed the title
Contributor
Benchmarks: Vortex queries 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.026x ➖, 0↑ 0↓)
datafusion / parquet (0.952x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.952x ➖, 0↑ 0↓)
duckdb / parquet (0.997x ➖, 0↑ 0↓)
No file size changes detected. |
Port the OnPair encoding to the 0.1.0 public API, including raw column parts, validated compact dictionaries, and MaxDictBits configuration. Pin the latest upstream validation fix, add compressed-domain equality pushdown, and strengthen file-borne dictionary and code validation. Signed-off-by: Francesco Gargiulo <gargiulo.fr@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Francesco Gargiulo <gargiulo.fr@gmail.com>
OnPair's own decoder already bounds-checks every code in-loop before copying (a near-free, predicted-never-taken branch); vortex-onpair's validate_codes was a second full pass over the codes buffer ahead of decode, doubling memory traffic on a hot path for no additional soundness (out-of-range codes still surface, as a panic instead of a VortexResult error, matching the behavior prior to the onpair 0.1.0 port). Signed-off-by: Francesco Gargiulo <gargiulo.fr@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The codes slot narrows to U8 only when dict_size is exactly 256, which is also the minimum size for a valid (complete) OnPair dictionary. The prior "at most 256 tokens" phrasing implied a range below 256 that can never occur. Signed-off-by: Francesco Gargiulo <francesco@spiraldb.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bits() returns ceil(log2(dict_size)), the minimum code width to address the dictionary, not the physical per-code storage width. The codes child is stored as a U8/U16 primitive array and may be re-encoded by the cascading compressor, so "Bits per token code" was misleading. Signed-off-by: Francesco Gargiulo <francesco@spiraldb.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous comments described try_decode_into's safe, buffer-derived bound and fast-path internals, which are implementation-specific and would not hold if the decode path switches to unsafe decode_into. Keep only the durable point: pad the output for the decoder's per-token over-copy and verify the exact size afterward. Signed-off-by: Francesco Gargiulo <francesco@spiraldb.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Compression accepts u64 byte offsets (large-binary inputs), but the codes_offsets child was always narrowed to u32, capping a chunk at 2^32 tokens and failing compression above it. Pick the narrowest of u32/u64 that holds the largest per-row code boundary instead, so codes_offsets scales with the u64 byte-offset capacity. The cascading compressor still narrows the common u32 case down to u16/u8, and the width round-trips via the existing codes_offsets_ptype metadata, so the serialized format is unchanged. Widen CodesWindow to Buffer<u64> to match; the u64->usize conversions are checked (cast_possible_truncation is denied) but fold away on 64-bit. The u64 branch cannot be reached with realistic test data (>4 GiB chunk), so cover it two ways: a unit test drives the width selection via a threshold parameter, and a read-path test hand-widens a small array's codes_offsets child and asserts canonical decode and the compressed-domain equality compare (CodesWindow) behave identically to the u32 width. Signed-off-by: Francesco Gargiulo <francesco@spiraldb.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The onpair dep was a bare git rev with no `version`, which fails the publish dry-run CI job (Cargo requires a version to package/publish). Add `version = "0.1.0"` alongside the pin; Cargo only checks a version is present, so local builds still resolve the pinned commit. Matches the tpchgen pattern below. Signed-off-by: Francesco Gargiulo <francesco@spiraldb.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Francesco Gargiulo <gargiulo.fr@gmail.com>
gargiulofrancesco
force-pushed
the
fg/onpair-integration
branch
from
July 16, 2026 14:25
d2e55d0 to
752e76f
Compare
| once_cell = "1.21" | ||
| oneshot = { version = "0.2.0", features = ["async"] } | ||
| onpair = { version = "0.0.4" } | ||
| onpair = { version = "0.1.0", git = "https://github.com/spiraldb/onpair.git", rev = "28df115c799674c663adedc8f81620ac82c7a890" } |
Comment on lines
264
to
269
| let dict_size = match u32::try_from(dict_offsets.len().saturating_sub(1)) { | ||
| Ok(dict_size) => dict_size, | ||
| Err(_) => vortex_panic!("OnPair dict_size exceeds u32"), | ||
| }; | ||
| let data = OnPairData::new(dict_bytes, dict_size, len); | ||
| let slots = OnPairSlots { |
Contributor
There was a problem hiding this comment.
you don't need to store the size its in dict_bytes
| .checked_sub(1) | ||
| .ok_or_else(|| vortex_err!(InvalidArgument: "OnPair dict_offsets must not be empty"))?; | ||
| u32::try_from(dict_size) | ||
| .map_err(|_| vortex_err!(InvalidArgument: "OnPair dictionary size exceeds u32")) |
Contributor
There was a problem hiding this comment.
code inline on 264-268 also
| let mut out_bytes = ByteBufferMut::with_capacity(total_size + onpair::DECODE_PADDING); | ||
| let written = onpair::try_decode_into(codes.as_slice(), dict, out_bytes.spare_capacity_mut()) | ||
| .map_err(|_| { | ||
| vortex_err!("OnPair codes decode to more bytes than uncompressed_lengths records") |
Contributor
There was a problem hiding this comment.
this should be a panic I think
Comment on lines
+66
to
+69
| offsets.resize(len + 1, O::from_usize(0)); | ||
| for _ in 0..len { | ||
| uncompressed_lengths.push(0); | ||
| } |
Comment on lines
-45
to
-56
| if mask.all_false() { | ||
| return OnPair::try_new( | ||
| array.dtype().clone(), | ||
| BufferHandle::new_host(ByteBuffer::empty()), | ||
| ConstantArray::new(0, len).into_array(), | ||
| ConstantArray::new(0u16, len).into_array(), | ||
| ConstantArray::new(0u32, len + 1).into_array(), | ||
| ConstantArray::new(0i32, len).into_array(), | ||
| Validity::AllInvalid, | ||
| 9, | ||
| ); | ||
| } |
| /// single chunk carries more than `u32_max` tokens, matching the `u64` byte | ||
| /// offsets accepted at compression. `u32_max` is a parameter so tests can drive | ||
| /// the `u64` branch without a multi-GiB array. | ||
| fn codes_offsets_array<O: Offset>(row_offsets: &[O], u32_max: usize) -> ArrayRef { |
… all-null Drop derived state and redundant metadata from the OnPair representation: - Remove `dict_size`/`bits()` from `OnPairData`; the dictionary size is recoverable from `dict_offsets.len() - 1`, so storing it (and validating it against the child) was redundant. - Rename the `total_tokens` metadata field to `codes_len`, which is what it actually records: the length of the `codes` slot child (a sliced array may retain codes outside its visible row range). - Have `onpair_compress` return `ArrayRef` and emit a `ConstantArray` of nulls for all-null input rather than a degenerate empty-code-stream OnPair array. Callers (btrblocks, benches, tests) downcast to `OnPair` as needed. - Drop the generic `Offset` parameter from compression; flatten to `u64` byte offsets unconditionally and size the flat buffer from the actual view lengths. `codes_offsets` width selection keeps the u32/u64 adaptivity. Decode paths (`canonical`, `ops`) no longer over-allocate `DECODE_PADDING` and `vortex_panic!` on a decoded-length mismatch instead of returning an error: the length is an internal invariant, not attacker-controlled input, so a mismatch is a bug rather than a recoverable condition. Signed-off-by: Francesco Gargiulo <francesco@spiraldb.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nary once Widen the dict_offsets child to u32 and validate the compact dictionary lazily, memoizing both in an Arc<OnceLock> on OnPairData so the cost is paid at most once per dictionary rather than per operation. Thread OnPairData through new_unchecked so slice/filter/cast share the cell, seed it from the trainer's conformant offsets at compress time, and drop the cell on buffer replacement. Add a test covering lazy rejection of a corrupt dictionary on first use, including on a derived slice. Signed-off-by: Francesco Gargiulo <francesco@spiraldb.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
joseph-isaacs
marked this pull request as ready for review
July 17, 2026 15:37
joseph-isaacs
approved these changes
Jul 17, 2026
joseph-isaacs
left a comment
joseph-isaacs
left a comment
Contributor
There was a problem hiding this comment.
Let get this merged
The onpair dependency was a git source pinned to commit 28df115, which goes against the workspace guideline of depending on published crates. onpair 0.1.1 is now on crates.io and its v0.1.1 tag is the exact commit that was pinned, so this is behavior-preserving: the resolved package is byte-identical. Drop the git source and version placeholder in favor of a plain registry dependency. Verified: cargo build -p vortex-onpair and all 36 vortex-onpair tests pass against the crates.io 0.1.1 package. Signed-off-by: Francesco Gargiulo <francesco@spiraldb.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
I, Francesco Gargiulo <gargiulo.fr@gmail.com>, hereby add my Signed-off-by to this commit: ab329f7 I, Francesco Gargiulo <gargiulo.fr@gmail.com>, hereby add my Signed-off-by to this commit: f838da3 I, Francesco Gargiulo <gargiulo.fr@gmail.com>, hereby add my Signed-off-by to this commit: 3bb9dc7 I, Francesco Gargiulo <gargiulo.fr@gmail.com>, hereby add my Signed-off-by to this commit: e5705e8 I, Francesco Gargiulo <gargiulo.fr@gmail.com>, hereby add my Signed-off-by to this commit: d43343e I, Francesco Gargiulo <gargiulo.fr@gmail.com>, hereby add my Signed-off-by to this commit: fc335b4 I, Francesco Gargiulo <gargiulo.fr@gmail.com>, hereby add my Signed-off-by to this commit: b099b5b I, Francesco Gargiulo <gargiulo.fr@gmail.com>, hereby add my Signed-off-by to this commit: a0a4ee3 Signed-off-by: Francesco Gargiulo <gargiulo.fr@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Updates the experimental
vortex-onpairencoding to the public API introduced inonpair0.1.u32/u64code offsets.u16token address space.u64code offsets.Scope
This PR intentionally does not include the new
LIKE,list_contains, ortakepushdowns. Those will be submitted in a follow-up PR.A separate follow-up will enable OnPair on the default read path without enabling it in the default compressor.
Dependency note
The dependency is temporarily pinned to an upstream OnPair commit containing the dictionary-validation fix. Before merging, that fix will be published in a new crates.io release and the Git dependency will be replaced with the released version.
Testing
cargo +nightly fmt --all -- --checkcargo test -p vortex-onpaircargo test -p vortex-btrblocks --features unstable_encodingscargo clippy -p vortex-onpair -p vortex-btrblocks --all-targets --all-features -- -D warnings