Allow rolling aggregations for window functions #464

kokosing · 2019-03-12T19:06:50Z

No description provided.

cla-bot · 2019-03-12T19:06:51Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. If you are contributing on behalf of someone else (e.g., your employer), the individual CLA may not be sufficient and your employer may need the Corporate CLA signed.

kokosing · 2019-03-12T19:09:17Z

It is WIP because I haven't run any tests.

I did simple stupid benchmark on a single node Presto server. Results are below, second run was executed on Presto without this patch:

presto:sf1> select count(a) from (SELECT sum(quantity) OVER (ROWS BETWEEN 2000 PRECEDING AND 2000 FOLLOWING) from lineitem) t(a);
  _col0
---------
 6001215
(1 row)

Query 20190312_185636_00020_9axmd, FINISHED, 1 node
Splits: 38 total, 38 done (100.00%)
0:02 [6M rows, 0B] [2.53M rows/s, 0B/s]

presto:sf1> select count(a) from (SELECT sum(quantity) OVER (ROWS BETWEEN 2000 PRECEDING AND 2000 FOLLOWING) from lineitem) t(a);
Query 20190312_185752_00000_ysa4v failed: No nodes available to run query

presto:sf1> select count(a) from (SELECT sum(quantity) OVER (ROWS BETWEEN 2000 PRECEDING AND 2000 FOLLOWING) from lineitem) t(a);
  _col0
---------
 6001215
(1 row)

Query 20190312_185756_00001_ysa4v, FINISHED, 1 node
Splits: 38 total, 38 done (100.00%)
5:03 [6M rows, 0B] [19.8K rows/s, 0B/s]

findepi · 2019-03-12T22:00:54Z

@kokosing

Extracted-From: https://github.com/Teradata/presto would be more appropriate
first commit has a typo: "Prever"

sopel39 · 2019-03-13T10:20:45Z

@kokosing ping me when you consider this ready for review

kokosing · 2019-04-11T06:42:35Z

Travis says:

2019-04-09T03:37:04.650-0500	INFO	pool-109-thread-2	io.prestosql.sql.gen.TestExpressionCompiler	FINISHED testBinaryOperatorsIntegerDecimal in 13.76s verified 0 expressions
Terminating due to java.lang.OutOfMemoryError: Java heap space
The

cla-bot · 2019-05-08T11:08:25Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

cla-bot · 2019-07-04T13:18:15Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

cla-bot · 2019-07-08T07:42:16Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

cla-bot · 2019-07-15T08:05:01Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

cla-bot · 2019-08-04T19:13:02Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

cla-bot · 2019-08-04T21:10:37Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

electrum · 2019-08-04T21:16:12Z

The "Minor fixes" commit looks good.

cla-bot · 2019-08-06T09:23:11Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

electrum · 2019-09-22T23:01:11Z

Add a test for rolling sum that includes nulls

electrum

Looks good to me. @kokosing I'm assuming you did a complete review as well.

electrum · 2019-09-22T21:34:41Z

...o-main/src/main/java/io/prestosql/operator/aggregation/AggregationFromAnnotationsParser.java

+    {
+        // Only include methods which take the same parameters as the corresponding input function
+        List<Method> removeInputFunctions = FunctionsParserHelper.findPublicStaticMethodsWithAnnotation(clazz, RemoveInputFunction.class).stream()
+                .filter(method ->


This would be simpler as two filter calls

electrum · 2019-09-22T21:36:22Z

...o-main/src/main/java/io/prestosql/operator/aggregation/AggregationFromAnnotationsParser.java

+                .filter(method ->
+                        Arrays.equals(method.getParameterTypes(), inputFunction.getParameterTypes())
+                                && Arrays.deepEquals(method.getParameterAnnotations(), inputFunction.getParameterAnnotations()))
+                .collect(toImmutableList());


The rest of this can be replaced with toOptional() from MoreCollectors

electrum · 2019-09-22T21:43:33Z

presto-main/src/main/java/io/prestosql/operator/aggregation/ParametricAggregation.java

@@ -86,6 +86,12 @@ public InternalAggregationFunction specialize(BoundVariables variables, int arit

        // Bind provided dependencies to aggregation method handlers
        MethodHandle inputHandle = bindDependencies(concreteImplementation.getInputFunction(), concreteImplementation.getInputDependencies(), variables, metadata);
+        Optional<MethodHandle> removeInputHandle = concreteImplementation.getRemoveInputFunction().map(


Let's format this like

Optional<MethodHandle> removeInputHandle = concreteImplementation.getRemoveInputFunction().map( removeInputFunction -> bindDependencies(removeInputFunction, concreteImplementation.getRemoveInputDependencies(), variables, metadata));

This visually aligns the bindDependencies call with the others, allowing us to easily see that it's similar.

electrum · 2019-09-22T22:53:25Z

presto-main/src/main/java/io/prestosql/operator/window/AggregateWindowFunction.java

+                currentStart = 0;
+                currentEnd = -1;
+            }
+            int overlapStart = Integer.max(frameStart, currentStart);


Use Math.max/min and static import

electrum · 2019-09-22T22:53:50Z

presto-main/src/main/java/io/prestosql/operator/window/AggregateWindowFunction.java

+                return;
+            }
+        }
+        // We couldn't or didn't want to modify the accumulation:  instead, discard the current accumulation and start fresh.


Formatting nits:

add newline before this

use single space after colon

electrum · 2019-09-22T22:56:17Z

presto-main/src/test/java/io/prestosql/operator/window/TestAggregateWindowFunction.java

@@ -57,6 +57,25 @@ public void testCountRowsOrdered()
                        .build());
    }

+    @Test
+    public void testCountRowsRolling()


Let's add a test for average as well, since it might be able to catch a problem that count would not.

I also added tests for PARTITION BY

Add a test for the WindowNode use case for Accumulators. Fix a few bugs in the aggregation tests uncovered by the additional coverage. The new tests didn't uncover any product bugs.

Add a removeInput() function to some Accumulators, and when it exists, use it in aggregate window functions to roll the aggregation forward incrementally. Dramatically speeds up queries such as: SELECT COUNT(quantity) OVER (ROWS BETWEEN 2000 PRECEDING AND 2000 FOLLOWING) Extracted from: prestodb/presto#8974

Implement removeInput() in some SUM aggregations, to speed up rolling window functions. This requires additional storage in the AggregationState for the input count, so that the aggregator knows when its result should become null. Extracted from: prestodb/presto#8974

cla-bot · 2019-09-30T11:11:33Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

kokosing

Comments addressed.

kokosing · 2019-09-30T11:08:39Z

presto-main/src/test/java/io/prestosql/operator/window/TestAggregateWindowFunction.java

@@ -57,6 +57,25 @@ public void testCountRowsOrdered()
                        .build());
    }

+    @Test
+    public void testCountRowsRolling()


I also added tests for PARTITION BY

kokosing · 2019-10-01T13:32:22Z

@alandpost Thank you! It took us a while to merge your work.

sopel39 · 2022-05-17T18:23:55Z

presto-main/src/main/java/io/prestosql/operator/aggregation/LongSumAggregation.java

@@ -31,27 +32,34 @@
    private LongSumAggregation() {}

    @InputFunction
-    public static void sum(@AggregationState NullableLongState state, @SqlType(StandardTypes.BIGINT) long value)
+    public static void sum(@AggregationState LongLongState state, @SqlType(StandardTypes.BIGINT) long value)


this actually causes significant perf regression as long long stats is much bigger to serialize, cc @martint @dain

I agree, and just filed an issue to showcase the problem #18818

jlpetz · 2023-09-18T00:11:52Z

Hi Trino,

This change appears to have resulted in a substantial performance regression for certain queries we have. For example something that was taking 9min is now taking 57min to run, a 6.3x increase. Per #18818

Is this something that could be looked into?

kokosing added WIP and removed WIP labels May 8, 2019

kokosing force-pushed the origin/master/108_port_8974_from_legacy_presto branch from 8bd5a0c to bea1565 Compare May 8, 2019 11:08

cla-bot bot removed the WIP label May 8, 2019

dain added the WIP label May 9, 2019

findepi mentioned this pull request May 27, 2019

Distributed sort for unpartitioned window functions #821

Closed

kokosing force-pushed the origin/master/108_port_8974_from_legacy_presto branch from bea1565 to cb58f28 Compare July 4, 2019 13:18

cla-bot bot removed the WIP label Jul 4, 2019

kokosing force-pushed the origin/master/108_port_8974_from_legacy_presto branch from c4f7fdb to 2defb0b Compare July 15, 2019 08:04

