Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use stable grouping set symbol orderings #18721

Merged
merged 2 commits into from
Sep 1, 2023
Merged

Use stable grouping set symbol orderings #18721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4892c9e
Use stable grouping set symbol orderings
pettyjamesm Aug 17, 2023
b4f278a
Simplify GroupIdNode#getInputSymbols()
pettyjamesm Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/trino-main/src/main/java/io/trino/sql/planner/LocalExecutionPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,6 @@
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import static com.google.common.base.Functions.forMap;
Expand Down Expand Up @@ -1887,7 +1886,7 @@ public PhysicalOperation visitGroupId(GroupIdNode node, LocalExecutionPlanContex

int outputChannel = 0;

for (Symbol output : node.getGroupingSets().stream().flatMap(Collection::stream).collect(Collectors.toSet())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be enough to say toImmutableSet here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this particular usage site it would be, but the same logic of producing distinct grouping set symbols is used in a few other places inside of GroupIdNode so I think it makes sense to just use it as a public method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it would be nice to separate "fix stability" from "reduce code duplication". maybe two commits?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately all of these changes are necessary to avoid the usage of unstable-ordering Set implementations, so breaking the commit into two would just introduce temporary changes that would immediately be deleted by the code de-duplication refactor- so I think it's simpler to leave it as a single commit.

for (Symbol output : node.getDistinctGroupingSetSymbols()) {
newLayout.put(output, outputChannel++);
outputTypes.add(source.getTypes().get(source.getLayout().get(node.getGroupingColumns().get(output))));
}
Expand Down
2 changes: 1 addition & 1 deletion core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,7 +1343,7 @@ private static List<Set<FieldId>> enumerateGroupingSets(GroupingSetAnalysis grou
List<Set<FieldId>> sets = IntStream.rangeClosed(0, rollup.size())
.mapToObj(prefixLength -> rollup.subList(0, prefixLength).stream()
.flatMap(Collection::stream)
.collect(Collectors.toSet()))
.collect(toImmutableSet()))
.collect(toImmutableList());

partialSets.add(sets);
Expand Down
39 changes: 22 additions & 17 deletions core/trino-main/src/main/java/io/trino/sql/planner/plan/GroupIdNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package io.trino.sql.planner.plan;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand All @@ -23,17 +24,15 @@
import com.google.errorprone.annotations.Immutable;
import io.trino.sql.planner.Symbol;

import java.util.Collection;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static io.trino.util.MoreLists.listOfListsCopy;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toSet;

@Immutable
public class GroupIdNode
Expand Down Expand Up @@ -76,10 +75,9 @@ public GroupIdNode(
@Override
public List<Symbol> getOutputSymbols()
{
return ImmutableList.<Symbol>builder()
.addAll(groupingSets.stream()
.flatMap(Collection::stream)
.collect(toSet()))
Set<Symbol> distinctGroupingSetSymbols = getDistinctGroupingSetSymbols();
return ImmutableList.<Symbol>builderWithExpectedSize(distinctGroupingSetSymbols.size() + aggregationArguments.size() + 1)
.addAll(distinctGroupingSetSymbols)
.addAll(aggregationArguments)
.add(groupIdSymbol)
.build();
Expand All @@ -103,6 +101,14 @@ public List<List<Symbol>> getGroupingSets()
return groupingSets;
}

@JsonIgnore
public Set<Symbol> getDistinctGroupingSetSymbols()
{
return groupingSets.stream()
.flatMap(List::stream)
.collect(toImmutableSet()); // Produce a stable ordering of grouping set symbols in the output
}

@JsonProperty
public Map<Symbol, Symbol> getGroupingColumns()
{
Expand All @@ -129,20 +135,19 @@ public <R, C> R accept(PlanVisitor<R, C> visitor, C context)

public Set<Symbol> getInputSymbols()
{
return ImmutableSet.<Symbol>builder()
.addAll(aggregationArguments)
.addAll(groupingSets.stream()
.map(set -> set.stream()
.map(groupingColumns::get).collect(Collectors.toList()))
.flatMap(Collection::stream)
.collect(toSet()))
pettyjamesm marked this conversation as resolved.
Show resolved Hide resolved
.build();
Set<Symbol> distinctGroupingSetSymbols = getDistinctGroupingSetSymbols();
ImmutableSet.Builder<Symbol> builder = ImmutableSet.builderWithExpectedSize(aggregationArguments.size() + distinctGroupingSetSymbols.size());
builder.addAll(aggregationArguments);
for (Symbol groupingSetSymbol : distinctGroupingSetSymbols) {
builder.add(groupingColumns.get(groupingSetSymbol));
}
return builder.build();
}

// returns the common grouping columns in terms of output symbols
public Set<Symbol> getCommonGroupingColumns()
{
Set<Symbol> intersection = new HashSet<>(groupingSets.get(0));
Set<Symbol> intersection = new LinkedHashSet<>(groupingSets.get(0));
for (int i = 1; i < groupingSets.size(); i++) {
intersection.retainAll(groupingSets.get(i));
}
Expand Down