Skip to content

perf(select): move owned row-address masks during union - #9884

Open
WanSuiMm wants to merge 1 commit into
lance-format:mainfrom
WanSuiMm:codex/move-owned-mask-unions
Open

WanSuiMm wants to merge 1 commit into
lance-format:mainfrom
WanSuiMm:codex/move-owned-mask-unions

Conversation

@WanSuiMm

Copy link
Copy Markdown

Owned RowAddrTreeMap |= rhs currently borrows rhs, cloning bitmap storage
when 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_union Criterion group with borrowed, consuming-extend
and 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 b3c861988 versus this patch:

Disjoint fixture Baseline Candidate Speedup
32 fragments/input, one pair 2909.934 us 5.978 us 486.7x
128 fragments/input, pair 1 12413.828 us 38.324 us 323.9x
128 fragments/input, pair 2 12090.619 us 35.628 us 339.4x
128 fragments/input, pair 3 10941.735 us 32.864 us 332.9x

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> improved
disjoint 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> is
unchanged in this patch and remains a control.

Validation on main b3c861988 plus this patch:

  • cargo test -p lance-select --lib: 116 passed, including union/property and
    existing 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: completed
    for 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-lto profile on an AMD Ryzen 7
8845H under x86_64 WSL2 Ubuntu 22.04. No end-to-end speed or allocation count
is claimed.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@lance-gatekeeper lance-gatekeeper Bot left a comment

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.

⚠️ Gate recommendation: approve with a non-blocking risk.

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.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Oct 11, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant