-
Notifications
You must be signed in to change notification settings - Fork 694
feat(optimizer): recommend index for backfill to users #23199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'm not sure if it's a good idea to provide such recommendation in SQL interface. Using index to accelerate queries (no matter batch or streaming) seems to be an advanced practice: typically users need to understand the locality of their query and learn to inspect & comprehend query plan with EXPLAIN
commands. Given that for batch queries, such topic is only covered in documentation, maybe we should be aligned with that for streaming queries as well. 🤔 cc @hzxa21 for discussion
if self.table_indexes().is_empty() { | ||
self.notice_recommended_index(columns); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the primary key of this table scan already has the best locality? Do we still recommend index here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, do we still send a notice when user is actually going to create an index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the primary key of this table scan already has the best locality? Do we still recommend index here?
Yes. We check it inside notice_recommended_index
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, do we still send a notice when user is actually going to create an index
What do you mean when user is actually going to create an index? If users have created the related indexes, it means we will select those indexes, so we won't recommend it. This PR only recommend indexes when there are no indexes could be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, when user is going to follow the instruction to issue a CREATE INDEX
command, will they still receive a notice for the TableScan of the index job? 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Because we only recommend an index when there is no indexes could be used, but once users create the index we recommend, the index must be able to be used.
Considering we are supporting index selection for backfilling, we should update the doc of index as well to tell users how to create an index to give the streaming queries a better locality . |
I think regardless of whether we give index recommendation via SQL notice. We need to add documentation in the doc site to guide user on how to create index to accelerate backfill and why. I have created an issue a couple weeks agao: risingwavelabs/risingwave-docs#617. Please provide more contexts if needed to help doc team draft the doc. Regarding to index recommendation via SQL, I think showing the notice only in |
Looks like this PR extends new SQL syntax or updates existing ones. Make sure that:
|
Adding an |
ACTION, | ||
ADAPTIVE, | ||
ADD, | ||
ADVISOR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI #23208
Close this PR, since we have locality enforcement right now. #23275 |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
explain (advisor)
+ sql.Example
Checklist
Documentation
Release note