Skip to content

Add option to skip statree index evaluation during query#8737

Merged
KKcorps merged 4 commits intoapache:masterfrom
KKcorps:add_skip_startree_option
May 24, 2022
Merged

Add option to skip statree index evaluation during query#8737
KKcorps merged 4 commits intoapache:masterfrom
KKcorps:add_skip_startree_option

Conversation

@KKcorps
Copy link
Copy Markdown
Contributor

@KKcorps KKcorps commented May 19, 2022

Currently, there are some cases where Startree index might not work properly (such as using aggregation with OR filters). For such cases, we can have the option in query itself to skip startree index.

You can set option(useStarTree=false) to skip starTree index.

@KKcorps KKcorps requested review from Jackie-Jiang and removed request for Jackie-Jiang May 19, 2022 13:32
@KKcorps KKcorps added enhancement Improvement to existing functionality query Related to query processing labels May 19, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 20, 2022

Codecov Report

Merging #8737 (e26e3d7) into master (b4c4585) will increase coverage by 0.00%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##             master    #8737    +/-   ##
==========================================
  Coverage     69.67%   69.68%            
- Complexity     4612     4619     +7     
==========================================
  Files          1730     1732     +2     
  Lines         90312    90741   +429     
  Branches      13421    13508    +87     
==========================================
+ Hits          62928    63230   +302     
- Misses        23015    23108    +93     
- Partials       4369     4403    +34     
Flag Coverage Δ
integration1 26.98% <54.54%> (+0.12%) ⬆️
integration2 25.38% <54.54%> (+0.21%) ⬆️
unittests1 66.32% <45.45%> (+0.15%) ⬆️
unittests2 14.23% <0.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...t/core/plan/AggregationGroupByOrderByPlanNode.java 81.81% <0.00%> (ø)
...rg/apache/pinot/core/plan/AggregationPlanNode.java 89.71% <0.00%> (ø)
.../org/apache/pinot/core/startree/StarTreeUtils.java 78.35% <ø> (+9.66%) ⬆️
...va/org/apache/pinot/spi/utils/CommonConstants.java 21.31% <ø> (ø)
.../org/apache/pinot/core/util/QueryOptionsUtils.java 60.00% <25.00%> (-6.67%) ⬇️
...pinot/core/plan/maker/InstancePlanMakerImplV2.java 76.42% <100.00%> (+0.34%) ⬆️
...pinot/core/query/request/context/QueryContext.java 99.04% <100.00%> (+0.01%) ⬆️
...data/manager/realtime/DefaultSegmentCommitter.java 0.00% <0.00%> (-80.00%) ⬇️
...che/pinot/controller/util/TableMetadataReader.java 63.41% <0.00%> (-36.59%) ⬇️
...er/api/resources/LLCSegmentCompletionHandlers.java 43.56% <0.00%> (-18.82%) ⬇️
... and 63 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4c4585...e26e3d7. Read the comment docs.

Copy link
Copy Markdown
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some thoughts, I think it might be better to just keep using the current key (useStarTree) in the debug option, and allow it to be configured within the query option as well for the following benefits:

  • Easier migration: user doesn't need to remember to use different key in debug option vs query option
  • Currently we always use star-tree when applicable, but in the future we might automatically choose to use/not use star-tree based on the query, and user can use useStarTree=true to force using star-tree
  • We can consider deprecating the debug option and treat all debug option as query option for backward compatibility. Keeping the same key can make this much easier, and users who already use debug option do not need to make any change

@KKcorps
Copy link
Copy Markdown
Contributor Author

KKcorps commented May 23, 2022

Done. I have kept the methods name the existing one though i.e. isSkipStarTree for now since the current logic is that it needs to be disabled explicitly and is enabled by default. Once we reach the stage of where we start making a choice whether to use starTree or not, we can change the method to usStarTree

@KKcorps KKcorps force-pushed the add_skip_startree_option branch from f5b7946 to 3329ec2 Compare May 23, 2022 13:58
Copy link
Copy Markdown
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only minor non-blocking comments

@KKcorps KKcorps merged commit 15416e5 into apache:master May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to existing functionality query Related to query processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants