-
Notifications
You must be signed in to change notification settings - Fork 33
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
Implement analyze skipping index statement #284
Implement analyze skipping index statement #284
Conversation
Signed-off-by: Rupal Mahajan <[email protected]>
Signed-off-by: Rupal Mahajan <[email protected]>
…ch_spark into analyze-skipping-index Signed-off-by: Rupal Mahajan <[email protected]>
@@ -105,6 +106,10 @@ vacuumCoveringIndexStatement | |||
: VACUUM INDEX indexName ON tableName | |||
; | |||
|
|||
analyzeSkippingIndexStatement | |||
: ANALYZE SKIPPING INDEX ON tableName |
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.
Is this grammar finalized? What is the semantic meaning?
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.
This is proposed grammar. Please comment if you have any other suggestions. Analyze refers to examining data to get insights. This command will return recommendation for creating skipping index (skipping index columns with suggested data structure) based on table data.
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.
This is proposed grammar.
Any reference / compatibility analysis with the mainstream syntax?
Please comment if you have any other suggestions.
Just brainstorming -
ANALYZE TABLE tableName FOR SKIPPING INDEX RECOMMENDATIONS;
Or
ANALYZE TABLE tableName RECOMMEND SKIPPING INDEX COLUMNS;
The assumption is we may want to do more things other from the recommendation.
Signed-off-by: Rupal Mahajan <[email protected]>
Signed-off-by: Rupal Mahajan <[email protected]>
Signed-off-by: Rupal Mahajan <[email protected]>
Signed-off-by: Rupal Mahajan <[email protected]>
Signed-off-by: Rupal Mahajan <[email protected]>
Signed-off-by: Rupal Mahajan <[email protected]>
…ndex Signed-off-by: Rupal Mahajan <[email protected]>
Signed-off-by: Rupal Mahajan <[email protected]>
|
||
class DataTypeSkippingStrategy extends AnalyzeSkippingStrategy { | ||
|
||
val rules = Map( |
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'm thinking if more flexible to move this static mapping to config file? Or maybe not necessary for this P0 solution?
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.
good idea. added this here thinking it's specific to data type based recommendation and won't be used by other strategies(e.g. recommendation based on table stats).
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 will take this up as fast follow up because it will unblock sql plugin if we can finalize grammar before 2.13 release.
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.
If we also want to merge implementation in this PR, could you update user manual like this? https://github.com/opensearch-project/opensearch-spark/blob/main/docs/index.md#all-indexes. Or if no time, we can just merge grammar in this PR.
Signed-off-by: Rupal Mahajan <[email protected]>
Signed-off-by: Rupal Mahajan <[email protected]>
Signed-off-by: Rupal Mahajan <[email protected]>
Thanks! Updated user manual. |
val partitionFields = table.partitioning().flatMap { transform => | ||
transform | ||
.references() | ||
.collect({ case reference => | ||
reference.fieldNames() | ||
}) | ||
.flatten | ||
.toSet | ||
} |
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'm not sure if this is the right API because I've only used table.schema()
. Could you double check this along with the comment above later? I will merge this PR for now so we can get the grammar into SQL plugin side.
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.
Sure will do. Thanks!
Description
Add
ANALYZE SKIPPING INDEX
statement. This returns recommendation for skipping index based on following rules.Issues Resolved
#221
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.