Replacing SummingMergeTree implementation to use standard aggregation functions #1330

vavrusa · 2017-10-10T04:49:49Z

The issue with the current SummingMergeTree implementation is that merging is quite slow when the table contains nested structures ending with Map. The goal is to:

Reimplement SummingSortedBlockInputStream to use built-in aggregation functions (sum and sumMap)
Add specialized version for sumMap() for case with composite keys
Get rid of the custom SummingSortedBlockInputStream::mergeMap() implementation

I decided to split these into separate PRs, this first replaces numeric field summation and simple nested structure summation (single key, single value). When the nested structure contains a composite key, it falls back to existing implementation.

This is basically useful for testing SummingSortedBlockInputStream against the AggregatingBlockInputStream, which are used in the {Summing,Aggregating}MergeTree table engines respectively.

…ctions This replaces custom summation function implementations with an implementation using built-in aggregation functions (sum and sumMap). The goal is to be able to use specialized variants of aggregation functions, and to have a single efficient implementation.

vavrusa · 2017-10-10T04:55:23Z

Current implementation when just summing numeric column:

$ ./dbms/src/DataStreams/tests/aggregating_stream 500000 sum
Elapsed 0.02 sec., 26839873.32 rows/sec.

SummingSorted
One × 2

499996 abc abc
499997 def abcd
499998 abcd def
499999 defg abc

Current implementation when adding a nested column (1 key 1 value):

$ ./dbms/src/DataStreams/tests/aggregating_stream 500000 sum nested
Elapsed 1.93 sec., 259157.86 rows/sec.

SummingSorted
One × 2

499996 abc abc [0,1,2,3,4] [75001,74999,75003,74997,75000]
499997 def abcd [0,1,2,3,4] [75001,74999,75000,75003,74997]
499998 abcd def [0,1,2,3,4] [74998,75002,74997,75000,75003]
499999 defg abc [0,1,2,3,4] [75001,74999,75003,74997,75000]

The PR implementation when just summing numeric column:

Elapsed 0.01 sec., 35880875.49 rows/sec.

SummingSorted
One × 2

499996 abc abc
499997 def abcd
499998 abcd def
499999 defg abc

The PR implementation (1 key 1 value, 1 key 2 values):

$ ./dbms/src/DataStreams/tests/aggregating_stream 500000 sum nested
Elapsed 0.26 sec., 1935681.19 rows/sec.

SummingSorted
One × 2

499996 abc abc [0,1,2,3,4] [75001,74999,75003,74997,75000]
499997 def abcd [0,1,2,3,4] [75001,74999,75000,75003,74997]
499998 abcd def [0,1,2,3,4] [74998,75002,74997,75000,75003]
499999 defg abc [0,1,2,3,4] [75001,74999,75003,74997,75000]

$ ./dbms/src/DataStreams/tests/aggregating_stream 500000 sum nested multivalue
Elapsed 2.14 sec., 233978.77 rows/sec.

SummingSorted
 One × 2

499996 abc abc [0,1,2,3,4] [75001,74999,75003,74997,75000] [750010,749990,750030,749970,750000]
499997 def abcd [0,1,2,3,4] [75001,74999,75000,75003,74997] [750010,749990,750000,750030,749970]
499998 abcd def [0,1,2,3,4] [74998,75002,74997,75000,75003] [749980,750020,749970,750000,750030]
499999 defg abc [0,1,2,3,4] [75001,74999,75003,74997,75000] [750010,749990,750030,749970,750000]

