Skip to content

Cpp code dump#1

Merged
eddyxu merged 8 commits into
mainfrom
lei/init_cpp
Jul 8, 2022
Merged

Cpp code dump#1
eddyxu merged 8 commits into
mainfrom
lei/init_cpp

Conversation

@eddyxu

@eddyxu eddyxu commented Jul 8, 2022

Copy link
Copy Markdown
Member

The initial patch to open source C++ codebase.

@eddyxu
eddyxu requested a review from changhiskhan July 8, 2022 06:08
@eddyxu
eddyxu merged commit 13f3e27 into main Jul 8, 2022
@eddyxu
eddyxu deleted the lei/init_cpp branch July 8, 2022 06:16
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`
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]>
@westonpace westonpace mentioned this pull request May 21, 2026
3 tasks
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]>
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant