Allow rolling aggregations for window functions #8974

alandpost · 2017-09-14T20:26:53Z

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)

This PR also includes a commit that adds removeInput() to some SUM
aggregations, but at the cost of increasing the size of the aggregation
state. That may not be desired, and avoiding the bad effect there would
require refactoring into more window-function-specific code paths.

In the long term, we want to be able to use different algorithms with
different state structures for rolling window aggregations as opposed
to standard grouping. For example, a rolling min() needs to hold onto
all the values seen, whereas a non-rolling min() just needs to hold onto
the minimum value seen.

See #8982 for a list of the
aggregations and what changes they would need to support rolling.

maciejgrzybek

I like this patch. Added two comments though. Think of them as notes, not necessarily requests for changes.

Speaking of second commit, I think it's generally OK to proceed with this patch in current shape and schedule further refactoring to separate WF aggregates from non-WF aggregates to not pay the cost of rolling accumulators in non-rolling contexts because it'll be slightly bigger refactor.
Although, it should be weighted how much this can potentially affect network load in production systems. E.g. the extreme case of having sum(x) group by x with x being a column of all distinct values (cardinality = NDV). The overhead is an extra long (8B) per intermediate state, which in this degenerated case can end up being 1 long per row of an overhead.

maciejgrzybek · 2017-09-15T01:01:38Z

presto-main/src/main/java/com/facebook/presto/operator/aggregation/AccumulatorCompiler.java

@@ -130,7 +131,9 @@ public static GenericAccumulatorFactoryBinder generateAccumulatorFactoryBinder(A

        // Generate methods
        generateAddInput(definition, stateField, inputChannelsField, maskChannelField, metadata.getInputMetadata(), metadata.getInputFunction(), callSiteBinder, grouped);
-        generateAddInputWindowIndex(definition, stateField, metadata.getInputMetadata(), metadata.getInputFunction(), callSiteBinder);
+        generateAddOrRemoveInputWindowIndex(definition, stateField, metadata.getInputMetadata(), metadata.getInputFunction(), "addInput", callSiteBinder);
+        metadata.getRemoveInputFunction().ifPresent(


I think in case of not having remove input function, we still should generate a function matching the implemented interface but with, let's say the only statement in the body to be throw new IllegalStateException.
However, I didn't find anything to support the statement that your implementation may be illegal/may be prone to the undefined behavior, based on https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html and https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.invokeinterface
But I think a good rule of thumb is not to generate the code which cannot be generated straightforwardly by the compiler.
So, let's treat this comment as a note/warning, not necessarily an "error"/must change.

maciejgrzybek · 2017-09-15T01:05:31Z

...src/main/java/com/facebook/presto/operator/aggregation/AggregationFromAnnotationsParser.java

+                .collect(toImmutableList());
+
+        // Note that a missing removeInput function is not an error -- they are optional.
+        checkArgument(removeInputFunctions.size() <= 1, "Ambiguous aggregation removeInput functions");


I think it's rather checkState, not an checkArgument because it may happen because of the wrong content of the class not because someone passed wrong argument to the method, although I understand the transitivity of "wrong method comes from the wrong class which was passed as an argument"...

maciejgrzybek · 2017-09-15T02:01:10Z

@electrum, @martint this is the patch implementing the idea I was discussing with you when I was visiting Menlo campus for the meetup in May. Please have a look, it speeds up sliding windows significantly, reducing the time complexity from O(n^2) to O(n) in some cases.
We observed speed up of queries from minutes to seconds in some of our internal tests.

electrum · 2017-09-15T19:48:59Z

presto-main/src/test/java/com/facebook/presto/operator/TestHashAggregationOperator.java

@@ -93,8 +93,8 @@

    private static final InternalAggregationFunction LONG_AVERAGE = metadata.getFunctionRegistry().getAggregateFunctionImplementation(
            new Signature("avg", AGGREGATE, DOUBLE.getTypeSignature(), BIGINT.getTypeSignature()));
-    private static final InternalAggregationFunction LONG_SUM = metadata.getFunctionRegistry().getAggregateFunctionImplementation(
-            new Signature("sum", AGGREGATE, BIGINT.getTypeSignature(), BIGINT.getTypeSignature()));
+    private static final InternalAggregationFunction LONG_MIN = metadata.getFunctionRegistry().getAggregateFunctionImplementation(


Why change this? Does sum no longer work?

The second commit in the PR changes the aggregation state for LongSumAggregation to LongLongState. The test is validating the aggregation state, and thus needs some change. It was simplest for me to change the aggregation in the test to be one with the same aggregation state that LongSumAggregation used previously.

electrum · 2017-09-15T19:53:30Z

presto-main/src/main/java/com/facebook/presto/operator/aggregation/DoubleSumAggregation.java

-    @CombineFunction
-    public static void combine(@AggregationState NullableDoubleState state, @AggregationState NullableDoubleState otherState)
+    @RemoveInputFunction
+    public static void removeInput(@AggregationState LongDoubleState state, @SqlType(StandardTypes.DOUBLE) double value)


I don't see any tests for these. Could we add them to TestDoubleSumAggregation and TestLongSumAggregation? Or as window function queries?

I'm not at all sure that the second commit, which adds removeInput for some Sum aggregations, is a good idea. It increases the size of the aggregation state, which will hurt straightforward grouping queries that don't even use window functions. There are several other aggregations, however, that can support rolling without any additional costs: I attempted to triage the aggregations here: #8982

I think for testing, it makes sense to extend AbstractTestAggregationFunction to use removeInput() if it exists. I can look into doing this next week. Please keep in mind that I have only one week left at Teradata -- the 22nd will be my last day.

I added AbstractTestAggregationFunction.testSlidingWindow in the latest version of this PR.

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)

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.

alandpost · 2017-09-21T20:07:30Z

@electrum It would be great if you could take another look at this patch. I think it's good to go, with good unit test coverage of the new feature. Leaving out the changes to Sum (by dropping the last commit in the PR) would be reasonable in my opinion. Tomorrow is my last day at Teradata.

stale · 2019-04-03T14:21:21Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

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

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

facebook-github-bot added the CLA Signed label Sep 14, 2017

alandpost requested a review from maciejgrzybek September 14, 2017 20:27

alandpost force-pushed the aggregator-remove-input branch from 61ff46b to 1e4aec4 Compare September 14, 2017 20:36

maciejgrzybek reviewed Sep 15, 2017

View reviewed changes

maciejgrzybek requested a review from electrum September 15, 2017 01:57

alandpost changed the title ~~Allow rolling sum for window functions~~ Allow rolling aggregations for window functions Sep 15, 2017

alandpost mentioned this pull request Sep 15, 2017

Rolling aggregation support for specific aggregators #8982

Closed

electrum reviewed Sep 15, 2017

View reviewed changes

alandpost force-pushed the aggregator-remove-input branch from 1e4aec4 to a51529a Compare September 21, 2017 19:44

Add test for Accumulator.addInput from WindowIndex

9c7b7f7

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.

alandpost force-pushed the aggregator-remove-input branch 2 times, most recently from 2197cbc to 22b3b5f Compare September 21, 2017 19:57

alandpost added 2 commits September 21, 2017 16:02

Allow rolling sum for window functions

1809213

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.

alandpost force-pushed the aggregator-remove-input branch from 22b3b5f to 1809213 Compare September 21, 2017 20:03

highker mentioned this pull request Mar 8, 2019

[POC] Presto Stream Processing #12410

Closed

stale bot added the stale label Apr 3, 2019

stale bot closed this Apr 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow rolling aggregations for window functions #8974

Allow rolling aggregations for window functions #8974

alandpost commented Sep 14, 2017 •

edited

Loading

maciejgrzybek left a comment

maciejgrzybek Sep 15, 2017

maciejgrzybek Sep 15, 2017

maciejgrzybek commented Sep 15, 2017

electrum Sep 15, 2017

alandpost Sep 15, 2017

electrum Sep 15, 2017

alandpost Sep 15, 2017

alandpost Sep 21, 2017

alandpost commented Sep 21, 2017

stale bot commented Apr 3, 2019

Allow rolling aggregations for window functions #8974

Allow rolling aggregations for window functions #8974

Conversation

alandpost commented Sep 14, 2017 • edited Loading

maciejgrzybek left a comment

Choose a reason for hiding this comment

maciejgrzybek Sep 15, 2017

Choose a reason for hiding this comment

maciejgrzybek Sep 15, 2017

Choose a reason for hiding this comment

maciejgrzybek commented Sep 15, 2017

electrum Sep 15, 2017

Choose a reason for hiding this comment

alandpost Sep 15, 2017

Choose a reason for hiding this comment

electrum Sep 15, 2017

Choose a reason for hiding this comment

alandpost Sep 15, 2017

Choose a reason for hiding this comment

alandpost Sep 21, 2017

Choose a reason for hiding this comment

alandpost commented Sep 21, 2017

stale bot commented Apr 3, 2019

alandpost commented Sep 14, 2017 •

edited

Loading