kokosing added the WIP label Aug 4, 2019

kokosing changed the title ~~[WIP] Allow rolling aggregations for window functions~~ Allow rolling aggregations for window functions Aug 4, 2019

kokosing force-pushed the origin/master/108_port_8974_from_legacy_presto branch from 2defb0b to a51c9af Compare August 4, 2019 19:13

cla-bot bot removed the WIP label Aug 4, 2019

kokosing force-pushed the origin/master/108_port_8974_from_legacy_presto branch from a51c9af to 600dfbc Compare August 4, 2019 21:10

kokosing mentioned this pull request Aug 5, 2019

Minor fixes around aggregation execution #1254

Merged

sopel39 self-requested a review August 6, 2019 08:56

kokosing force-pushed the origin/master/108_port_8974_from_legacy_presto branch from 600dfbc to a28b298 Compare August 6, 2019 09:23

electrum self-requested a review September 9, 2019 16:19

electrum approved these changes Sep 22, 2019

View reviewed changes

alandpost added 3 commits September 30, 2019 12:46

Add test for Accumulator.addInput from WindowIndex

d406ff2

Add a test for the WindowNode use case for Accumulators. Fix a few bugs in the aggregation tests uncovered by the additional coverage. The new tests didn't uncover any product bugs.

kokosing force-pushed the origin/master/108_port_8974_from_legacy_presto branch from a28b298 to e6b97d0 Compare September 30, 2019 11:11

kokosing commented Sep 30, 2019

View reviewed changes

kokosing merged commit e8cf714 into trinodb:master Oct 1, 2019

kokosing deleted the origin/master/108_port_8974_from_legacy_presto branch October 1, 2019 13:31

kokosing mentioned this pull request Oct 1, 2019

Release notes for 320 #1570

Closed

6 tasks

martint added this to the 320 milestone Oct 5, 2019

sopel39 reviewed May 17, 2022

View reviewed changes

weiatwork mentioned this pull request Aug 26, 2023

Suboptimal AggregationState in DoubleSumAggregation and LongSumAggregation #18818

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow rolling aggregations for window functions #464

Allow rolling aggregations for window functions #464

kokosing commented Mar 12, 2019

cla-bot bot commented Mar 12, 2019

kokosing commented Mar 12, 2019

findepi commented Mar 12, 2019

sopel39 commented Mar 13, 2019

kokosing commented Apr 11, 2019

cla-bot bot commented May 8, 2019

cla-bot bot commented Jul 4, 2019

cla-bot bot commented Jul 8, 2019

cla-bot bot commented Jul 15, 2019

cla-bot bot commented Aug 4, 2019

cla-bot bot commented Aug 4, 2019

electrum commented Aug 4, 2019

cla-bot bot commented Aug 6, 2019

electrum commented Sep 22, 2019

electrum left a comment

electrum Sep 22, 2019

electrum Sep 22, 2019

electrum Sep 22, 2019

electrum Sep 22, 2019

electrum Sep 22, 2019

electrum Sep 22, 2019

kokosing Sep 30, 2019

cla-bot bot commented Sep 30, 2019

kokosing left a comment

kokosing Sep 30, 2019

kokosing commented Oct 1, 2019

sopel39 May 17, 2022

weiatwork Aug 26, 2023

jlpetz commented Sep 18, 2023 •

edited

Loading

Allow rolling aggregations for window functions #464

Allow rolling aggregations for window functions #464

Conversation

kokosing commented Mar 12, 2019

cla-bot bot commented Mar 12, 2019

kokosing commented Mar 12, 2019

findepi commented Mar 12, 2019

sopel39 commented Mar 13, 2019

kokosing commented Apr 11, 2019

cla-bot bot commented May 8, 2019

cla-bot bot commented Jul 4, 2019

cla-bot bot commented Jul 8, 2019

cla-bot bot commented Jul 15, 2019

cla-bot bot commented Aug 4, 2019

cla-bot bot commented Aug 4, 2019

electrum commented Aug 4, 2019

cla-bot bot commented Aug 6, 2019

electrum commented Sep 22, 2019

electrum left a 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

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

cla-bot bot commented Sep 30, 2019

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing commented Oct 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlpetz commented Sep 18, 2023 • edited Loading

jlpetz commented Sep 18, 2023 •

edited

Loading