The function is rewritten to avoid allocations on every insert with Field deserialising each array. The key type is now specialized, so it can be accessed directly. The value type is variant type, but only individual values are deserialised (which is cheap, since they're PODs). The function also support summing of multiple columns by the same key. The SummingSortedBlockInputStream uses the function in case of Nested structure with one numeric key and many values to sum.

vavrusa · 2017-10-12T04:27:01Z

Implemented more sumMap a bit more efficiently, and added support for single key and multiple arrays to sum. New implementation:

$ ./dbms/src/DataStreams/tests/aggregating_stream 500000 sum nested
Elapsed 0.08 sec., 6263231.08 rows/sec.

SummingSorted
 One × 2

499996 abc abc [0,1,2,3,4] [75001,74999,75003,74997,75000]
499997 def abcd [0,1,2,3,4] [75001,74999,75000,75003,74997]
499998 abcd def [0,1,2,3,4] [74998,75002,74997,75000,75003]
499999 defg abc [0,1,2,3,4] [75001,74999,75003,74997,75000]

$ ./dbms/src/DataStreams/tests/aggregating_stream 500000 sum nested multivalue
Elapsed 0.10 sec., 5039763.74 rows/sec.

SummingSorted
 One × 2

499996 abc abc [0,1,2,3,4] [75001,74999,75003,74997,75000] [750010,749990,750030,749970,750000]
499997 def abcd [0,1,2,3,4] [75001,74999,75000,75003,74997] [750010,749990,750000,750030,749970]
499998 abcd def [0,1,2,3,4] [74998,75002,74997,75000,75003] [749980,750020,749970,750000,750030]
499999 defg abc [0,1,2,3,4] [75001,74999,75003,74997,75000] [750010,749990,750030,749970,750000]

The implementation is a bit faster with HashMap instead of std::map, but it's not ordered so it doesn't maintain the property of keys being sorted in the result array, maybe that's not needed though @bocharov ?

Binary function variant could be further specialized as it doesn't need to store vector of values for each key, but I'm not sure if the performance gain is worth code duplication at this point.

vavrusa · 2017-10-12T21:00:17Z

I should proofread my comments, sorry 🙈

alexey-milovidov · 2017-10-12T21:13:04Z

dbms/src/DataStreams/SummingSortedBlockInputStream.cpp

+                desc.function->add(desc.state.data(), &col, cursor->pos, nullptr);
+                // This stream discards rows that are zero across all summed columns
+                if (!res)
+                    res = col->get64(cursor->pos) != 0;


It should discard rows, that was summed to zero (that becomes zero after summation).

Right, added a better comment. For unsigned types, it's true that the row is summed to zero if all input numbers are also zero. The slight difference is with signed numbers sequence, like -1 +1, such row will result in zero, but it won't be deleted until the next merge. I don't know how to get state cheaply out of an aggregation function (there's only interface for inserting to column), which is why I used input.

Ok.

(The only way to look at aggregation state is "insertResultInto" and it can be expensive.)

alexey-milovidov · 2017-10-12T21:15:29Z

dbms/src/DataStreams/SummingSortedBlockInputStream.h

@@ -69,15 +69,20 @@ class SummingSortedBlockInputStream : public MergingSortedBlockInputStream
     *   and can be deleted at any time.
     */

-    /// Stores numbers of key-columns and value-columns.
-    struct MapDescription
+    struct AggregateDescription


Missing comment.

alexey-milovidov · 2017-10-12T21:18:50Z

dbms/src/DataStreams/SummingSortedBlockInputStream.cpp

+        if (desc.created)
+        {
+            desc.function->insertResultInto(desc.state.data(), *desc.merged_column);
+            desc.function->destroy(desc.state.data());


The code is not exception safe: destroy is not called in exception, memory leak when the function with non-trivial state (sumMap) was used. Probably you should also add a destructor for AggregateDescription.

Good point! Added try/catch and an explicit state destructor.

alexey-milovidov · 2017-10-12T21:19:27Z

dbms/src/DataStreams/SummingSortedBlockInputStream.h

+        ColumnPtr merged_column;
+        std::vector<char> state;
+        bool created = false;
+        /* Compatibility with the mergeMap */


Why not separate struct?

Because I don't know whether the legacy mergeMap, or sumMap will be used when identifying maps in block, so instead of building two structures, I'm using just one. I'm happy to rework it to separate structs, it doesn't matter much.

alexey-milovidov

Almost Ok, only few modifications required.

…ption

ludv1x · 2017-10-17T13:49:31Z

@vavrusa It looks like that your changes broke the following tests:

00148_summing_merge_tree_nested_map_multiple_values:                   [ FAIL ] - return code 9
Received exception from server:
Code: 9. DB::Exception: Received from localhost:9000, ::1. DB::Exception: Sizes of columns doesn't match: k: 4, payload: 8. 

00146_summing_merge_tree_nested_map:                                   [ FAIL ] - return code 9
Received exception from server:
Code: 9. DB::Exception: Received from localhost:9000, ::1. DB::Exception: Sizes of columns doesn't match: d: 4, payload: 8. 

00084_summing_merge_tree:                                              [ FAIL ] - return code 9
Received exception from server:
Code: 9. DB::Exception: Received from localhost:9000, ::1. DB::Exception: Sizes of columns doesn't match: d: 2, x: 4. 

00043_summing_empty_part:                                              [ FAIL ] - return code 9
Received exception from server:
Code: 9. DB::Exception: Received from localhost:9000, ::1. DB::Exception: Sizes of columns doesn't match: d: 1, v: 8.

Could you check it, please?

vavrusa · 2017-10-17T17:52:19Z

I'll take a look!

vavrusa · 2017-10-17T23:04:01Z

@ludv1x I ran the mentioned test and fixes several issues here #1367 it'd be great to be able to run these via CI or something automatically, are you working on that?

ludv1x · 2017-10-18T19:35:28Z

@vavrusa Yes, we have CI and autotests for each PR, but they are run only for people from the whitelist.
So, you are not in the whitelist and your PRs are not tested.
I think we have to add you into it-)

