Cpp code dump#1
Merged
Merged
Conversation
westonpace
pushed a commit
that referenced
this pull request
Jul 25, 2024
before: <img width="710" alt="Screenshot 2024-07-21 at 4 24 44 PM" src="https://github.com/user-attachments/assets/65a953ac-bbbf-4244-b3fa-d5b7c368806e"> after: <img width="589" alt="Screenshot 2024-07-21 at 4 28 49 PM" src="https://github.com/user-attachments/assets/16a20200-d8f2-4e5a-8e59-2be42281222e"> to reproduce: `cargo run --release --example benchmark ` in `rust/lance-encoding/compression-algo/fsst` machine info: `11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz` `Linux 192 5.10.0-28-amd64 #1 SMP Debian 5.10.209-2 (2024-01-31) x86_64 GNU/Linux`
6 tasks
westonpace
pushed a commit
that referenced
this pull request
May 14, 2026
…es (#6767) ## Summary After a writer flushed a memtable to L0 and an external compactor merged that generation into the base table — legitimately draining `flushed_generations` to empty — a subsequent restart re-replayed the original WAL entries into the new active memtable, duplicating rows on read. Two bugs were interacting: 1. **Disambiguation:** `replay_memtable_from_wal` distinguished "fresh shard" from "flushed and compacted" via `flushed_generations.is_empty()`. That works in a closed-world deployment but breaks the moment an external compactor enters the picture — and the compactor is the *intended* consumer that drains that vector, so the signal is structurally broken under OSS-WAL. 2. **Cursor never advanced:** `MemTableFlusher::flush` read `covered_wal_entry_position` from `memtable.last_flushed_wal_entry_position()`, but that field is only set by the `mark_wal_flushed` test helper. In production it stayed at 0, so `replay_after_wal_entry_position` never advanced past 0. Under 0-based WAL positions this masked bug #1 — both "fresh" and "post-flush-of-0" produced cursor=0. ## Fix - **WAL positions are now 1-based** (`FIRST_WAL_ENTRY_POSITION = 1`). A cursor of `0` unambiguously means "no flush has stamped this shard," so replay collapses to `cursor.saturating_add(1)` without consulting `flushed_generations`. - **`WalFlushHandler::handle`** writes the just-appended position back into `state.last_flushed_wal_entry_position` under the state lock before signalling the completion cell. - **`MemTableFlusher::flush` / `flush_with_indexes`** now take an explicit `covered_wal_entry_position` arg. The production caller derives it per-memtable from the `WalFlushResult` carried in the completion cell — authoritative under concurrent flushes — falling back to `memtable.frozen_at_wal_entry_position()` when freeze did not trigger a flush. - **State seed at open** uses the post-replay WAL tip, not `manifest.wal_entry_position_last_seen` (the latter is bumped on every tailer read and can sit above any flushed generation). - Proto field docs on `ShardManifest.replay_after_wal_entry_position` / `wal_entry_position_last_seen` updated to spell out the 1-based convention and what default-0 means. ## Test plan - [x] Added `test_memtable_replay_skips_entries_after_external_compaction` in `rust/lance/src/dataset/mem_wal/write.rs`: open writer, put rows, close (flush), simulate the compactor by directly committing a manifest with empty `flushed_generations`, reopen, assert the memtable is empty. Fails on the pre-fix code; passes now. - [x] `cargo test -p lance --lib dataset::mem_wal` — 236/236 pass - [x] `cargo test -p lance --lib` — 1600/1600 pass - [x] `cargo test -p lance-index --lib` — 302/302 pass - [x] `cargo clippy --all --tests --benches -- -D warnings` — clean - [x] `cargo fmt --all -- --check` — clean ## Compatibility WAL position numbering changes from 0-based to 1-based. Existing on-disk manifests / WAL files written by the prior `oss-wal-multiplex` code are not migrated — coordinated with downstream consumers (sophon) to start fresh. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
geruh
referenced
this pull request
in geruh/lance
May 16, 2026
…es (lance-format#6767) ## Summary After a writer flushed a memtable to L0 and an external compactor merged that generation into the base table — legitimately draining `flushed_generations` to empty — a subsequent restart re-replayed the original WAL entries into the new active memtable, duplicating rows on read. Two bugs were interacting: 1. **Disambiguation:** `replay_memtable_from_wal` distinguished "fresh shard" from "flushed and compacted" via `flushed_generations.is_empty()`. That works in a closed-world deployment but breaks the moment an external compactor enters the picture — and the compactor is the *intended* consumer that drains that vector, so the signal is structurally broken under OSS-WAL. 2. **Cursor never advanced:** `MemTableFlusher::flush` read `covered_wal_entry_position` from `memtable.last_flushed_wal_entry_position()`, but that field is only set by the `mark_wal_flushed` test helper. In production it stayed at 0, so `replay_after_wal_entry_position` never advanced past 0. Under 0-based WAL positions this masked bug #1 — both "fresh" and "post-flush-of-0" produced cursor=0. ## Fix - **WAL positions are now 1-based** (`FIRST_WAL_ENTRY_POSITION = 1`). A cursor of `0` unambiguously means "no flush has stamped this shard," so replay collapses to `cursor.saturating_add(1)` without consulting `flushed_generations`. - **`WalFlushHandler::handle`** writes the just-appended position back into `state.last_flushed_wal_entry_position` under the state lock before signalling the completion cell. - **`MemTableFlusher::flush` / `flush_with_indexes`** now take an explicit `covered_wal_entry_position` arg. The production caller derives it per-memtable from the `WalFlushResult` carried in the completion cell — authoritative under concurrent flushes — falling back to `memtable.frozen_at_wal_entry_position()` when freeze did not trigger a flush. - **State seed at open** uses the post-replay WAL tip, not `manifest.wal_entry_position_last_seen` (the latter is bumped on every tailer read and can sit above any flushed generation). - Proto field docs on `ShardManifest.replay_after_wal_entry_position` / `wal_entry_position_last_seen` updated to spell out the 1-based convention and what default-0 means. ## Test plan - [x] Added `test_memtable_replay_skips_entries_after_external_compaction` in `rust/lance/src/dataset/mem_wal/write.rs`: open writer, put rows, close (flush), simulate the compactor by directly committing a manifest with empty `flushed_generations`, reopen, assert the memtable is empty. Fails on the pre-fix code; passes now. - [x] `cargo test -p lance --lib dataset::mem_wal` — 236/236 pass - [x] `cargo test -p lance --lib` — 1600/1600 pass - [x] `cargo test -p lance-index --lib` — 302/302 pass - [x] `cargo clippy --all --tests --benches -- -D warnings` — clean - [x] `cargo fmt --all -- --check` — clean ## Compatibility WAL position numbering changes from 0-based to 1-based. Existing on-disk manifests / WAL files written by the prior `oss-wal-multiplex` code are not migrated — coordinated with downstream consumers (sophon) to start fresh. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
This was referenced May 22, 2026
hamersaw
referenced
this pull request
in hamersaw/lance
Jun 4, 2026
…KeyIndex Per jackye1995's review #1 ("composite-key BTreeMemIndex"): instead of a separate parallel skiplist (`PkKeyIndex`), the composite PK index is a plain `BTreeMemIndex` keyed on a synthetic `Binary` column (`__pk_key__`) holding the order-preserving encoded tuple. `pk_key.rs` is now just the encoder (`encode_pk_tuple` + `encode_pk_batch`); the insert path materializes the encoded `Binary` column and feeds the existing index, and the probe seeks with `ScalarValue::Binary(encode_pk_tuple(values))`. Benefits: the composite case reuses `BTreeMemIndex`'s byte backend (incl. the inline-small-key node optimization) and its `to_training_batches`, so the in-memory probe, flush sidecar, and single-column path share one index type and one code path. Arity-split and its single-column memory/typed-fast-path wins are unchanged. Net deletion of the hand-rolled `PkKeyIndex` skiplist. mem_wal suite green; clippy --tests -D warnings clean. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
3 tasks
westonpace
added a commit
that referenced
this pull request
Jul 8, 2026
…rvation (#7675) Raises the per-partition memory pool from 100MB to 150MB and sets the sort spill reservation to 40MB (up from the DataFusion default of 10MB) to give sort operations more headroom while spilling to disk (we should still spill at roughly the same rate). The previous defaults were 100MB / 10MB. This _usually_ worked but certain patterns would lead to false memory exhaustion errors: ``` OSError: LanceError(IO): Resources exhausted: Additional allocation failed for ExternalSorterMerge[0] with top memory consumers (across reservations) as: ExternalSorterMerge[0]#1(can spill: false) consumed 58.5 MB, peak 58.5 MB, ExternalSorter[0]#0(can spill: true) consumed 41.4 MB, peak 89.9 MB. Error: Failed to allocate additional 345.9 KB for ExternalSorterMerge[0] with 27.6 MB already allocated for this reservation - 92.2 KB remain available for the total pool, /home/pace/lance/rust/lance-datafusion/src/chunker.rs:49:46 ``` The problem happens as follows: 1. The sort node accumulates batches of data without modifying them until it determines a spill is needed. During this phase each batch counts double against the pool reservation. This is meant to provide overhead for the later steps. In our above example we can see spilling was triggered at 41.4MB which is about half of the 90MB pool (half, because each batch is counted double) 2. The sort node determines that spilling is needed. First, it must sort the data that has accumulated in memory. Each batch is sorted by itself. This batch sort is in-place and doesn't affect reservations much. 3. A cursor is created for each in-memory batch. The in-memory batches are then fed into a merge sort. 4. The merge sort accumulates batches of data to send to the spill. Once a batch is accumulated it is written to the spill file. Both the cursors and the accumulation require additional space. This is the `ExternalSorterMerge[0]` mentioned above. It is given the overcounting described in step 1. In other words, once this starts, we have half the reservation in `ExternalSorter[0]` and half the reservation in `ExternalSorterMerge[0]`. This "additional space" _should_ be about the same size as the input. This is why we count each batch twice. In practice, the `ExternalSorterMerge[0]` reservation ends up being slightly higher (for various reasons). This is what the `sort_spill_reservation_bytes` is supposed to account for. Datafusion defaults this to 10MB. There is no guidance (and I can't get Claude to come up with any good guidance) as to what this value should be set to. However, 10MB seems like too little. This PR updates it to about 1/3 of the memory pool size. In theory it shouldn't grow proportionally to the memory pool but in practice it seems to. I also really don't want to expose it as yet another knob that users have to tune so I'm hoping 1/3 is slightly conservative but good enough. --------- Co-authored-by: Claude Sonnet 4.6 <[email protected]>
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.
The initial patch to open source C++ codebase.