Conversation
Signed-off-by: Lei, HUANG <mrsatangel@gmail.com>
github-actions
bot
added
size/M
docs-not-required
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the memtable iteration mechanism by deprecating and removing the direct Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a good refactoring that removes the test-only Memtable::iter method and replaces its usages with the more general Memtable::ranges() method. However, there is a potential issue in the implementation of IterBuilder for MemtableRanges that could lead to panics in tests for SimpleBulkMemtable. My review includes a suggestion to make this implementation more robust.
| assert_eq!(self.ranges.len(), 1); | ||
| self.ranges.values().next().unwrap().build_iter() |
There was a problem hiding this comment.
Using assert_eq! here can cause a panic if memtable.ranges() returns more than one range. This is a valid scenario for SimpleBulkMemtable as its ranges() implementation can produce multiple ranges, unlike its previous iter() implementation which performed a compaction. The compacting logic seems to have been lost in this refactoring.
To prevent panics in tests and improve robustness, it's safer to return an error when there isn't exactly one range.
if self.ranges.len() != 1 {
return UnsupportedOperationSnafu {
err_msg: format!(
"Building an iterator from MemtableRanges expects 1 range, but got {}",
self.ranges.len()
),
}
.fail();
}
self.ranges.values().next().unwrap().build_iter()There was a problem hiding this comment.
Pull request overview
This PR removes the test-only Memtable::iter API and updates memtable implementations, unit tests, and benchmarks to scan data via the ranges()/IterBuilder path instead.
Changes:
- Removed
Memtable::iterfrom theMemtabletrait and deleted per-memtable implementations. - Updated unit tests/benches to read via
ranges(...).build(...)or per-range iterators. - Simplified test-only helper code that existed mainly to support the deprecated iterator path.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mito2/src/test_util/memtable_util.rs | Drops EmptyMemtable::iter and relies on ranges() for test scaffolding. |
| src/mito2/src/memtable/time_series.rs | Removes iter impl; updates tests to build iterators from ranges(). |
| src/mito2/src/memtable/time_partition.rs | Updates tests to use ranges() + IterBuilder instead of iter(). |
| src/mito2/src/memtable/simple_bulk_memtable/test_only.rs | Removes deprecated iterator builder helper code, keeps only metadata accessor. |
| src/mito2/src/memtable/simple_bulk_memtable.rs | Removes iter impl; updates tests to use ranges()/RangesOptions. |
| src/mito2/src/memtable/partition_tree.rs | Removes iter from Memtable impl; updates some tests; removes predicate test. |
| src/mito2/src/memtable/bulk.rs | Removes unimplemented iter() stub. |
| src/mito2/src/memtable.rs | Removes Memtable::iter from trait; changes MemtableRanges’ IterBuilder::build. |
| src/mito2/benches/simple_bulk_memtable.rs | Bench updated to scan via ranges().build(...). |
| src/mito2/benches/memtable_bench.rs | Bench updated to scan/filter via ranges() for TimeSeriesMemtable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn build(&self, _metrics: Option<MemScanMetrics>) -> Result<BoxedBatchIterator> { | ||
| UnsupportedOperationSnafu { | ||
| err_msg: "MemtableRanges does not support build iterator", | ||
| } | ||
| .fail() | ||
| assert_eq!(self.ranges.len(), 1); | ||
| self.ranges.values().next().unwrap().build_iter() | ||
| } |
| use api::v1::{Mutation, OpType, Rows, SemanticType}; | ||
| use common_query::prelude::{greptime_timestamp, greptime_value}; | ||
| use common_time::Timestamp; | ||
| use datafusion_common::Column; | ||
| use datafusion_expr::{BinaryExpr, Expr, Literal, Operator}; | ||
| use datatypes::data_type::ConcreteDataType; | ||
| use datatypes::prelude::Vector; | ||
| use datatypes::scalars::ScalarVector; |
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?
Remove
Memtable::itermethod and implementations.PR Checklist
Please convert it to a draft if some of the following conditions are not met.