feat: some optimistic paths for instant manipulate#7812
feat: some optimistic paths for instant manipulate#7812
Conversation
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
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 introduces significant performance optimizations within the Highlights
Changelog
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 introduces an optimistic execution path for InstantManipulate to improve performance by avoiding unnecessary array materialization, particularly when using __tsid. The changes are mostly centered around instant_manipulate.rs, with a key modification in take_record_batch_optional to reuse constant columns. While the optimization itself is well-implemented, I've identified a critical issue in the planner logic that currently prevents this new fast path from being activated. Additionally, I've suggested an improvement to make the column resolution logic more robust. Addressing these points will ensure the new feature works as intended and improves the overall resilience of the query plan.
src/query/src/promql/planner.rs
Outdated
| .time_index_column | ||
| .clone() | ||
| .expect("time index should be set in `setup_context`"), | ||
| self.ctx.tag_columns.clone(), |
There was a problem hiding this comment.
The tag_columns passed to InstantManipulate::new here is incorrect when use_tsid is true. self.ctx.tag_columns holds the original tag columns from the table metadata, not ["__tsid"]. This prevents the reuse_all_non_sample_columns optimization in InstantManipulate from being triggered, which undermines a key goal of this pull request.
To fix this, you should conditionally pass vec!["__tsid".to_string()] when self.ctx.use_tsid is true, similar to how series_key_columns is determined for SeriesDivide.
if self.ctx.use_tsid {
vec![store_api::metric_engine_consts::DATA_SCHEMA_TSID_COLUMN_NAME.to_string()]
} else {
self.ctx.tag_columns.clone()
},| let LogicalPlan::Extension(Extension { node }) = input else { | ||
| return Vec::new(); | ||
| }; | ||
|
|
||
| node.as_any() | ||
| .downcast_ref::<SeriesDivide>() | ||
| .map(|series_divide| series_divide.tags().to_vec()) | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
This implementation of resolve_tag_columns only inspects the immediate input of InstantManipulate. However, the planner constructs a plan where the input is a SeriesNormalize node, which in turn wraps the SeriesDivide node. Consequently, the downcast_ref::<SeriesDivide>() will fail, and tag columns won't be resolved from the input plan if they are not explicitly provided.
While the primary fix should be in the planner to pass the correct tag_columns, making this function more robust by looking through SeriesNormalize would be a valuable improvement for future use cases. You might consider traversing the plan to find the underlying SeriesDivide node.
| let reuse_all_non_sample_columns = | ||
| matches!(self.tag_columns.as_slice(), [tag] if tag == "__tsid"); |
There was a problem hiding this comment.
So we only set reuse_all_non_sample_columns to true when there are no tags except tsid?
| .with_exprs_and_inputs(vec![], vec![series_divide]) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(plan.tag_columns, vec!["__tsid".to_string()]); |
There was a problem hiding this comment.
Also assert reuse_all_non_sample_columns?
| reuse_all_non_sample_columns: true, | ||
| input, | ||
| metric: ExecutionPlanMetricsSet::new(), | ||
| }); |
There was a problem hiding this comment.
In non-test code, we won't set reuse_all_non_sample_columns to true because tags also contain a host column. Is it expected?
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe2a9f91d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.reuse_all_non_sample_columns && Some(index) != self.field_index { | ||
| arrays.push(reuse_constant_column(array, output_len)?); |
There was a problem hiding this comment.
take() path
When __tsid is used as the series key, this branch treats every non-time, non-field_index column as a constant and rebuilds it with reuse_constant_column(). PromPlanner::prom_vector_selector_to_plan still passes only self.ctx.field_columns.first() into InstantManipulate, so on metric-engine tables with multiple value columns (field_1, field_2, …) those extra samples now bypass take_indices entirely. Any instant query that expands or skips points via lookback will therefore return stale/corrupted values for the secondary fields instead of the rows selected by compute::take().
Useful? React with 👍 / 👎.
2 participants
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?
Some optimistic paths to avoid unnecessary array materialization
PR Checklist
Please convert it to a draft if some of the following conditions are not met.