I made special comment here to force tests running #1367 (comment)

vavrusa · 2017-10-18T19:45:51Z

@ludv1x Ah thanks. Can I see the test results when it finishes or is it internal?

ludv1x · 2017-10-18T20:15:15Z

@vavrusa You can see only final statistics, logs are not available publicly.

vavrusa added 2 commits October 9, 2017 21:35

DataStreams/tests: add SummingSortedBlockInputStream test variant

bd11a21

This is basically useful for testing SummingSortedBlockInputStream against the AggregatingBlockInputStream, which are used in the {Summing,Aggregating}MergeTree table engines respectively.

vavrusa added 2 commits October 11, 2017 21:17

DataStreams/test: test case for nested field with two arrays for summing

c8d5464

alexey-milovidov added 5 commits October 12, 2017 23:49

Update AggregateFunctionSumMap.h

35abe99

Update AggregateFunctionSumMap.h

b03b24f

Update AggregateFunctionSumMap.h

25efd60

Update AggregateFunctionSumMap.h

9f36168

Update AggregateFunctionSumMap.h

619d2df

alexey-milovidov added 3 commits October 13, 2017 00:03

Update AggregateFunctionSumMap.h

a5e953a

Update SummingSortedBlockInputStream.cpp

d714505

Update SummingSortedBlockInputStream.cpp

5ae7b1c

alexey-milovidov reviewed Oct 12, 2017

View reviewed changes

Update SummingSortedBlockInputStream.cpp

77039de

alexey-milovidov reviewed Oct 12, 2017

View reviewed changes

alexey-milovidov requested changes Oct 12, 2017

View reviewed changes

vavrusa added 2 commits October 12, 2017 14:55

SummingSortedBlockInputStream: added comments, destructor for state

18f52d4

SummingSortedBlockInputStream: use a separate structure for MapDescri…

c4b4a1f

…ption

alexey-milovidov merged commit 41b0bea into ClickHouse:master Oct 13, 2017

alexey-milovidov mentioned this pull request Mar 2, 2018

sumMaps doesn't aggregate with constant input #1975

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing SummingMergeTree implementation to use standard aggregation functions #1330

Replacing SummingMergeTree implementation to use standard aggregation functions #1330

vavrusa commented Oct 10, 2017

vavrusa commented Oct 10, 2017 •

edited

Loading

vavrusa commented Oct 12, 2017

vavrusa commented Oct 12, 2017 •

edited

Loading

alexey-milovidov Oct 12, 2017

vavrusa Oct 12, 2017

alexey-milovidov Oct 13, 2017

alexey-milovidov Oct 12, 2017

vavrusa Oct 12, 2017

alexey-milovidov Oct 12, 2017 •

edited

Loading

vavrusa Oct 12, 2017

alexey-milovidov Oct 12, 2017

vavrusa Oct 12, 2017

alexey-milovidov left a comment

ludv1x commented Oct 17, 2017

vavrusa commented Oct 17, 2017

vavrusa commented Oct 17, 2017

ludv1x commented Oct 18, 2017

vavrusa commented Oct 18, 2017

ludv1x commented Oct 18, 2017

Replacing SummingMergeTree implementation to use standard aggregation functions #1330

Replacing SummingMergeTree implementation to use standard aggregation functions #1330

Conversation

vavrusa commented Oct 10, 2017

vavrusa commented Oct 10, 2017 • edited Loading

vavrusa commented Oct 12, 2017

vavrusa commented Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-milovidov Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-milovidov left a comment

Choose a reason for hiding this comment

ludv1x commented Oct 17, 2017

vavrusa commented Oct 17, 2017

vavrusa commented Oct 17, 2017

ludv1x commented Oct 18, 2017

vavrusa commented Oct 18, 2017

ludv1x commented Oct 18, 2017

vavrusa commented Oct 10, 2017 •

edited

Loading

vavrusa commented Oct 12, 2017 •

edited

Loading

alexey-milovidov Oct 12, 2017 •

edited

Loading