fix: infer time index from column meta on derived table#8013
fix: infer time index from column meta on derived table#8013
Conversation
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
github-actions
Bot
added
size/M
docs-not-required
![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 enhances range query support to handle derived inputs, such as those resulting from a UNION ALL, by allowing the time index to be inferred from field metadata when the underlying table cannot be directly resolved. It also introduces a flag to prevent invalid default BY column inference in these scenarios and adds comprehensive test cases. Review feedback highlights a critical compilation error caused by the removal of the WildcardOptions import while it is still in use, as well as significant indentation inconsistencies in the newly added logic.
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
| Err(error) => { | ||
| if matches!(&error, catalog::error::Error::TableNotExist { .. }) | ||
| && metadata_time_index_expr.is_some() | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Could you add some comments to document why do we need this check?
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
killme2008
left a comment
killme2008
left a comment
There was a problem hiding this comment.
LGTM
Let’s release a new minor version once this PR is merged. @waynexia @fengjiachun
| @@ -485,21 +489,49 @@ impl RangePlanRewriter { | |||
| /// return `(time_index, [row_columns])` to the rewriter. | |||
| /// If the user does not explicitly use the `by` keyword to indicate time series, | |||
| /// `[row_columns]` will be use as default time series | |||
There was a problem hiding this comment.
Do we need to change the comment too?
| Err(error) => { | ||
| if matches!(&error, catalog::error::Error::TableNotExist { .. }) | ||
| && metadata_time_index_expr.is_some() | ||
| { | ||
| continue; | ||
| } |
3 participants
Signed-off-by: Ruihang Xia waynestxia@gmail.comI hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
try to infer correct time index from the column meta when operations like UNION produce a synthetic table. likely a regression from #7558
What's changed and what's your intention?
PR Checklist
Please convert it to a draft if some of the following conditions are not met.