Aggregation-in-order from query plan by KochetovNicolai · Pull Request #43592 · ClickHouse/ClickHouse

KochetovNicolai · 2022-11-23T18:43:21Z

Changelog category (leave one):

Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Implement aggregation-in-order optimization on top of query plan. It is enabled by default (but works only together with optimize_aggregation_in_order, which is disabled by default). Set query_plan_aggregation_in_order = 0 to use previous AST-based version.

nickitat · 2022-12-02T22:47:42Z

maybe relevant slowdowns:
https://s3.amazonaws.com/clickhouse-test-reports/43592/5edc9953b0b303a8be0a82b9df54c09abf77bb07/performance_comparison_[1/4]/report.html
https://s3.amazonaws.com/clickhouse-test-reports/43592/5edc9953b0b303a8be0a82b9df54c09abf77bb07/performance_comparison_[3/4]/report.html

nickitat · 2022-12-02T20:35:06Z

src/Processors/QueryPlan/AggregatingStep.h

    bool group_by_use_nulls;

-    InputOrderInfoPtr group_by_info;
+    SortDescription sort_description_for_merging;


we need a comment explaining the purpose of these two. maybe just a link to comment under optimisation

nickitat · 2022-12-02T22:00:47Z

src/Processors/QueryPlan/Optimizations/enableMemoryBoundMerging.cpp

+        if (enforce_aggregation_in_order)
+            reading->enforceAggregationInOrder();
+    }
+


would be good also to call updateInputStream on parent steps to propagate sorting

Good idea.
Don't want to do it is this pr.

nickitat · 2022-12-02T22:02:55Z

src/Processors/QueryPlan/Optimizations/optimizeReadInOrder.cpp

+/// We would like to have:
+/// (a, b, c) - a sort description for reading from table (it's into input_order)
+/// (a, c) - a sort description for merging (an input of AggregatingInOrderTransfrom is sorted by this GROUP BY keys)
+/// (a, c, d) - a group by soer description (an input of FinishAggregatingInOrderTransform is sorted by all GROUP BY keys)


Suggested change

/// (a, c, d) - a group by soer description (an input of FinishAggregatingInOrderTransform is sorted by all GROUP BY keys)

/// (a, c, d) - a group by sort description (an input of FinishAggregatingInOrderTransform is sorted by all GROUP BY keys)

nickitat · 2022-12-02T22:12:45Z

src/Processors/QueryPlan/Optimizations/optimizeReadInOrder.cpp

+    MatchedTrees::Matches matches;
+    FixedColumns fixed_key_columns;
+
+    /// For every column in PK find any macth from GROUP BY key.


Suggested change

/// For every column in PK find any macth from GROUP BY key.

/// For every column in PK find any match from GROUP BY key.

nickitat · 2022-12-02T22:13:20Z

src/Processors/QueryPlan/Optimizations/optimizeReadInOrder.cpp

+            //std::cerr << "------- matching " << static_cast<const void *>(node) << " " << node->result_name
+            //    << " to " << static_cast<const void *>(match.node) << " " << (match.node ? match.node->result_name : "") << std::endl;


Suggested change

//std::cerr << "------- matching " << static_cast<const void *>(node) << " " << node->result_name

// << " to " << static_cast<const void *>(match.node) << " " << (match.node ? match.node->result_name : "") << std::endl;

nickitat · 2022-12-02T22:40:44Z

src/Processors/QueryPlan/ReadFromRemote.cpp

+            ErrorCodes::LOGICAL_ERROR,
+            "Cannot enforce aggregation in order for ReadFromRemote step up to stage {}",
+            QueryProcessingStage::toString(stage));
+


Suggested change

context.setSetting("enable_memory_bound_merging_of_aggregation_results", true);

I would rather separate enforceAggregationInOrder and enforceSorting calls.

KochetovNicolai · 2022-12-06T12:35:22Z

Checked query from perftest and don't see any difference:

--- New
localhost:9000, queries 114, QPS: 0.363, RPS: 36284788.405, MiB/s: 276.831, result RPS: 0.000, result MiB/s: 0.000.

