Skip to content

Commit cb83004

Browse files
authored
refactor: remove capacity in Compactor::build_tables (#443)
1 parent 14edbf4 commit cb83004

File tree

3 files changed

+10
-5
lines changed

3 files changed

+10
-5
lines changed

guide/src/contribution/composite_primary_keys.md

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ This document outlines a practical, incremental plan to add composite (multi-col
6060

6161
- Extend `DynSchema` to track `primary_indices: Vec<usize>` in metadata (replacing the single `primary_key_index`).
6262
- Update `DynRecordRef::new` and readers to honor multiple PK indices.
63+
- Ensure tombstone writes (row == None) still populate all PK columns from `Ts.key` so ordering/lookups remain correct.
6364
- Define a composite key wrapper for `Value`/`ValueRef` (or generate a per-dyn-schema composite type if feasible). Initially out-of-scope for phase 1.
6465

6566
## Step-by-Step Plan
@@ -103,15 +104,16 @@ Phase 3: Dynamic records (optional)
103104
7. `DynSchema` multi-PK
104105
- Store `primary_indices` metadata; update dynamic arrays/refs to keep all PK columns in projections.
105106
- Provide a composite `ValueRef` key wrapper for in-memory operations.
106-
- Acceptance: dynamic tests mirroring integration scenarios pass.
107+
- Ensure tombstones populate PK components from `Ts.key` in builders (e.g., `DynRecordBuilder::push`).
108+
- Acceptance: dynamic tests mirroring integration scenarios pass, including tombstone rows retaining all PK components.
107109

108110
## Code Touchpoints
109111

110112
- Traits/APIs: `src/record/mod.rs` (Schema), `src/option.rs` (DbOption::new)
111113
- Read paths: `src/lib.rs` (get/scan/package), `src/transaction.rs` (get/scan)
112114
- Macro codegen: `tonbo_macros/src/record.rs`, `tonbo_macros/src/keys.rs`, `tonbo_macros/src/data_type.rs`
113115
- Key types: `src/record/key/composite/`
114-
- Dynamic (phase 3): `src/record/dynamic/*`
116+
- Dynamic (phase 3): `src/record/dynamic/*` (incl. tombstone handling in `DynRecordBuilder::push`)
115117

116118
## Testing Strategy
117119

@@ -126,6 +128,9 @@ Phase 3: Dynamic records (optional)
126128
- WAL/compaction unaffected (basic smoke tests).
127129
- (Optional) Property tests: ordering equivalence vs. native tuple lexicographic ordering when Option B is used.
128130

131+
- Tombstones:
132+
- For both single- and multi-column PKs, verify that tombstone rows (row == None) keep all PK column values populated from `Ts.key` in Arrow arrays and through `RecordRef::from_record_batch`.
133+
129134
## Backward Compatibility & Migration
130135

131136
- All existing single-PK code continues to work without changes due to default-impl fallbacks.
@@ -167,3 +172,4 @@ pub struct User {
167172
- [ ] Configure Parquet sorting/statistics for all PK components.
168173
- [ ] Add unit/trybuild/integration tests.
169174
- [ ] Update user guide (mention composite PK support and examples).
175+
- [ ] Ensure tombstone rows keep PK component values (builders/readers), validated by tests.

src/compaction/mod.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,8 @@ where
5959
) -> Result<(), CompactionError<R>> {
6060
let mut stream = MergeStream::<R>::from_vec(streams, u32::MAX.into(), None).await?;
6161

62-
// Kould: is the capacity parameter necessary?
6362
let mut builder =
64-
<R::Schema as RecordSchema>::Columns::builder(schema.arrow_schema().clone(), 8192);
63+
<R::Schema as RecordSchema>::Columns::builder(schema.arrow_schema().clone(), 0);
6564
let mut min = None;
6665
let mut max = None;
6766

src/stream/level.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,6 @@ impl<'level, R> LevelStream<'level, R>
6666
where
6767
R: Record,
6868
{
69-
// Kould: only used by Compaction now, and the start and end of the sstables range are known
7069
#[allow(clippy::too_many_arguments)]
7170
pub(crate) fn new(
7271
version: &Version<R>,
@@ -80,6 +79,7 @@ where
8079
ts: Timestamp,
8180
limit: Option<usize>,
8281
projection_mask: ProjectionMask,
82+
// TODO: Refactor some top level components to a context structure.
8383
fs: Arc<dyn DynFs>,
8484
parquet_lru: Arc<dyn DynLruCache<Ulid> + Send + Sync>,
8585
order: Option<Order>,

0 commit comments

Comments
 (0)