Introduce in-Segment Trim for GroupBy OrderBy Query #6991

wuwenw · 2021-05-27T01:07:39Z

Description

One of the major bottlenecks for the current GroupBy OrderBy query on high cardinality columns is the merge phase. Essentially every segment brings a large number of intermediate results to a global concurrent map for further aggregation and merge, which takes up a lot of space and is very time-consuming. This PR introduces an optimization option that each segment trims its intermediate results to a given size. The size is configurable by the user and is guaranteed to be max(limit N * 5, 5000). It won't affect accuracy much but reduces the running time for high cardinality dataset. ~5 times faster for String data with 10M cardinality. This option is turned off by default to ensure backward compatibility.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Optimized GroupBy OrderBy queries by introducing an in-segment trim option that can significantly reduce the size of intermediate results and speed up the execution.

Documentation

GroupBy OrderBy In-Segment Trim
This option is used to apply an additional filter in the segments for the GroupBy OrderBy query. Based on a configurable trim_size k, each segment will sort and send back the top k results only. It will slightly decrease the accuracy but boost up the performance for high-cardinality data.

Related query options:
SEGMENT TRIM ON
SEGMENT TRIM SIZE

Related server config:
enable.segment.group.trim
size.segment.group.trim

Related query keywords:
LIMIT N

SEGMENT TRIM ON and enable.segment.group.trim are used to enable/disable this feature. By default, it is turned off and segments will send back all results.

If it is enabled, then actual trim_size is calculated based on the parameters (SEGMENT TRIM SIZE and size.segment.group.trim) and query keyword LIMIT N.

Trim_size = max(5*LIMIT, minTrimSize) where minTrimSize is set up by the user (Default value: 5000)

…to master

# Conflicts: # pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionarySingleColumnGroupKeyGenerator.java

pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java

pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/IntermediateResultsBlock.java

pinot-core/src/main/java/org/apache/pinot/core/data/table/TableResizer.java

...re/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java

siddharthteotia

@wuwenw thanks for the PR. Yet to fully go through it. Please give a day to go through it.

codecov-commenter · 2021-05-27T18:27:32Z

Codecov Report

Merging #6991 (03beb3e) into master (0185482) will decrease coverage by 7.81%.
The diff coverage is 64.74%.

@@             Coverage Diff              @@
##             master    #6991      +/-   ##
============================================
- Coverage     73.23%   65.42%   -7.82%     
  Complexity       12       12              
============================================
  Files          1439     1454      +15     
  Lines         71333    72091     +758     
  Branches      10334    10441     +107     
============================================
- Hits          52243    47162    -5081     
- Misses        15578    21517    +5939     
+ Partials       3512     3412     -100

Flag	Coverage Δ
integration	`?`
unittests	`65.42% <64.74%> (+0.07%)`	⬆️

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

Impacted Files	Coverage Δ
...roker/requesthandler/BaseBrokerRequestHandler.java	`17.37% <0.00%> (-53.88%)`	⬇️
...e/pinot/common/minion/MergeRollupTaskMetadata.java	`0.00% <0.00%> (ø)`
...e/pinot/common/minion/MinionTaskMetadataUtils.java	`0.00% <0.00%> (-75.00%)`	⬇️
...mmon/restlet/resources/SegmentServerDebugInfo.java	`0.00% <0.00%> (ø)`
...che/pinot/controller/api/debug/TableDebugInfo.java	`0.00% <0.00%> (ø)`
...ces/PinotSegmentUploadDownloadRestletResource.java	`10.00% <0.00%> (-45.03%)`	⬇️
...t/controller/api/resources/TableDebugResource.java	`0.00% <0.00%> (ø)`
...troller/helix/core/minion/ClusterInfoAccessor.java	`32.14% <0.00%> (-47.86%)`	⬇️
...x/core/minion/generator/TaskGeneratorRegistry.java	`76.00% <ø> (-4.00%)`	⬇️
.../org/apache/pinot/core/common/MinionConstants.java	`0.00% <0.00%> (ø)`
... and 435 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 0185482...03beb3e. Read the comment docs.

pinot-core/src/main/java/org/apache/pinot/core/data/table/TableResizer.java

...re/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java

pinot-core/src/main/java/org/apache/pinot/core/data/table/TableResizer.java

...re/src/main/java/org/apache/pinot/core/operator/query/AggregationGroupByOrderByOperator.java

pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationGroupByOrderByPlanNode.java

pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java

into master

This reverts commit 116e74c.

…bator-pinot into master" This reverts commit 544e9a8, reversing changes made to 7d0b896.

pinot-core/src/main/java/org/apache/pinot/core/data/table/IntermediateRecord.java

pinot-core/src/main/java/org/apache/pinot/core/data/table/TableResizer.java

...n/java/org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGenerator.java

...re/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DefaultGroupByExecutor.java

pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderByMultiValueQueriesTest.java

...t-core/src/test/java/org/apache/pinot/queries/InterSegmentOrderBySingleValueQueriesTest.java

Jackie-Jiang

LGTM. Please revise the tests as discussed offline

...re/src/main/java/org/apache/pinot/core/operator/query/AggregationGroupByOrderByOperator.java

pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationGroupByOrderByPlanNode.java

pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java

Jackie-Jiang

LGTM otherwise

Jackie-Jiang · 2021-06-09T17:34:31Z

pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java

@@ -52,39 +53,53 @@
 import org.slf4j.LoggerFactory;


+


(nit) remove

Jackie-Jiang · 2021-06-09T17:34:46Z

...re/src/main/java/org/apache/pinot/core/operator/query/AggregationGroupByOrderByOperator.java

@@ -38,28 +44,34 @@
 @SuppressWarnings("rawtypes")
 public class AggregationGroupByOrderByOperator extends BaseOperator<IntermediateResultsBlock> {
  private static final String OPERATOR_NAME = "AggregationGroupByOrderByOperator";
+  private static final int TRIM_OFF = -1;


Seems not used?

Jackie-Jiang · 2021-06-09T17:35:37Z

.../src/test/java/org/apache/pinot/core/query/aggregation/groupby/GroupByInSegmentTrimTest.java

+ * Currently tests 'max' functions, and can be easily extended to
+ * test other conditions such as GroupBy without OrderBy
+ */
+public class GroupByInSegmentTrimTest {


This test could be flaky, fix as discussed offline

wuwenw added 16 commits April 22, 2021 23:35

change groupby sort to topk

38bd07d

Merge branch 'master' of https://github.com/apache/incubator-pinot in…

453555d

…to master

benchmarking

7daef2a

after benchmark

da5c6eb

put back benchmark

aed0827

format

e028d0b

format

3903882

format

148673a

in segment optimiz

18795be

Merge remote-tracking branch 'origin/master' into master

5808de5

# Conflicts: # pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionarySingleColumnGroupKeyGenerator.java

clean code

9fc8dcf

clean code

cc06797

clean code

ea7b63e

clean code

cc890fd

clean code

f32f172

add more test

36b304d

Jackie-Jiang reviewed May 27, 2021

View reviewed changes

siddharthteotia reviewed May 27, 2021

View reviewed changes

wuwenw added 2 commits May 27, 2021 22:15

add table_resizer test

a45e3cc

add more comments

3470a93

wuwenw requested a review from Jackie-Jiang May 28, 2021 02:55

Jackie-Jiang reviewed May 28, 2021

View reviewed changes

wuwenw added 7 commits June 2, 2021 01:10

change based on reviews

abb7261

change groupby sort to topk

a384765

benchmarking

361fd21

after benchmark

8b91dc9

put back benchmark

bbe146e

format

1d158a4

format

b900b4f

wuwenw added 11 commits June 2, 2021 11:52

clean code

9e359ea

add more test

9447889

add table_resizer test

388dfb8

add more comments

98eb6f7

change based on reviews

b19d247

rebased

7d0b896

Merge branch 'inSegmentOpt' of https://github.com/wuwenw/incubator-pinot

544e9a8

into master

rebased

116e74c

Revert "rebased"

bfb6065

This reverts commit 116e74c.

Revert "Merge branch 'inSegmentOpt' of https://github.com/wuwenw/incu…

1a9748a

…bator-pinot into master" This reverts commit 544e9a8, reversing changes made to 7d0b896.

change commit orders

d580a29

wuwenw changed the base branch from master to upgrade_jdk_11 June 2, 2021 17:50

wuwenw changed the base branch from upgrade_jdk_11 to master June 2, 2021 17:50

add license header

1a2eb6f

wuwenw requested a review from Jackie-Jiang June 3, 2021 00:49

change config logic

ead8b05

Jackie-Jiang reviewed Jun 3, 2021

View reviewed changes

change based on reviews

1eedfbe

wuwenw requested a review from Jackie-Jiang June 4, 2021 03:54

Jackie-Jiang approved these changes Jun 4, 2021

View reviewed changes

wuwenw added 3 commits June 7, 2021 15:06

change based on reviews

c516279

change based on reviews

c2b1628

add operator level test

818c0ea

wuwenw requested a review from Jackie-Jiang June 9, 2021 02:24

Jackie-Jiang approved these changes Jun 9, 2021

View reviewed changes

wuwenw added 4 commits June 9, 2021 14:53

minor change

a8ad544

minor change

dccc9cf

minor change

aa5d367

minor change

03beb3e

Jackie-Jiang merged commit 2a5d5b7 into apache:master Jun 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce in-Segment Trim for GroupBy OrderBy Query #6991

Introduce in-Segment Trim for GroupBy OrderBy Query #6991

wuwenw commented May 27, 2021 •

edited

Loading

siddharthteotia left a comment

codecov-commenter commented May 27, 2021 •

edited

Loading

Jackie-Jiang left a comment

Jackie-Jiang left a comment

Jackie-Jiang Jun 9, 2021

Jackie-Jiang Jun 9, 2021

Jackie-Jiang Jun 9, 2021

Introduce in-Segment Trim for GroupBy OrderBy Query #6991

Introduce in-Segment Trim for GroupBy OrderBy Query #6991

Conversation

wuwenw commented May 27, 2021 • edited Loading

Description

Upgrade Notes

Release Notes

Documentation

siddharthteotia left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 27, 2021 • edited Loading

Codecov Report

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang Jun 9, 2021

Choose a reason for hiding this comment

Jackie-Jiang Jun 9, 2021

Choose a reason for hiding this comment

Jackie-Jiang Jun 9, 2021

Choose a reason for hiding this comment

wuwenw commented May 27, 2021 •

edited

Loading

codecov-commenter commented May 27, 2021 •

edited

Loading