0.000%		2.556 sec.	
10.000%		2.629 sec.	
20.000%		2.673 sec.	
30.000%		2.692 sec.	
40.000%		2.721 sec.	
50.000%		2.745 sec.	
60.000%		2.765 sec.	
70.000%		2.783 sec.	
80.000%		2.811 sec.	
90.000%		2.889 sec.	
95.000%		2.956 sec.	
99.000%		3.108 sec.	
99.900%		3.125 sec.	
99.990%		3.125 sec.	

--- Old
localhost:9000, queries 242, QPS: 0.367, RPS: 36722549.794, MiB/s: 280.171, result RPS: 0.000, result MiB/s: 0.000.

0.000%		2.530 sec.	
10.000%		2.614 sec.	
20.000%		2.648 sec.	
30.000%		2.668 sec.	
40.000%		2.688 sec.	
50.000%		2.710 sec.	
60.000%		2.735 sec.	
70.000%		2.751 sec.	
80.000%		2.778 sec.	
90.000%		2.818 sec.	
95.000%		2.883 sec.	
99.000%		3.128 sec.	
99.900%		3.203 sec.	
99.990%		3.203 sec.

KochetovNicolai · 2022-12-06T16:12:27Z

Merged cause only comments were changed since last checks.

KochetovNicolai added 2 commits November 21, 2022 18:58

Aggregating in order [in progress].

aeb1de5

Aggregating in order [in progress].

62debf4

robot-ch-test-poll added the pr-improvement Pull request with some product improvements label Nov 23, 2022

KochetovNicolai and others added 2 commits November 28, 2022 15:15

Add order info for aggregating step in plan. Added test.

1dfa188

Merge branch 'master' into aggregating-in-order-from-query-plan

6d0646e

KochetovNicolai marked this pull request as ready for review November 28, 2022 15:54

Fixing style.

500d548

nickitat self-assigned this Nov 28, 2022

KochetovNicolai added 3 commits November 30, 2022 14:31

Add a separate optimisation to enable memory bound aggregation.

43b3c10

Fix style.

7db145b

Fixing tests.

5edc995

nickitat reviewed Dec 2, 2022

View reviewed changes

Review fixes.

9de5338

Merge branch 'master' into aggregating-in-order-from-query-plan

0ed82f3

KochetovNicolai merged commit 52c1355 into master Dec 6, 2022

KochetovNicolai deleted the aggregating-in-order-from-query-plan branch December 6, 2022 16:11

novikd mentioned this pull request Apr 6, 2023

[Umbrella] Analyzer, Planner migration #42648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregation-in-order from query plan#43592

Aggregation-in-order from query plan#43592
KochetovNicolai merged 10 commits intomasterfrom
aggregating-in-order-from-query-plan

KochetovNicolai commented Nov 23, 2022

Uh oh!

nickitat commented Dec 2, 2022

Uh oh!

nickitat Dec 2, 2022

Uh oh!

nickitat Dec 2, 2022

Uh oh!

KochetovNicolai Dec 6, 2022

Uh oh!

nickitat Dec 2, 2022

Uh oh!

nickitat Dec 2, 2022

Uh oh!

nickitat Dec 2, 2022

Uh oh!

nickitat Dec 2, 2022

Uh oh!

KochetovNicolai Dec 6, 2022

Uh oh!

KochetovNicolai commented Dec 6, 2022

Uh oh!

KochetovNicolai commented Dec 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	/// (a, c, d) - a group by soer description (an input of FinishAggregatingInOrderTransform is sorted by all GROUP BY keys)
	/// (a, c, d) - a group by sort description (an input of FinishAggregatingInOrderTransform is sorted by all GROUP BY keys)

	/// For every column in PK find any macth from GROUP BY key.
	/// For every column in PK find any match from GROUP BY key.

		//std::cerr << "------- matching " << static_cast<const void *>(node) << " " << node->result_name
		// << " to " << static_cast<const void *>(match.node) << " " << (match.node ? match.node->result_name : "") << std::endl;



	context.setSetting("enable_memory_bound_merging_of_aggregation_results", true);

Conversation

KochetovNicolai commented Nov 23, 2022

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Uh oh!

nickitat commented Dec 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KochetovNicolai commented Dec 6, 2022

Uh oh!

KochetovNicolai commented Dec 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants