feat: resolve data overlay files on the take (and scan) read path#7536
Conversation
Read impact of overlay filesWhat does resolving overlays on read cost, vs. a dataset with none? Measured on a
The routing itself (deciding base-vs-overlay per cell) is negligible — under 2 ms |
Read impact of overlay files on a wide value columnFollow-up to the Wide column — 3072-d embedding, 100K rows (~1.2 GiB base)
For reference, the narrow Takeaways for a wide column:
Contiguous overlays are effectively free on a wide scan. 0 / 4 / 16 contiguous layers all land at ~590–620 ms, no measurable per-layer cost — the bitmap-major contiguous path from this PR doing exactly what it's for: against a base read that large, the contiguous merge is lost in the noise. (Contrast Scattered (stride) coverage adds a bounded ~45% (~270 ms) that — unlike Cold-cache, every wide scan reads the full ~1.2 GiB column regardless of layer count (16 layers at 1% coverage add <5% IO); a wide Net: for wide vector columns the overlay read tax is smaller in relative terms than the Benchmark added in #7544 ( |
e2f98f5 to
efab63a
Compare
3ba338b to
a7d4508
Compare
efab63a to
6a39624
Compare
4487c79 to
a7cfcf0
Compare
a7cfcf0 to
72cc6d2
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesOverlay-aware fragment reads now plan coverage from metadata, lazily load required overlay values, merge them into projected batches, and apply this flow to Overlay read support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FragmentReader
participant plan_overlays
participant resolve_overlays
participant merge_overlay_batch
participant RowIdAndDeletes
FragmentReader->>plan_overlays: plan overlay coverage from fragment metadata
FragmentReader->>resolve_overlays: resolve requested fragment offsets
resolve_overlays-->>merge_overlay_batch: loaded overlay atom values
FragmentReader->>merge_overlay_batch: provide base batch
merge_overlay_batch-->>FragmentReader: merged batch
FragmentReader->>RowIdAndDeletes: apply row IDs and deletion filtering
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
Clean design. I think the I/O could probably be optimized better in the future (I dropped a comment about the holes) but we can cross that bridge when we come to it.
| /// An overlay contributes to a projected field when any id in its `data_file.fields` | ||
| /// belongs to that top-level field's subtree. Overlays are written against the field | ||
| /// ids the file actually stores, which for a struct or list column is its *leaf* | ||
| /// child ids (the V2_1 structural encoding does not record the parent id), so a leaf | ||
| /// is mapped back to its top-level projected ancestor — the whole column is then | ||
| /// fetched and replaced as a unit. Each contributing overlay *file* appears once in | ||
| /// `files`, shared by every top-level field it covers. |
There was a problem hiding this comment.
The wording here is confusing.
An overlay contributes to a projected field when any id in its
data_file.fieldsbelongs to that top-level field's subtree
What "top-level field" is the emphasized 'that' referring to? I think it is referring to the projected field but projected fields are not necessarily top-level fields.
| for (field_pos, &field_id) in overlay.data_file.fields.iter().enumerate() { | ||
| if let Some(&top) = top_level_of.get(&field_id) | ||
| && seen.insert(top.id) | ||
| { | ||
| contributions.push((top, field_pos)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Let say I have a struct as so:
my_struct(field_id=7)
my_struct.foo(field_id=8)
my_struct.bar(field_id=9)
If my projection targets field 8 and the overlay targets field 9 then I will not find it here and the overlay will not be applied. I think this is correct but all this talk of top-level fields made it seem like an overlay should be applied whenever there is a common top-level field (which there is in this case).
There was a problem hiding this comment.
Actually, here's a more concerning case. Let's say I have:
outer_struct(field_id=11)
outer_struct.middle_struct(field_id=12)
outer_struct.middle_struct.inner_struct(field_id=13)
Let's say my projection asks for field_id=12 (middle_struct). Then top_level_of will be 12->11. Then my overlay will have field_id=13 which is not present in top_level_of and so I will not apply that overlay but I should!
| Ok(result.project_by_schema(&output_schema)?) | ||
| } | ||
|
|
||
| /// Merge data overlay values into a stream of base batches. |
There was a problem hiding this comment.
| /// Merge data overlay values into a stream of base batches. | |
| /// Merge data overlay values onto a stream of base batches. |
Super trivial and possibly incorrect nit.
| // The offset_in_frag of every row this read will return, materialized once. | ||
| // Cost is one u32 per output row (a whole-fragment scan is 4 bytes/row), and | ||
| // it lets us both prune overlays to the read and slice each batch's offsets | ||
| // below without reading any data. Only paid when the fragment has overlays. | ||
| let offsets_in_frag: Arc<Vec<u32>> = | ||
| Arc::new(params.to_offsets_total(total_num_rows).values().to_vec()); |
There was a problem hiding this comment.
I wonder if we can avoid this materialization? In resolve_overlays it is immediately converted into a bitmap. In the map function it is sliced. These seem like things we could do without materializing. I think slicing support already exists for ReadBatchParams and we could add conversion-to-bitmap support (or just convert to bitmap immediately and then slice the bitmap)
Alternatively, maybe ReadBatchParams should change to a roaring bitmap/treemap in the first place 🤔
But I guess that would be a larger refactor 😛
| // TODO(overlay perf): these reads use the default reader priority. Once we | ||
| // benchmark take/scan over overlays, decide whether overlay value reads should | ||
| // inherit `read_config.reader_priority` (or get a dedicated priority) so they | ||
| // schedule alongside the base reads. |
There was a problem hiding this comment.
It's more than just a performance concern. An invalid priority can lead to deadlock. For example, the default prioirty is 0 (highest priority), if we issue a bunch of overlay reads for a batch that we're not ready to read at all then they can clog up the backpressure queue.
However, I think we are ok. I don't think we initiate the fetch until the task is polled for the first time. That won't happen unless we have a dedicated consumer for the data. A priority of 0 is probably the correct answer in this case (we are initiating the reads quite late in the process)
The potential performance optimization would be to start fetching the overlay data earlier. For example, imagine we are in a compute constrained workload. The current approach probably gives us bubbles:
So, in the end, I guess it is a perf question. Still, let's update the comment to say we are using priority 0 because we've already indicated we are ready to consume this batch and so it is safe to consider it highest priority.
| /// `offset_in_overlay = <cells this coverage has before the batch>` (a single | ||
| /// `rank` lookup), and each following cell is one more. Coverages are applied | ||
| /// newest-first, and the first overlay to claim a row wins. | ||
| fn route_contiguous( |
There was a problem hiding this comment.
Actually, sorry, switching back to request changes until the projection mapping comment (#7536 (comment)) is resolved.
dec272a to
b82ac8d
Compare
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
github-actions
Bot
added
the
A-format
Wire overlay resolution into FragmentReader so `take` and scan reads merge data overlay cells over the base data, removing the temporary guard that refused reads of fragments carrying overlays. Reads are lazy and rank-pushed: each overlay file is opened once (no value bytes read up front), requested offsets are routed to the newest covering overlay at their coverage rank, and only the touched ranks are fetched via the existing `take` primitive. Base and overlay IO run concurrently, then the column is assembled with `interleave`. This makes overlay reads O(requested rows) rather than O(coverage). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Adds tests for combinations the initial read-path suite missed: row-id pass-through alongside an overlay, an older overlay routed zero ranks (empty-ranks fetch branch), a newest NULL value shadowing an older non-null, per-field newest-wins across two sparse overlays, a plan present but all requested offsets uncovered (all-fall-through), and a dataset-level take spanning multiple overlaid fragments. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`route_overlays` was offset-major: for every physical offset it probed
each coverage bitmap newest-first. On a full scan that is
`O(N_rows * K_overlays)` roaring `contains` probes — ~16M for a 1M-row,
16-overlay scan — of which 99% only confirm a fall-through to base. It
dominated scan CPU (~87% of the scan thread) and made scan cost grow
linearly with overlay count, badly so for scattered (stride) coverage.
A scan reads a contiguous physical range, so offset `o` is output row
`o - base`. The fast path now intersects each coverage with the batch's
offset range (a container-level op that drops a non-overlapping batch in
`O(containers)`) and routes only the in-range bits directly to rows.
Those bits carry consecutive coverage ranks starting at the count of bits
below `base`, so ranks need a single `rank` lookup rather than a probe per
cell. `take` still supplies arbitrary offsets and keeps the general path.
Microbenchmark of `route_overlays` over a 1M-row scan (ms/scan):
overlays stride old -> new contiguous old -> new
1 13.3 -> 0.67 2.8 -> 0.55
4 46.3 -> 0.86 3.5 -> 0.66
16 178.5 -> 1.48 9.4 -> 0.96
64 829.0 -> 4.07 33.3 -> 2.09
A fuzz test asserts the fast path produces byte-identical routing to the
general path across random bases, lengths, overlay counts, and densities.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Review feedback: the "rank" language in the overlay read path is hard to follow. Name the three coordinate spaces explicitly and use them everywhere: - offset_in_frag: a row's physical position in the fragment - offset_in_batch: a row's position within the batch being assembled - offset_in_overlay: a value's position in an overlay's dense value column (what a roaring bitmap calls "rank") Renames `OverlayRouting::needed_ranks` -> `offsets_in_overlay`, `fetch_overlay_ranks` -> `fetch_overlay_values`, and the contiguous-path `base` -> `frag_start` (it was colliding with "base column"). Rewrites the module and function docs in plainer language. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Review feedback: prune overlay readers by row selection, not just projection. Overlay readers were opened eagerly in `FileFragment::open`, before the read's rows were known, so a `take` that misses an overlay's cells still paid to open its file. Split the work: - `plan_overlays` (at open): build the coverage plan from metadata only — no files opened, no IO. - `resolve_overlays` (at read, in `merge_overlays`): the read's offsets are known here, so open only the overlay files whose coverage intersects the requested rows; overlays disjoint from the read are dropped and never opened. Projection pruning (skip overlays covering no projected field) is unchanged. Trade-off: a reused `FragmentReader` reopens overlay readers per read rather than once at open. Acceptable — overlays are resolved per read operation, and the plan/state is shared via `Arc` so cloning a reader stays cheap. Test: `test_take_prunes_overlays_outside_row_selection` deletes an overlay's data file and shows a take missing its coverage still succeeds (never opened), while a take hitting it fails. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`plan_overlays` walked every projected field against every overlay, which is `O(num_fields x num_overlays)` — costly when a fragment has thousands of fields. Rebuild it as a single newest-first pass over the overlays that indexes each covered field into a `field_id -> overlays` map, making planning `O(total overlay fields + projected fields)`. Overlays are already sorted newest-last on load (`sort_overlays_newest_last`), so the pass just iterates the slice in reverse for newest-first precedence — dropping the redundant `overlay_indices_newest_first` sort helper (its ordering rule stays covered by `test_overlays_sorted_newest_last_on_load`). A `debug_assert` documents the load-time invariant the reverse walk relies on. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`OverlayRouting` (with its methods), `route_overlays`, and `assemble_overlay_column` were `pub` but are only used within the `overlay` module — its sibling helpers (`route_contiguous`, `route_arbitrary`, `fetch_overlay_values`, ...) are already private. Narrow them to match, so the module's real entry points (`plan_overlays`, `resolve_overlays`, `merge_overlay_batch`, `OverlayReadPlanner`, `FieldOverlayPlan`) are clear at a glance. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
`merge_overlays` slices each batch out of the read's `offsets_in_frag` with a running `rows_seen` accumulator, but every existing scan test used single-batch 6-row fragments, so the cross-batch (`start > 0`) path was never exercised. Add a scan over a 10-row fragment at batch_size 4 with overlays in the 1st, 2nd, and 3rd batches; the test asserts the read actually spans multiple batches before checking alignment. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Add three read-path cases that were untested: an empty `take` (plan present, no offsets to route), an end-to-end overlay on a variable-width `Utf8` column (exercises the string value-pushdown path through the real reader, plus a NULL override), and projection pruning doing zero IO — proven by deleting the overlay's data file and confirming a read that projects only the unrelated column still succeeds while projecting the overlaid column fails. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
An overlay is written against the field ids the file stores, which for a struct or list column under the V2_1 structural encoding are its *leaf* child ids — the parent's top-level id is never recorded. `plan_overlays` matched only top-level ids, so a struct/list overlay was silently skipped on read (V2_1) and returned stale base values, while V2_0 — which does record the parent id — resolved it. The mismatch was untested. Map every id in the projection subtree (top-level fields and their nested descendants) to its top-level projected field, so a leaf id resolves to the column it belongs to; the whole column is then fetched and replaced as a unit. Add end-to-end tests for struct and list overlays on both formats. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The struct/list fix mapped an overlay's stored fields to their top-level
projected column and replaced the whole column, which panicked whenever an
overlay's fields did not equal that column's full leaf set — e.g. an
overlay on sub-field `s.a` while the read projects the whole struct `s`
(it fetched `s` from a file holding only `a`). Reported by review.
Resolve per *atom* instead: a per-row field an overlay replaces as a unit
(a primitive leaf, or a whole list/map; structs are recursed through). An
overlay contributes to an atom when its stored leaf ids fall in the atom's
leaf set; the atom's value is fetched (projected to exactly the atom) and
spliced back into its output column, rebuilding the struct spine along the
path. Overlays on different sub-fields of one struct resolve
independently, newest-wins is per atom, and an overlay on a non-projected
field is skipped (its file never opened). Adds `enumerate_atoms`,
`collect_leaf_ids`, `descend_by_ids`, `splice_by_ids`.
Also per review: clarify the reader-priority comment (priority 0 is
correct because overlay reads are issued late, only when a ready consumer
polls the batch), note the offsets-materialization follow-up, and fix a
doc typo ("into" -> "onto").
Tests: sub-field overlay under a parent-struct projection (the panic),
non-projected sibling skipped, three-level nesting, multiple sub-field
overlays with per-atom newest-wins, and descend/splice unit tests
(including struct-null preservation).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…ng accessors Self-review cleanups. `AtomOverlayPlan` is the opened counterpart of `PlannedAtom`, so name it `LoadedAtom` to match the `PlannedAtomOverlay` -> `LoadedAtomOverlay` pairing and stop it reading like a planning-phase type. Drop the `OverlayRouting::offsets_in_overlay`/`all_fall_through` getters: every caller is in-module and some already read the fields directly, so the thin forwarders only created a mixed convention. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Add two end-to-end cases surfaced by self-review: - A NULL *base* cell (distinct from a NULL overlay value): a covered NULL base is overridden to the overlay's value, an uncovered NULL base falls through and stays NULL. - A top-level Map column, exercising a multi-leaf atom (key + value leaf ids both mapping back to the one Map atom). Maps require the 2.2+ file format, so this case runs at V2_2 rather than the V2_0/V2_1 parametrization. Also hoist the test module's StructArray/Fields imports to the top of `overlay_read` and update a stale reference to the removed `all_fall_through`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
4bb5e39 to
5ff87ce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance/src/dataset/fragment.rs`:
- Around line 3749-3752: The pruning tests at rust/lance/src/dataset/fragment.rs
lines 3749-3752 and 4189-4192 must validate the specific missing-file errors
instead of only calling is_err(): match each result as the Error variant and
assert its message references miss.lance at the first site and valov.lance at
the second.
In `@rust/lance/src/dataset/overlay.rs`:
- Around line 796-854: Consolidate the six input-only tests around resolve into
one parametrized #[rstest], with a named #[case::{name}(...)] for each existing
scenario. Pass each case’s base array, offsets, overlays, and expected values as
parameters, then invoke resolve and assert_i32_eq once in the shared test body;
preserve all current cases and expectations.
- Around line 297-308: Restrict the visibility of the crate-internal overlay API
by changing LoadedAtom, OverlayReadPlanner, is_empty, plan_overlays,
resolve_overlays, and merge_overlay_batch from pub to pub(crate). Leave public
re-exports unchanged so the intended external API remains available only through
those re-exports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d182d7ad-f36f-4f35-a04e-3925b0e94945
📒 Files selected for processing (3)
rust/lance/src/dataset.rsrust/lance/src/dataset/fragment.rsrust/lance/src/dataset/overlay.rs
| pub struct LoadedAtom { | ||
| /// The top-level output column the atom lives in (its name locates the batch | ||
| /// column; its field tree drives the descend/splice into that column). | ||
| top_field: Arc<Field>, | ||
| /// Child field ids from `top_field` down to the atom (empty when the atom *is* | ||
| /// the top-level column). Drives descending to, and splicing back, the atom. | ||
| ancestor_ids: Vec<i32>, | ||
| /// Projection of exactly the atom (its ancestor path pruned to the atom subtree), | ||
| /// used to fetch the atom's values from the overlay file. | ||
| fetch_projection: Arc<Schema>, | ||
| overlays_newest_first: Vec<LoadedAtomOverlay>, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer pub(crate) over pub for these crate-internal items.
overlay is declared pub(crate) mod overlay;, so LoadedAtom, OverlayReadPlanner (Line 343), is_empty (Line 350), plan_overlays (Line 372), resolve_overlays (Line 570), and merge_overlay_batch (Line 656) are only reachable within the crate regardless. Marking them pub overstates the intended surface; use pub(crate).
As per coding guidelines: "Prefer pub(crate) over pub for crate-internal items, and use pub use re-exports for the actual public API surface."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance/src/dataset/overlay.rs` around lines 297 - 308, Restrict the
visibility of the crate-internal overlay API by changing LoadedAtom,
OverlayReadPlanner, is_empty, plan_overlays, resolve_overlays, and
merge_overlay_batch from pub to pub(crate). Leave public re-exports unchanged so
the intended external API remains available only through those re-exports.
Source: Coding guidelines
| #[test] | ||
| fn test_no_overlays_returns_base() { | ||
| let base = i32_array([Some(1), Some(2), Some(3)]); | ||
| let resolved = resolve(&base, &offsets(0, 3), &[]); | ||
| assert_i32_eq(&resolved, [Some(1), Some(2), Some(3)]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_single_overlay_value_offset() { | ||
| // Base ages [30, 25, 40, 22]; overlay sets offset_in_frag 1 -> 26, whose | ||
| // value sits at offset_in_overlay 0. | ||
| let base = i32_array([Some(30), Some(25), Some(40), Some(22)]); | ||
| let overlay = (bitmap([1]), i32_array([Some(26)])); | ||
| let resolved = resolve(&base, &offsets(0, 4), &[overlay]); | ||
| assert_i32_eq(&resolved, [Some(30), Some(26), Some(40), Some(22)]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_value_offsets_multiple_cells() { | ||
| // Coverage {0, 2, 3} -> values at offset_in_overlay 0, 1, 2. | ||
| let base = i32_array([Some(10), Some(11), Some(12), Some(13)]); | ||
| let overlay = ( | ||
| bitmap([0, 2, 3]), | ||
| i32_array([Some(100), Some(120), Some(130)]), | ||
| ); | ||
| let resolved = resolve(&base, &offsets(0, 4), &[overlay]); | ||
| assert_i32_eq(&resolved, [Some(100), Some(11), Some(120), Some(130)]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_newest_overlay_wins() { | ||
| // Two overlays both cover offset_in_frag 1; the newest (first in the slice) | ||
| // wins. | ||
| let base = i32_array([Some(0), Some(1), Some(2)]); | ||
| let newest = (bitmap([1]), i32_array([Some(999)])); | ||
| let older = (bitmap([1, 2]), i32_array([Some(111), Some(222)])); | ||
| let resolved = resolve(&base, &offsets(0, 3), &[newest, older]); | ||
| // offset 1 -> newest (999); offset 2 -> only older covers it (222). | ||
| assert_i32_eq(&resolved, [Some(0), Some(999), Some(222)]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_null_override_vs_fall_through() { | ||
| // A covered offset with a NULL value overrides the cell to NULL; an | ||
| // absent offset falls through to the base. | ||
| let base = i32_array([Some(1), Some(2), Some(3)]); | ||
| let overlay = (bitmap([0]), i32_array([None])); | ||
| let resolved = resolve(&base, &offsets(0, 3), &[overlay]); | ||
| assert_i32_eq(&resolved, [None, Some(2), Some(3)]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_physical_start_offset() { | ||
| // The batch covers physical rows [10, 13); the overlay covers offset 11. | ||
| let base = i32_array([Some(0), Some(0), Some(0)]); | ||
| let overlay = (bitmap([11]), i32_array([Some(7)])); | ||
| let resolved = resolve(&base, &offsets(10, 3), &[overlay]); | ||
| assert_i32_eq(&resolved, [Some(0), Some(7), Some(0)]); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consolidate these input-only variants into a single parametrized rstest.
test_no_overlays_returns_base, test_single_overlay_value_offset, test_value_offsets_multiple_cells, test_newest_overlay_wins, test_null_override_vs_fall_through, and test_physical_start_offset differ only by (base, offsets, overlays, expected) and all funnel through resolve + assert_i32_eq. A single #[rstest] with #[case::{name}(...)] per scenario keeps them readable and removes the boilerplate.
As per coding guidelines: "Use rstest for Rust tests ... when cases differ only by inputs; use #[case::{name}(...)] for readable Rust case names."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance/src/dataset/overlay.rs` around lines 796 - 854, Consolidate the
six input-only tests around resolve into one parametrized #[rstest], with a
named #[case::{name}(...)] for each existing scenario. Pass each case’s base
array, offsets, overlays, and expected values as parameters, then invoke resolve
and assert_i32_eq once in the shared test body; preserve all current cases and
expectations.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…intermediate struct Tighten the two pruning tests to assert the failure is a not-found error naming the deleted overlay file, not a bare is_err() that any unrelated failure would satisfy. Extend test_overlay_deeply_nested_subfield to also project the intermediate struct by id while the overlay targets a deeper leaf, covering the case where a top-level-only mapping would miss the overlay. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/lance/src/dataset/fragment.rs (2)
2657-2713: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnchecked slice indexing risks a panic if the row-count invariant ever breaks.
batch_offsets = &offsets_in_frag[start..start + num_rows as usize](line 2705) has no bounds check. Correctness today relies onrows_seennever exceedingoffsets_in_frag.len(), i.e. that the sum oftask.num_rowsacross all batches frommergedalways equalsparams.to_offsets_total(total_num_rows).len(). That invariant holds by construction in the reviewed callers, but a future change to either side (e.g. a caller passing mismatchedparams/total_num_rows, or a reader producing a different row count) would panic here instead of surfacing a clear error — this crashes the read task rather than propagating aResult::Err.Per coding guidelines, prefer
debug_assert!for this kind of internal invariant, or return an explicit error, rather than leaving it unguarded.🛡️ Suggested guard
task: async move { let batch_offsets = &offsets_in_frag[start..start + num_rows as usize]; + debug_assert!( + start + num_rows as usize <= offsets_in_frag.len(), + "overlay merge: batch rows_seen ({}) exceed offsets_in_frag len ({}) — \ + params/total_num_rows mismatch with the merged batch stream", + start + num_rows as usize, + offsets_in_frag.len() + ); merge_overlay_batch(inner, batch_offsets, &plans).await }As per coding guidelines: "Prefer
debug_assert!overassert!for non-safety invariants" and "Do not silently guard against impossible conditions; usedebug_assert!, return an explicit error, or remove the check."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/fragment.rs` around lines 2657 - 2713, Guard the batch slice in merge_overlays by validating that start + num_rows stays within offsets_in_frag before indexing, using debug_assert! for the internal row-count invariant or returning an explicit error that propagates through the task. Preserve the existing merge_overlay_batch flow for valid ranges and avoid leaving the unchecked slice as the only failure path.Source: Coding guidelines
912-967: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant full-fragment clone on every overlay-planning
open().Line 962 does
Arc::new(self.clone()), cloning the wholeFileFragment— including a fresh copy ofself.metadata, which was already deep-cloned one line earlier at line 952 (Arc::new(self.metadata.clone())) for the reader's own field. For a fragment with overlays, this doubles the metadata clone cost on everyopen()call.Consider reusing the metadata
Arc(or storing only whatOverlayReadStateactually needs — e.g.fragment_id+ a lighter handle) instead of cloning the entireFileFragmentagain.♻️ Possible approach
- let mut reader = FragmentReader::try_new( - self.id(), - deletion_vec, - row_id_sequence, - opened_files, - ArrowSchema::from(projection), - self.count_rows(None).await?, - num_physical_rows, - Arc::new(self.metadata.clone()), - )?; + let metadata = Arc::new(self.metadata.clone()); + let mut reader = FragmentReader::try_new( + self.id(), + deletion_vec, + row_id_sequence, + opened_files, + ArrowSchema::from(projection), + self.count_rows(None).await?, + num_physical_rows, + metadata, + )?; // Plan overlay resolution from coverage metadata (no files opened here); the // readers are opened lazily on read, pruned to the rows each read touches. if !self.metadata.overlays.is_empty() { let planner = plan_overlays(self, projection)?; if !planner.is_empty() { reader.overlay = Some(OverlayReadState { planner: Arc::new(planner), - fragment: Arc::new(self.clone()), + fragment: Arc::new(self.clone()), // still needs the full FileFragment (dataset handle), just avoid re-cloning metadata separately if possible read_config: Arc::new(read_config.clone()), }); } }Whether this is worth doing depends on how expensive
FileFragment::clone()actually is (i.e. whethermetadatais the bulk of the cost) — worth confirming against the actual struct definition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/fragment.rs` around lines 912 - 967, Remove the full-fragment clone in the overlay setup within Fragment::open: replace OverlayReadState’s Arc::new(self.clone()) usage with a representation containing only the data overlay reads require, reusing the metadata Arc already created for FragmentReader::try_new where applicable. Update OverlayReadState and its consumers accordingly while preserving lazy overlay planning and reads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rust/lance/src/dataset/fragment.rs`:
- Around line 2657-2713: Guard the batch slice in merge_overlays by validating
that start + num_rows stays within offsets_in_frag before indexing, using
debug_assert! for the internal row-count invariant or returning an explicit
error that propagates through the task. Preserve the existing
merge_overlay_batch flow for valid ranges and avoid leaving the unchecked slice
as the only failure path.
- Around line 912-967: Remove the full-fragment clone in the overlay setup
within Fragment::open: replace OverlayReadState’s Arc::new(self.clone()) usage
with a representation containing only the data overlay reads require, reusing
the metadata Arc already created for FragmentReader::try_new where applicable.
Update OverlayReadState and its consumers accordingly while preserving lazy
overlay planning and reads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6c4c2d5-294c-4cf5-b402-b037e76a3dd3
📒 Files selected for processing (1)
rust/lance/src/dataset/fragment.rs
"atom" was coined terminology. Rename it to "atomic field", matching the existing `atomic layout` concept in lance-core (field.rs), which already denotes a field treated as one indivisible per-row unit whose interior children aren't addressed individually. Types: LoadedAtom -> LoadedAtomicField, PlannedAtom -> PlannedAtomicField, and the *Overlay/enumerate_/*_id counterparts. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance/src/dataset/overlay.rs`:
- Line 684: Rename the local variable assigned from
plan.fetch_projection.fields[0] from atomic_field to fetch_top_field, reflecting
that it is the pruned top-level projection field. Update the corresponding
descend_by_ids call to pass fetch_top_field, leaving the traversal behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4193f83f-c2fb-4d27-852c-b00f8559110f
📒 Files selected for processing (2)
rust/lance/src/dataset/fragment.rsrust/lance/src/dataset/overlay.rs
| // Fetch each overlay's values and descend to the atomic field array. The fetch is | ||
| // projected to the atomic field's ancestor path, so the fetched column is the pruned | ||
| // top-level column; `descend_by_ids` walks it down to the atomic field. | ||
| let atomic_field = &plan.fetch_projection.fields[0]; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Misleading variable name: this is the fetch projection's top-level field, not the atomic field.
plan.fetch_projection.fields[0] is the pruned top-level column (the root of the ancestor spine), which is exactly what descend_by_ids(&column, atomic_field, &plan.ancestor_ids) needs as its field root — it then walks down ancestor_ids to the atomic field. Naming it atomic_field invites the opposite reading. Consider fetch_top_field for parity with plan.top_field used on the base side at Line 716.
♻️ Suggested rename
- let atomic_field = &plan.fetch_projection.fields[0];
+ let fetch_top_field = &plan.fetch_projection.fields[0];Then update the descend_by_ids(&column, atomic_field, ...) call site accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance/src/dataset/overlay.rs` at line 684, Rename the local variable
assigned from plan.fetch_projection.fields[0] from atomic_field to
fetch_top_field, reflecting that it is the pruned top-level projection field.
Update the corresponding descend_by_ids call to pass fetch_top_field, leaving
the traversal behavior unchanged.
…c fields enumerate_atomic_fields tested `matches!(field.data_type(), DataType::Struct(_))` at every node. Field::data_type() rebuilds the whole arrow DataType of a struct's subtree on each call, so enumerating a wide/deeply-nested projection was superlinear (~O(N * depth)) with heavy transient allocation. Use the O(1) `logical_type.is_struct()` check instead; same semantics, no allocation. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
Thanks for testing the extra cases. This covers the possibilities I can think of. Any others we can address as we find them.
| // Projecting the *intermediate* struct `outer.middle` (field id 2) while | ||
| // the overlay targets a deeper field (id 3) must still apply: the | ||
| // overlay's leaf id falls inside the projected subtree, so it maps to a | ||
| // projected atomic field. (This is the case wjones127/westonpace flagged where a |
There was a problem hiding this comment.
Some of these test comments are probably a little superfluous but I know how hard it is to avoid generating them 😆
A minimal, first slice of overlay-aware compaction (OSS-1326), stacked on #7536 (OSS-1324, `will/oss-1324-take-can-read-overlays`) — review against that base. ## What it does Adds a `max_overlays_per_fragment: Option<usize>` option to `CompactionOptions`. When a fragment carries **more than** this many data overlay files, the planner marks it `CompactItself`, so `compact_files` fully rewrites it into a fresh fragment with its overlays **and** deletions materialized into the base data. A fragment at or below the limit is not a candidate on this basis. The read side needs no new machinery: compaction already scans each fragment through the normal read path, which (per #7536) resolves overlays and applies deletions, so the merged post-image is what gets written to the new base file. ### Index correctness A full rewrite removes the overlays, and with them the staleness signal the query path uses to mask an index older than an overlay. So the `Rewrite` commit now drops the rewritten fragment from the coverage of any index left stale by those overlays — **field-aware** (only indices covering a field an overlay actually supplied) and **version-gated** (`overlay.committed_version > index.dataset_version`). Those rows fall back to a flat scan until reindex; an index is never left serving stale values. Non-stale indices (built at/after the overlay, or on un-overlaid fields) are remapped normally. This reuses the existing `Operation::Rewrite` path — no new persisted operation or conflict-matrix change. ## Scope / follow-ups Only the "fully compact the fragment" strategy is implemented. Overlay→overlay merge, in-place overlay→base folds (column rewrite preserving row addresses), and rebuild-in-place index reconciliation are left as follow-ups. ## Tests - Planner/e2e (`optimize.rs`): overlay count over the limit triggers a full compaction (fresh single-file fragment, overlays cleared, values materialized); at-or-below limit is a no-op; deletions are materialized alongside overlays; a stale scalar index is dropped from the compacted fragment's coverage and the indexed query stays correct. - Unit (`transaction.rs`): `prune_overlay_stale_fields_from_indices` is field-aware and version-gated (stale index dropped; index at/after the overlay kept; index on an un-overlaid field untouched). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a configurable overlay-count trigger for compaction; fragments exceeding the limit are compacted into standalone form. * Default limit is 10 overlays per fragment, configurable via `lance.compaction.max_overlays_per_fragment` (set to `none` to disable). * **Bug Fixes** * Prevented stale index coverage from returning outdated values after overlay/materialization compaction. * Improved accuracy by pruning rewritten fragment coverage from older index versions, causing queries to fall back to scans where needed. * **Tests** * Added coverage for the overlay trigger and index-staleness pruning behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
Exposes the `DataOverlay` commit operation to Python so overlays can be created and committed from Python (needed to benchmark and use data overlay files without dropping into Rust). Mirrors the existing `DataReplacement` binding. ## What's here - `LanceOperation.DataOverlay` with `DataOverlayFile` and `DataOverlayGroup`. A `DataOverlayFile` carries the value `DataFile` plus **exactly one** of `shared_offsets` (dense coverage shared by every field) or `field_offsets` (sparse, one offset set per field). The commit stamps `committed_version`; passing both/neither coverage is rejected with a clear error. - PyO3 conversions (both directions) in `python/src/transaction.rs`. - Fills in the `overlays` field on the Python `FragmentMetadata -> Fragment` conversion, which OSS-1322 left unset (Python metadata doesn't carry overlays — they're committed via `DataOverlay`). ## Tests `test_data_overlay_*` in `test_dataset.py`: dense round-trip resolves on read; newest overlay wins; sparse per-field coverage resolves fields independently; and the exactly-one-coverage validation rejects both/neither. ## Stacking Stacked on **#7536** (OSS-1324 read path) — base retargets automatically when it lands. Next PR (benchmarks) stacks on this. > Note: the `python/src/fragment.rs` one-liner arguably belongs in the OSS-1322 PR (#7535), which added `Fragment.overlays` without updating the binding; included here so the stack compiles. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added cell-level `DataOverlay` operations to append fragment overlays without rewriting entire fragments. - Supports dense and sparse overlays, with newest overlapping values taking precedence. - Persists overlay metadata on fragments and carries optional committed version stamps. - **Bug Fixes** - Preserves overlay metadata correctly through JSON serialization and Python↔Rust round-trips. - **Tests** - Added end-to-end coverage for dense/sparse overlays, precedence behavior, metadata round-tripping, and offset validation. - **Documentation/Chores** - Updated fragment metadata `repr` expectations and improved default `max_bytes_per_file` for fragment writing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
A minimal, first slice of overlay-aware compaction (OSS-1326), stacked on #7536 (OSS-1324, `will/oss-1324-take-can-read-overlays`) — review against that base. ## What it does Adds a `max_overlays_per_fragment: Option<usize>` option to `CompactionOptions`. When a fragment carries **more than** this many data overlay files, the planner marks it `CompactItself`, so `compact_files` fully rewrites it into a fresh fragment with its overlays **and** deletions materialized into the base data. A fragment at or below the limit is not a candidate on this basis. The read side needs no new machinery: compaction already scans each fragment through the normal read path, which (per #7536) resolves overlays and applies deletions, so the merged post-image is what gets written to the new base file. ### Index correctness A full rewrite removes the overlays, and with them the staleness signal the query path uses to mask an index older than an overlay. So the `Rewrite` commit now drops the rewritten fragment from the coverage of any index left stale by those overlays — **field-aware** (only indices covering a field an overlay actually supplied) and **version-gated** (`overlay.committed_version > index.dataset_version`). Those rows fall back to a flat scan until reindex; an index is never left serving stale values. Non-stale indices (built at/after the overlay, or on un-overlaid fields) are remapped normally. This reuses the existing `Operation::Rewrite` path — no new persisted operation or conflict-matrix change. ## Scope / follow-ups Only the "fully compact the fragment" strategy is implemented. Overlay→overlay merge, in-place overlay→base folds (column rewrite preserving row addresses), and rebuild-in-place index reconciliation are left as follow-ups. ## Tests - Planner/e2e (`optimize.rs`): overlay count over the limit triggers a full compaction (fresh single-file fragment, overlays cleared, values materialized); at-or-below limit is a no-op; deletions are materialized alongside overlays; a stale scalar index is dropped from the compacted fragment's coverage and the indexed query stays correct. - Unit (`transaction.rs`): `prune_overlay_stale_fields_from_indices` is field-aware and version-gated (stale index dropped; index at/after the overlay kept; index on an un-overlaid field untouched). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a configurable overlay-count trigger for compaction; fragments exceeding the limit are compacted into standalone form. * Default limit is 10 overlays per fragment, configurable via `lance.compaction.max_overlays_per_fragment` (set to `none` to disable). * **Bug Fixes** * Prevented stale index coverage from returning outdated values after overlay/materialization compaction. * Improved accuracy by pruning rewritten fragment coverage from older index versions, causing queries to fall back to scans where needed. * **Tests** * Added coverage for the overlay trigger and index-staleness pruning behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]> (cherry picked from commit 6b15a8e)
Exposes the `DataOverlay` commit operation to Python so overlays can be created and committed from Python (needed to benchmark and use data overlay files without dropping into Rust). Mirrors the existing `DataReplacement` binding. ## What's here - `LanceOperation.DataOverlay` with `DataOverlayFile` and `DataOverlayGroup`. A `DataOverlayFile` carries the value `DataFile` plus **exactly one** of `shared_offsets` (dense coverage shared by every field) or `field_offsets` (sparse, one offset set per field). The commit stamps `committed_version`; passing both/neither coverage is rejected with a clear error. - PyO3 conversions (both directions) in `python/src/transaction.rs`. - Fills in the `overlays` field on the Python `FragmentMetadata -> Fragment` conversion, which OSS-1322 left unset (Python metadata doesn't carry overlays — they're committed via `DataOverlay`). ## Tests `test_data_overlay_*` in `test_dataset.py`: dense round-trip resolves on read; newest overlay wins; sparse per-field coverage resolves fields independently; and the exactly-one-coverage validation rejects both/neither. ## Stacking Stacked on **#7536** (OSS-1324 read path) — base retargets automatically when it lands. Next PR (benchmarks) stacks on this. > Note: the `python/src/fragment.rs` one-liner arguably belongs in the OSS-1322 PR (#7535), which added `Fragment.overlays` without updating the binding; included here so the stack compiles. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added cell-level `DataOverlay` operations to append fragment overlays without rewriting entire fragments. - Supports dense and sparse overlays, with newest overlapping values taking precedence. - Persists overlay metadata on fragments and carries optional committed version stamps. - **Bug Fixes** - Preserves overlay metadata correctly through JSON serialization and Python↔Rust round-trips. - **Tests** - Added end-to-end coverage for dense/sparse overlays, precedence behavior, metadata round-tripping, and offset validation. - **Documentation/Chores** - Updated fragment metadata `repr` expectations and improved default `max_bytes_per_file` for fragment writing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]> (cherry picked from commit 9681621)
…es (#7549) Index masking for data overlay files. Builds on the overlay read path (#7536), now merged. ## Problem An index built before an overlay does not reflect the overlay's values, so its entries for overlay-covered cells may be stale. `WHERE age = 25` after an overlay sets a row's age to 26 must not return that row from the index; `WHERE age = 26` must find it. Queries must stay correct while overlays remain. ## Approach For each index a query relies on, compute which row addresses within each covered fragment carry an overlay committed *after* the index (`committed_version > index.dataset_version`) and touching a field the index covers. The check is: - **field-aware** — an overlay touching only non-indexed fields excludes nothing; - **version-gated** — an overlay already incorporated by the index (`committed_version <= index.dataset_version`) is ignored. This is the new `overlay_exclusion_offsets` helper in `dataset/overlay.rs`. Such rows are excluded from the index result and re-evaluated against their current (overlay-merged) values: - **Scalar (BTree)** (row-level): stale row addresses are blocked from `MaterializeIndexExec` output via `overlay_block: Option<RowAddrMask>`. Only those rows are re-evaluated via a targeted `TakeExec` + full-filter path. Non-stale rows in the same fragment remain on the indexed path. - **Vector ANN** (row-level): stale row addresses are passed to `DatasetPreFilter.overlay_block`, blocking them from ANN results. Only those rows are re-scored via a targeted `TakeExec` + flat-KNN path. Non-stale rows in the same fragment stay in ANN coverage. - **FTS** (fragment-level): FTS segments covering stale fragments are excluded from `MatchQueryExec` (via `new_with_segments`). Stale fragments fall to the existing `FlatMatchQueryExec` path, which reads current overlay-merged values. Fragment granularity is sufficient because flat text matching is row-precise and cheap relative to flat-KNN. This drops stale index hits and surfaces new matches the index never saw. ## Performance Benchmark: 1M rows, 100k rows/fragment (10 frags), 32-dim vec, NVMe, release build. ### Scalar (BTree) — `age = <value>` | overlays | stale_frag_ms | clean_frag_ms | |---|---:|---:| | 0 | 0.4 | 0.4 | | 1 | 0.9 | 0.6 | | 4 | 0.5 | 0.6 | | 16 | 0.6 | 0.6 | The overhead when overlays exist comes from the stale-check itself (segment metadata load), not from flat-scanning — only stale rows are taken, not the whole fragment. ### Vector ANN — fragment 0 with 1 overlaid row | vec_overlays | ann_ms | notes | |---|---:|---| | 0 | 9.6 | no overlays | | 1 | 10.4 | row 0 blocked, re-scored via targeted take | Previously at fragment-level: 10.0ms → 11.9ms (+1.9ms scanning 100k rows flat). With row-level: +0.8ms for targeted take of 1 row — overhead is O(stale_rows), not O(fragment_size). Fast-path summary: - **No overlays** (common case): O(num_fragments) boolean check, zero allocations. - **Overlays predate the index**: per-fragment version gate skips all field/bitmap work. - **Stale overlays exist**: index metadata from cache; `overlay_exclusion_offsets` called once per covered fragment with overlays. ## Tests `dataset::tests::dataset_overlay_index_masking` (e2e) and `dataset::overlay` (unit): - BTree: stale-drop and new-match; row-level precision (non-stale row in same fragment returned alongside stale row via Take path) - Compound AND expression: both leaf index searches collect stale rows - Overlay on an unrelated field excludes nothing - Overlay with `committed_version <= index.dataset_version` not excluded - NULL override - Multi-fragment - Vector index: stale row dropped, overlay-updated row re-scored back into the top-k - FTS: stale term not returned; new term found via flat path; overlay on non-FTS field excludes nothing - `overlay_exclusion_offsets` unit tests: version gate, field-awareness, sparse per-field coverage, multi-overlay union 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Resolves the
takerandom-access path against data overlay files, replacing the temporary "overlays not supported" error from OSS-1322. Implements OSS-1324.Because
takeand scan shareFragmentReader's read path, the merge is wired there once: each row is addressed by its physical offset (fromReadBatchParams::to_offsets_total) and resolved against the overlays that cover its field. This also enables the scan-path merge that the OSS-1322 PR stubbed out.How it works
takeprimitive — so overlay reads are O(requested rows), not O(coverage).interleave(rank → fetch-position remap).Overlays on nested (non-top-level) fields are not yet matched and are left for follow-up. There's a
TODO(overlay perf)on reader priority to settle with a benchmark.Tests
take covered/uncovered offsets; multiple overlays (newest wins); per-field coverage with unequal-length columns; NULL override; overlay on a deleted row (inert); multi-fragment scan — each over v2.0 and v2.1. Plus unit tests for the routing/assembly core, and an IO guard (
test_take_reads_only_needed_overlay_ranks) asserting a 2-row take over a fully-covering 100k-row overlay reads ~one miniblock, not the whole value column.Stacking
Stacked on the OSS-1322 branch (
will/oss-1322-write-data-overlay-and-scan-data). Merge that first; this PR's base retargets tomainautomatically when it lands.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests