Use "union" in partial aggregation output #12155

lukasz-stec · 2022-04-27T13:41:32Z

Description

Since partial aggregation can be disabled at runtime,
both aggregated and the input data need to be passed
through to the final step.
This change extends the partial aggregation output
with additional columns to use for raw input
and uses those columns to send raw input in case
partial aggregation is disabled.
An additional column contains information which
set of channels should be used by the final step.

This is rebased on top of #12101. Only last commit matters here.

This is an extension of #11011 and limited implementation of prestodb/presto#10305.
It does not work on a per-group basis but rather per operator factory instance (i.e.all operators for a given plan node id on one worker node).
It does not use row type because there is no support currently to operate on the row type fields during planning.

Benchmarks

tpch/tpcds benchmarks on partitioned orc sf1000, with and without #11289 (that PR adds RLE support for partitioned exchange which makes this pr more efficient)
adaptive-partial-aggregation-union-rle-hasNonNullValue-var-row.pdf

Summary
There are significant gains for TPCH, especially with partitioned exchange RLE support. Duration drops even more than CPU because of a significant drop in the data sent over the network.
For TPCDS, the improvement is only visible when combined with partitioned exchange RLE support.

Is this change a fix, improvement, new feature, refactoring, or other?

improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

core query engine

How would you describe this change to a non-technical end user or system administrator?

Improve performance of group by queries with that produce many distinct values

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

lukasz-stec · 2022-05-06T06:55:43Z

I ran tpch/tpcds benchmarks for unpartitioned data + again for partitioned on the same code.
For unpartitioned the gains are smaller + rle support does not help that much.

orc sf1000 unpartitioned
adaptive-partial-aggregation-union-rle-unpart-060522.pdf

orc sf1000 partitioned
adaptive-partial-aggregation-union-rle-part-060522.pdf

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

...n/src/main/java/io/trino/operator/aggregation/partial/PartialAggregationOutputProcessor.java

lukasz-stec

comments addressed

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

...n/src/main/java/io/trino/operator/aggregation/partial/PartialAggregationOutputProcessor.java

skrzypo987

Some comments

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNodeBuilder.java

core/trino-main/src/main/java/io/trino/operator/aggregation/GroupedAggregator.java

...n/src/main/java/io/trino/operator/aggregation/partial/PartialAggregationOutputProcessor.java

core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java

lukasz-stec

comments addressed

core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNodeBuilder.java

core/trino-main/src/main/java/io/trino/operator/aggregation/GroupedAggregator.java

...n/src/main/java/io/trino/operator/aggregation/partial/PartialAggregationOutputProcessor.java

sopel39 · 2022-05-27T09:52:44Z

please rebase

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

core/trino-main/src/main/java/io/trino/operator/aggregation/GroupedAggregator.java

...n/src/main/java/io/trino/operator/aggregation/partial/PartialAggregationOutputProcessor.java

lukasz-stec

comments addressed + rebased on the master + some clean up in the PartialAggregationOutputProcessor

core/trino-main/src/main/java/io/trino/operator/aggregation/GroupedAggregator.java

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

dain · 2022-05-27T20:16:00Z

I am so glad someone finally had the time to address this!

sopel39

some comments

sopel39 · 2022-05-31T07:22:00Z

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java


        requireNonNull(groupingSets, "groupingSets is null");
        groupIdSymbol.ifPresent(symbol -> checkArgument(groupingSets.getGroupingKeys().contains(symbol), "Grouping columns does not contain groupId column"));
        this.groupingSets = groupingSets;

        this.groupIdSymbol = requireNonNull(groupIdSymbol);
-
+        this.rawInputMaskSymbol = requireNonNull(rawInputMaskSymbol, "rawInputMaskSymbol is null");
+        checkArgument(rawInputMaskSymbol.isPresent() || step == SINGLE);


rawInputMaskSymbol is also not needed when groupingSets.getGroupingKeys().isEmpty(). Is it added anyway? If so, it shouldn't.

refactored to not use rawInputMaskSymbol for global aggregations.

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

sopel39 · 2022-05-31T07:44:52Z

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

@@ -104,6 +112,23 @@ public AggregationNode(
        outputs.addAll(groupingSets.getGroupingKeys());
        hashSymbol.ifPresent(outputs::add);
        outputs.addAll(aggregations.keySet());
+        if (step.isOutputPartial() && !groupingSets.getGroupingKeys().isEmpty()) {
+            // add mask channels


There are more symbols that might be propagated. Add check states:

that Aggregation#distinct is false for step.isOutputPartial()
(see DistinctGroupedAccumulator/DistinctAccumulator)

that Aggregation#orderingScheme is empty for step.isOutputPartial()
(see OrderingGroupedAccumulator/OrderedAccumulator)

that Aggregation#filter is empty when step.isOutputPartial()

How is this related to this change?

How is this related to this change?

Because you only output aggregation input symbols and mask as part of PA. You don't output oderingScheme symbols or filter symbol. Hence you need checks that these are not needed

I guess we either need to skip the aggregations with oderingScheme and filter (with validations on top) or support it in the final step.

Look at the classes I pointed. They don't support intermediate aggregation

So basically those aggregation modes do not support partial aggregation and never did, right?
So the adaptation here doesn't matter as it would fail on the intermediate state anyway.

discussed offline

sopel39 · 2022-05-31T08:22:55Z

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

@@ -444,6 +475,13 @@ public Optional<Symbol> getMask()
            return mask;
        }

+        public Stream<Symbol> getInputs()


getInputs name is too generic. What generates value for lambda arguments?

renamed to getRawInputs.
Lambda are functions used in reduce_agg

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

...n/src/main/java/io/trino/operator/aggregation/partial/PartialAggregationOutputProcessor.java

sopel39 · 2022-05-31T09:22:31Z

core/trino-main/src/main/java/io/trino/operator/aggregation/partial/SkipAggregationBuilder.java

@@ -16,25 +16,15 @@
 import com.google.common.util.concurrent.ListenableFuture;


I think this is a wrong package. Other builders are in builder package. I don't think we need partial package

This keeps classes related with partial aggregation in one place. It's better this way than split it based on function

InMemoryHash is also used in partial aggregation. It also uses PartialAggregationOutputProcessor. This package layout seems artificial.

it is used there but it's not specific to the partial aggregation.
I still think that keeping these classes in a separate package, rather than in big io.trino.operator.aggregation is better for readability as there are all closely related.
This package is actually specific to adaptive PA so maybe the better name should be adaptive?

core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java

sopel39 · 2022-05-31T14:24:27Z

core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java

+                    channel++;
+                }
+                outputMappings.put(rawInputMaskSymbol.orElseThrow(), channel);
+                channel++;


Why don't you just take AggregationNode#getOutputSymbols and assign channel mappings to them? They should have correct layout already. No need to repeat logic from AggregationNode constructor and map channel for each symbol section individually

there is no AggregationNode available + why it was not done line that before?

I see some solutions:

extract static List<Symbol> AggregationNode#createOutputSymbols(...) to wrap symbol creation logic in utility function. It was simple previously, but notwit's complex enough to have dedicated method

Add List<Symbol> outputSymbols to createHashAggregationOperatorFactory

I refactored this to use the List<Symbol> outputSymbol. Seems better, good hint!

...src/main/java/io/trino/sql/planner/iterative/rule/PushPartialAggregationThroughExchange.java

sopel39 · 2022-05-31T14:39:07Z

testing/trino-tests/src/test/java/io/trino/tests/TestAdaptivePartialAggregation.java

@@ -31,4 +32,16 @@ protected QueryRunner createQueryRunner()
                        "task.max-partial-aggregation-memory", "0B"))
                .build();
    }
+
+    @Test
+    public void testAggregationWithLambda()


move to AbstractTestAggregations. Plus a comment what it tests in general

There is already io.trino.testing.AbstractTestEngineOnlyQueries#testReduceAgg that testes reduce_agg with PA. IMO this test makes sense only here as it specifically tests reduce_agg (lambda arg) with partial aggregation disabled by adaptation.

IMO this test makes sense only here as it specifically tests reduce_agg (lambda arg) with partial aggregation disabled by adaptation.

These tests are engine aggregation tests with different execution modes. They are not as targeted as UTs. They mean to test different, sometimes unexpected execution flows. Please move the test to testReduceAgg.

This test only makes sense with the adaptation kicking in (low adaptive-partial-aggregation.min-rows and task.max-partial-aggregation-memory), which is not that easy to setup in testReduceAgg since task.max-partial-aggregation-memory is config property only + it is unusual execution flow (lambda + PA adaptation) + it makes sesne to keep tests for adaptation in one place

core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/test/PlanBuilder.java

sopel39 · 2022-05-31T14:51:45Z

core/trino-main/src/test/java/io/trino/operator/TestHashAggregationOperator.java

                .build();

        assertOperatorEquals(operatorFactory, operator2Input, operator2Expected);
    }

+    @Test
+    public void testFinalAggregation()


testFinalAggregationWithoutRawInput?

this actually tests all cases, raw, aggregated and mixed

sopel39 · 2022-05-31T14:52:01Z

core/trino-main/src/test/java/io/trino/operator/TestHashAggregationOperator.java

+    }
+
+    @Test
+    public void testFinalAggregationWithMask()


testFinalAggregationWithRawInput?

this tests cases with raw, aggregated, and mixed input but also with mask block (from the distinct operator if I'm not mistaken)

core/trino-main/src/test/java/io/trino/operator/TestHashAggregationOperator.java

sopel39 · 2022-05-31T14:59:46Z

core/trino-main/src/test/java/io/trino/operator/TestHashAggregationOperator.java

+                        createLongsBlock(0, 0, 2, 2), // hash channels
+                        nullRle(BIGINT, 4), // accumulator state
+                        booleanRle(true, 4), // mask channel
+                        createLongsBlock(1, 2, -1, -2), // aggregation input


I don't see a separate block for aggregation input and intermediate state.
I would use values that are easy to validate (e.g. 10xx for intermediate state and xx for raw input)
Consider using aggregation that doesn't have same intermediate state type as raw input. That would be better test coverage

the blocks commented as accumulator state are the intermediate state. I changed the comment. Does this look better now?
I don't think type here matters for coverage. The sum is also easy to follow

It's better. Please change values though, so intermediate state and raw values have different order of magnitude, so they don't accidentally match in aggregation operator consumes wrong channel.

lukasz-stec

I think I either answered or addressed in the code all of the comments but gonna look through them again in a few hours to double-check (they were coming in parts)

core/trino-main/src/main/java/io/trino/operator/HashAggregationOperator.java

core/trino-main/src/main/java/io/trino/operator/aggregation/AggregatorFactory.java

lukasz-stec · 2022-05-31T14:40:08Z

core/trino-main/src/main/java/io/trino/operator/aggregation/AggregatorFactory.java

+            this.rawInputChannels = ImmutableList.copyOf(inputChannels);
+        }
+        else {
+            intermediateStateChannel = OptionalInt.of(inputChannels.get(0));


this is how it was working before my change ie. it was assumed that the first channel from inputChannels was intermediateStateChannel in case of final aggregation. I can move this logic a little bit further up, but to fix it completely AggregationNode.Aggregation would have to change.
See also io.trino.sql.planner.iterative.rule.PushPartialAggregationThroughExchange#split

lukasz-stec · 2022-05-31T14:50:09Z

core/trino-main/src/main/java/io/trino/operator/aggregation/AggregatorFactory.java

        this.maskChannel = requireNonNull(maskChannel, "maskChannel is null");
        this.spillable = spillable;
        this.lambdaProviders = ImmutableList.copyOf(requireNonNull(lambdaProviders, "lambdaProviders is null"));

-        checkArgument(step.isInputRaw() || inputChannels.size() == 1, "expected 1 input channel for intermediate aggregation");
+        checkArgument(step.isInputRaw() || intermediateStateChannel.isPresent(), "expected intermediateStateChannel for intermediate aggregation but got %s ", intermediateStateChannel);


This check is redundant since you set intermediateStateChannel on step.isInputRaw() few lines above.

I dropped the validation.

Would it be possible to validate rawInputChannels count?

I think not

lukasz-stec · 2022-05-31T14:55:33Z

core/trino-main/src/main/java/io/trino/operator/aggregation/AggregatorFactory.java

@@ -66,7 +78,8 @@ public Aggregator createAggregator()
        else {
            accumulator = accumulatorFactory.createIntermediateAccumulator(lambdaProviders);
        }
-        return new Aggregator(accumulator, step, intermediateType, finalType, inputChannels, maskChannel);
+        List<Integer> aggregatorInputChannels = intermediateStateChannel.isEmpty() ? rawInputChannels : ImmutableList.of(intermediateStateChannel.getAsInt());


Which params do you want to be explicit? Here i join two explicit fields to one list to keep the current Aggregator api intact.
Btw I renamed local var aggregatorInputChannels to inputChannels to show what is changing here and what is not.

core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/test/PlanBuilder.java

lukasz-stec · 2022-05-31T15:49:34Z

testing/trino-tests/src/test/java/io/trino/tests/TestAdaptivePartialAggregation.java

@@ -31,4 +32,16 @@ protected QueryRunner createQueryRunner()
                        "task.max-partial-aggregation-memory", "0B"))
                .build();
    }
+
+    @Test
+    public void testAggregationWithLambda()


There is already io.trino.testing.AbstractTestEngineOnlyQueries#testReduceAgg that testes reduce_agg with PA. IMO this test makes sense only here as it specifically tests reduce_agg (lambda arg) with partial aggregation disabled by adaptation.

testing/trino-tests/src/test/java/io/trino/tests/TestAdaptivePartialAggregation.java

...src/main/java/io/trino/sql/planner/iterative/rule/PushPartialAggregationThroughExchange.java

lukasz-stec · 2022-05-31T16:15:20Z

core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java

+                    channel++;
+                }
+                outputMappings.put(rawInputMaskSymbol.orElseThrow(), channel);
+                channel++;


there is no AggregationNode available + why it was not done line that before?

lukasz-stec

Just 2 comments for now

lukasz-stec · 2022-06-01T12:10:36Z

core/trino-main/src/main/java/io/trino/operator/aggregation/AggregatorFactory.java

+            this.rawInputChannels = ImmutableList.copyOf(inputChannels);
+        }
+        else {
+            intermediateStateChannel = OptionalInt.of(inputChannels.get(0));


Not really, what we talk about here is arguments to the aggregation function. The list of arguments is translated from symbols to channel indexes but still, the first argument is the intermediate state for final aggregation. so the index 0 in this list is the "magic" number. In order to avoid that we would have to have a separate field for the intermediate state symbol in the Aggregation class (the intermediateSymbol local var in the io.trino.sql.planner.iterative.rule.PushPartialAggregationThroughExchange#split). That may be a good idea anyway but it's not related to this change.

lukasz-stec · 2022-06-01T12:29:13Z

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

@@ -104,6 +112,23 @@ public AggregationNode(
        outputs.addAll(groupingSets.getGroupingKeys());
        hashSymbol.ifPresent(outputs::add);
        outputs.addAll(aggregations.keySet());
+        if (step.isOutputPartial() && !groupingSets.getGroupingKeys().isEmpty()) {
+            // add mask channels


I guess we either need to skip the aggregations with oderingScheme and filter (with validations on top) or support it in the final step.

lukasz-stec

I addressed most of the comments.
A big chunk of work was to add support for global aggregations. I'm not sure this is needed given the complexity this brings. I could disable the adaptation in the TestPushPartialAggregationThroughJoin instead.

I decided that for now, it is better not to support adaptation for CTAS (TableWriterNode, TableFinishNode) as it involves more changes in this already not small PR and it's not that critical but this may be a followup PR. I created #12697 for this.

core/trino-main/src/main/java/io/trino/operator/aggregation/AggregatorFactory.java

lukasz-stec · 2022-06-06T07:37:21Z

core/trino-main/src/main/java/io/trino/operator/aggregation/AggregatorFactory.java

+            this.rawInputChannels = ImmutableList.copyOf(inputChannels);
+        }
+        else {
+            intermediateStateChannel = OptionalInt.of(inputChannels.get(0));


I don't see a need for that. Intermediate symbol is represented by AggregationNode#aggregations.key

I don't think AggregationNode#aggregations.key is the Intermediate symbol for the final step (it is for partial)

...main/src/main/java/io/trino/operator/aggregation/builder/InMemoryHashAggregationBuilder.java

lukasz-stec · 2022-06-06T09:24:34Z

core/trino-main/src/main/java/io/trino/operator/aggregation/partial/SkipAggregationBuilder.java

@@ -16,25 +16,15 @@
 import com.google.common.util.concurrent.ListenableFuture;


it is used there but it's not specific to the partial aggregation.
I still think that keeping these classes in a separate package, rather than in big io.trino.operator.aggregation is better for readability as there are all closely related.
This package is actually specific to adaptive PA so maybe the better name should be adaptive?

lukasz-stec · 2022-06-06T09:41:39Z

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

@@ -104,6 +112,23 @@ public AggregationNode(
        outputs.addAll(groupingSets.getGroupingKeys());
        hashSymbol.ifPresent(outputs::add);
        outputs.addAll(aggregations.keySet());
+        if (step.isOutputPartial() && !groupingSets.getGroupingKeys().isEmpty()) {
+            // add mask channels


So basically those aggregation modes do not support partial aggregation and never did, right?
So the adaptation here doesn't matter as it would fail on the intermediate state anyway.

lukasz-stec · 2022-06-06T10:01:57Z

testing/trino-tests/src/test/java/io/trino/tests/TestAdaptivePartialAggregation.java

@@ -31,4 +32,16 @@ protected QueryRunner createQueryRunner()
                        "task.max-partial-aggregation-memory", "0B"))
                .build();
    }
+
+    @Test
+    public void testAggregationWithLambda()


This test only makes sense with the adaptation kicking in (low adaptive-partial-aggregation.min-rows and task.max-partial-aggregation-memory), which is not that easy to setup in testReduceAgg since task.max-partial-aggregation-memory is config property only + it is unusual execution flow (lambda + PA adaptation) + it makes sesne to keep tests for adaptation in one place

lukasz-stec · 2022-06-07T15:40:43Z

core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java

+                    channel++;
+                }
+                outputMappings.put(rawInputMaskSymbol.orElseThrow(), channel);
+                channel++;


I refactored this to use the List<Symbol> outputSymbol. Seems better, good hint!

lukasz-stec · 2022-08-03T09:39:30Z

rebased on the master

sopel39 · 2022-08-04T08:45:55Z

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

@@ -104,6 +112,23 @@ public AggregationNode(
        outputs.addAll(groupingSets.getGroupingKeys());
        hashSymbol.ifPresent(outputs::add);
        outputs.addAll(aggregations.keySet());
+        if (step.isOutputPartial() && !groupingSets.getGroupingKeys().isEmpty()) {
+            // add mask channels


discussed offline

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

lukasz-stec

latest comments answered

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

sopel39 · 2022-08-05T10:26:28Z

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

+                    .flatMap(Optional::stream)
+                    .distinct()
+                    .forEach(outputsBuilder::add);
+            // add raw inputs to the aggregations to be used by adaptive partial aggregation.


Do we run tests with adaptive PA on/off and intermediate step?

sopel39 · 2022-08-05T10:29:15Z

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java

-        outputs.addAll(groupingSets.getGroupingKeys());
-        hashSymbol.ifPresent(outputs::add);
-        outputs.addAll(aggregations.keySet());
+        ImmutableList.Builder<Symbol> outputsBuilder = ImmutableList.builder();


Could you benchmark this and Push partial aggregation though join?

lukasz-stec · 2022-08-09T09:29:29Z

I ran tpch/tpcds benchmark on the current version (recently rebased on the master) + with push_partial_aggregation_through_join=true.
The results are below. The improvement from this changes alone are smaller than before (this may be because of the optimized PartitionedOutputOperator). With the rule on, tpch improvement is still at 7%. There is no improvement for tpcds.

Benchmarks_paa_union_push_pa_through_join.pdf
cc @sopel39

lukasz-stec · 2022-08-19T08:46:50Z

I ran tpch/tpcds sf1k orc part with this (paa union) on top of #13573 (decimnal serde optimization) and push_partial_aggregation_through_join=true
It seems that this PR is still worth doing but it depends on push_partial_aggregation_through_join being enabled.

sf1k-orc-part-dec-serde-paa-union-push-pa-t-join.pdf

sopel39 · 2022-08-19T10:09:55Z

It seems that this PR is still worth doing but it depends on push_partial_aggregation_through_join being enabled.

What about dec serde opt + push_partial_aggregation_through_join?

lukasz-stec · 2022-08-19T10:38:55Z

What about dec serde opt + push_partial_aggregation_through_join?

see row 1 in the summary

Since partial aggregation can be disabled at runtime, both aggregated and the input data need to be passed through to the final step. This change extends the partial aggregation output with additional columns to use for raw input and uses those columns to send raw input in case partial aggregation is disabled. An additional column contains information which set of channels should be used by the final step.

lukasz-stec · 2022-09-02T13:26:46Z

I ran another orc part sf1k for this rebased on a master with #13573.
There are noticeable tpch gains (6% CPU).

One thing that is surprising is that the baseline does have the same improvement seen in #13573 and it should as it contains this change.
This may be due to benchmark variability (in this ran or the others).

sf1k-orc-part-dec-serde-paa-union-over-dec-serde-master.pdf

colebow · 2023-03-30T18:13:28Z

👋 @lukasz-stec - this PR has become inactive. If you're still interested in working on it, please let us know.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

lukasz-stec · 2023-03-31T14:09:22Z

closing as there is no interest in merging this

cla-bot bot added the cla-signed label Apr 27, 2022

lukasz-stec marked this pull request as ready for review April 29, 2022 07:18

lukasz-stec requested review from sopel39, dain and skrzypo987 April 29, 2022 07:18

sopel39 reviewed May 24, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

sopel39 reviewed May 24, 2022

View reviewed changes

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch from c5b8233 to e9952f1 Compare May 24, 2022 20:23

lukasz-stec commented May 24, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

...n/src/main/java/io/trino/operator/aggregation/partial/PartialAggregationOutputProcessor.java Outdated Show resolved Hide resolved

lukasz-stec requested a review from sopel39 May 24, 2022 20:24

skrzypo987 reviewed May 25, 2022

View reviewed changes

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch from e9952f1 to 38276d9 Compare May 25, 2022 12:46

lukasz-stec commented May 25, 2022

View reviewed changes

lukasz-stec requested a review from skrzypo987 May 25, 2022 12:46

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch from 38276d9 to e92d670 Compare May 26, 2022 08:25

sopel39 reviewed May 27, 2022

View reviewed changes

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch from e92d670 to 27b85fd Compare May 27, 2022 14:10

sopel39 reviewed May 27, 2022

View reviewed changes

...n/src/main/java/io/trino/operator/aggregation/partial/PartialAggregationOutputProcessor.java Outdated Show resolved Hide resolved

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch from 27b85fd to dc431b7 Compare May 27, 2022 14:28

lukasz-stec commented May 27, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/operator/aggregation/GroupedAggregator.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Outdated Show resolved Hide resolved

lukasz-stec requested a review from sopel39 May 27, 2022 14:32

sopel39 reviewed May 31, 2022

View reviewed changes

lukasz-stec commented May 31, 2022

View reviewed changes

lukasz-stec requested a review from sopel39 May 31, 2022 16:20

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch from dc431b7 to f92ea04 Compare May 31, 2022 18:41

lukasz-stec commented Jun 1, 2022

View reviewed changes

lukasz-stec mentioned this pull request Jun 6, 2022

Support adaptive partial aggregation for CTAS aggregations #12697

Open

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch 5 times, most recently from 126523f to 3c42707 Compare June 7, 2022 15:41

lukasz-stec commented Jun 7, 2022

View reviewed changes

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch from 3c42707 to eed7bba Compare June 7, 2022 17:34

lukasz-stec requested a review from sopel39 June 7, 2022 21:03

sopel39 requested a review from raunaqmorarka August 1, 2022 10:24

lukasz-stec added the performance label Aug 2, 2022

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch from eed7bba to 5f5bf3f Compare August 3, 2022 09:38

sopel39 reviewed Aug 4, 2022

View reviewed changes

lukasz-stec commented Aug 4, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/plan/AggregationNode.java Show resolved Hide resolved

lukasz-stec requested a review from sopel39 August 4, 2022 13:37

sopel39 reviewed Aug 5, 2022

View reviewed changes

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch from 5f5bf3f to 0177bd4 Compare August 12, 2022 08:05

lukasz-stec force-pushed the ls/007-pa-union-additional-column branch from 0177bd4 to 90f5848 Compare September 1, 2022 17:41

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

lukasz-stec closed this Mar 31, 2023

		@@ -16,25 +16,15 @@
		import com.google.common.util.concurrent.ListenableFuture;

Use "union" in partial aggregation output #12155

Use "union" in partial aggregation output #12155

Conversation

lukasz-stec commented Apr 27, 2022 • edited Loading

Description

Benchmarks

Related issues, pull requests, and links

Documentation

Release notes

lukasz-stec commented May 6, 2022

lukasz-stec left a comment

Choose a reason for hiding this comment

skrzypo987 left a comment

Choose a reason for hiding this comment

lukasz-stec left a comment

Choose a reason for hiding this comment

sopel39 commented May 27, 2022

lukasz-stec left a comment

Choose a reason for hiding this comment

dain commented May 27, 2022

sopel39 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 Jun 1, 2022 • 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

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

sopel39 May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-stec 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

lukasz-stec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-stec 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

lukasz-stec commented Aug 3, 2022

Choose a reason for hiding this comment

lukasz-stec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-stec commented Aug 9, 2022

lukasz-stec commented Aug 19, 2022

lukasz-stec commented Apr 27, 2022 •

edited

Loading

sopel39 Jun 1, 2022 •

edited

Loading

sopel39 May 31, 2022 •

edited

Loading