Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
Moving absent-fragment selections addresses the avoidable copying in #9578. Retaining the borrowed Roaring merge for occupied fragments keeps this optimization narrow.
The remaining risk is overlap performance: the reported original 28.1% control slowdown remains unexplained, and the filtered reruns do not establish a general no-regression result. The supported benefit is scoped to disjoint CPU unions; representative overlap-heavy and end-to-end workloads remain unverified.
This branch has not been deployed
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
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.
Owned
RowAddrTreeMap |= rhscurrently borrowsrhs, cloning bitmap storagewhen a fragment is absent from the accumulator. Consume the RHS map and move
those selections into place, while retaining the existing borrowed Roaring
merge for occupied partial fragments. Expand the existing union regression
test and add an
owned_unionCriterion group with borrowed, consuming-extendand nullable AllowList controls.
Addresses the owned-union portion of #9578. The other optimization items remain
outside this PR; dependencies, public APIs and serialization are unchanged.
The benchmark merges four maps (first accumulator plus three RHS maps), with
32/128 fragments per map and about 16/64 MiB total bitmap payload. Every
fragment selects alternating offsets from a 2^20-offset domain. Setup clones
and final output destruction are excluded. The overlap control has identical
fragment IDs and identical bitmap contents; the AllowList consumer has
empty NULL sets. These are CPU operation microbenchmarks, not query timings.
Criterion mean times for the owned API, on main
b3c861988versus this patch:The baseline owned API was 559.8x/315.2x slower than consuming
Extend<Self>on the two disjoint sizes, reproducing the avoidable-copying cost.
Large identical owned controls changed by -14.1%, +1.7% and +6.7%.
The initial small identical owned pair changed from 610.238 to 781.751 us
(+28.1%), so the predeclared strict screen (no observed control pair over
10% regression) failed. Unchanged borrowed/Extend controls also shifted
substantially; this does not identify the anomaly's cause.
A separate diagnosis reused the same binaries without rebuilding and ran
only the three small identical cases in fresh BC/CB/BC pairs. Owned time
changed by -3.4%, -0.3% and +2.9%, giving 0/3 pairs over 10%. That filtered
sequence changes preceding benchmark history; it does not explain the original
outlier or replace the failed strict screen. Positive performance claims here
are limited to the disjoint microbenchmark, with no general no-regression claim.
Retaining the borrowed bitmap merge on occupied fragments is intentional.
An initial local candidate that delegated directly to
Extend<Self>improveddisjoint unions but showed about 9% and 13% slowdowns in two large identical
controls. Roaring's owned union compares cardinalities and consumes containers;
its borrowed path avoids those length comparisons. The dense bitmap kernel is
shared, so the measurements do not isolate a single cause.
Extend<Self>isunchanged in this patch and remains a control.
Validation on main
b3c861988plus this patch:cargo test -p lance-select --lib: 116 passed, including union/property andexisting NULL algebra tests.
cargo fmt --all: passed.cargo clippy --all --tests --benches -- -D warnings: passed.cargo bench -p lance-select --bench row_addr_mask --profile release-no-lto -- owned_union --sample-size 20 --warm-up-time 1 --measurement-time 2: completedfor both revisions, with two additional large-fixture pairs after compilation
finished. Fresh benchmark processes are the replication unit; Criterion's
within-process confidence intervals do not measure cross-run uncertainty.
The measurements use upstream-pinned Rust 1.98.1, Criterion 0.8.2,
Roaring 0.11.4, and the repository
release-no-ltoprofile on an AMD Ryzen 78845H under x86_64 WSL2 Ubuntu 22.04. No end-to-end speed or allocation count
is claimed.