refactor(mito2): remove PartitionTreeMemtable#8080
refactor(mito2): remove PartitionTreeMemtable#8080evenyag wants to merge 11 commits intoGreptimeTeam:mainfrom
Conversation
Signed-off-by: evenyag <realevenyag@gmail.com>
Restore PartitionTreeMemtable construction when memtable.type=partition_tree is explicit, and move the sparse-encoding bulk override into the default (no explicit memtable.type) arm so phase 2's memtable.type=bulk wins on reopen. Rewrite test_reopen_time_series_sparse_memtable_with_bulk to use a metric-engine-shaped schema and sparse-encoded rows with WriteHint::Sparse, so the test actually exercises a PartitionTreeMemtable in phase 1 and verifies WAL replay into the new BulkMemtable on reopen without flushing. Signed-off-by: evenyag <realevenyag@gmail.com>
Re-apply the unconditional sparse-encoding override in `MemtableBuilderProvider::builder_for_options` and route the `MemtableOptions::PartitionTree` arm to `BulkMemtable` with a deprecation warning. After this change, `PartitionTreeMemtableBuilder` is no longer reachable from the engine runtime; benchmarks still reference the type. Remove `test_reopen_time_series_sparse_memtable_with_bulk` and the `put_sparse_rows` helper added in the previous commit — that test only existed to validate the PartitionTree -> Bulk reopen migration and is unnecessary now that the override is in place. Signed-off-by: evenyag <realevenyag@gmail.com>
Relocate the timestamp_array_to_i64_slice helper from memtable/partition_tree/data.rs to the read module so that the read path no longer depends on the partition_tree internals. All call sites (both inside and outside the partition_tree module) now import from crate::read. Signed-off-by: evenyag <realevenyag@gmail.com>
The time_partition tests use the memtable builder purely as a generic backend for the TimePartitions write/scan paths; nothing in them is specific to the partition-tree memtable. Switch the seven affected tests to TimeSeriesMemtableBuilder so the tests no longer depend on PartitionTreeMemtableBuilder. Signed-off-by: evenyag <realevenyag@gmail.com>
The runtime already falls back to BulkMemtable for the PartitionTree variant. Drop the now-unreachable implementation, its metrics, the partition_tree benchmarks, the metric-engine Unsupported fallback in bulk_insert.rs, and the test helpers that only existed for the deleted module. MemtableOptions::PartitionTree, its parsing, the runtime fallback, the store-api MEMTABLE_PARTITION_TREE_* constants, and the SQL fixtures remain so existing region options keep round-tripping. Signed-off-by: evenyag <realevenyag@gmail.com>
PartitionTreeMemtable was the only caller passing skip_partition_column=true; every other caller passes false. Now that the partition_tree module is gone, the parameter is uniformly false and the guard branch is dead. Drop the parameter from the trait method and both impls, remove the guard and the is_partition_column helper, and update the four remaining call sites in mito2 plus the bench. Signed-off-by: evenyag <realevenyag@gmail.com>
Signed-off-by: evenyag <realevenyag@gmail.com>
Signed-off-by: evenyag <realevenyag@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request removes the experimental PartitionTreeMemtable implementation and transitions its functionality to BulkMemtable, which now serves as the default for regions using sparse primary key encoding. The changes involve a comprehensive cleanup of the partition_tree module, associated configuration options, metrics, and benchmarks. Feedback highlights that the current implementation for sparse encoding fails to respect user-provided BulkMemtableConfig, overriding it with defaults; it is suggested to update the logic to preserve these configurations when present.
| if primary_key_encoding == PrimaryKeyEncoding::Sparse { | ||
| if options.memtable.is_some() { | ||
| common_telemetry::info!( | ||
| "Overriding memtable config, use BulkMemtable for sparse primary key encoding" | ||
| ); | ||
| } | ||
| return Arc::new( | ||
| BulkMemtableBuilder::new(self.write_buffer_manager.clone(), !dedup, merge_mode) | ||
| .with_compact_dispatcher(self.compact_dispatcher.clone()), | ||
| ); | ||
| } |
There was a problem hiding this comment.
When primary_key_encoding is Sparse, the code forces the use of BulkMemtable. However, the current implementation ignores any user-provided BulkMemtableConfig in options.memtable because it initializes the BulkMemtableBuilder with default settings. This means that if a user explicitly configured bulk memtable options (like merge_threshold), those settings will be lost when sparse encoding is active. The logic should be updated to preserve the configuration if options.memtable is already set to Bulk.
| if primary_key_encoding == PrimaryKeyEncoding::Sparse { | |
| if options.memtable.is_some() { | |
| common_telemetry::info!( | |
| "Overriding memtable config, use BulkMemtable for sparse primary key encoding" | |
| ); | |
| } | |
| return Arc::new( | |
| BulkMemtableBuilder::new(self.write_buffer_manager.clone(), !dedup, merge_mode) | |
| .with_compact_dispatcher(self.compact_dispatcher.clone()), | |
| ); | |
| } | |
| if primary_key_encoding == PrimaryKeyEncoding::Sparse { | |
| let mut builder = BulkMemtableBuilder::new(self.write_buffer_manager.clone(), !dedup, merge_mode) | |
| .with_compact_dispatcher(self.compact_dispatcher.clone()); | |
| if let Some(MemtableOptions::Bulk(config)) = &options.memtable { | |
| builder = builder.with_config(config.clone()); | |
| } else if options.memtable.is_some() { | |
| common_telemetry::info!( | |
| "Overriding memtable config, use BulkMemtable for sparse primary key encoding" | |
| ); | |
| } | |
| return Arc::new(builder); | |
| } |
Signed-off-by: evenyag <realevenyag@gmail.com>
Signed-off-by: evenyag <realevenyag@gmail.com>
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
This PR removes PartitionTreeMemtable.
PR Checklist
Please convert it to a draft if some of the following conditions are not met.