Skip to content

Commit

Permalink
[fix](nereids) fix merge_percentile_to_array when has same agg functi…
Browse files Browse the repository at this point in the history
…on (#44783)

Related PR: #34313

Problem Summary
The original PR did not handle the following scenario:
```sql
SELECT SUM(a), PERCENTILE(pk, 0.1) AS c1, PERCENTILE(pk, 0.1) AS c2, PERCENTILE(pk, 0.4) AS c3 FROM test_merge_percentile;
```
In this case, the aggregate outputs include two identical functions
(PERCENTILE(pk, 0.1)). When constructing the LogicalProject, a map was
used where the key is the child of an Alias and the value is the Alias
itself. However, this approach loses information when two Aliases share
the same child.
This PR modifies the map structure to use the child of an Alias as the
key and a list of Alias objects as the value. This ensures that all
Alias instances with the same child are preserved, resolving the issue
of lost information in such cases.
  • Loading branch information
feiniaofeiafei authored Dec 2, 2024
1 parent ce9c717 commit 70b0a08
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,10 @@ private Plan doMerge(LogicalAggregate<Plan> aggregate) {
(List<Expression>) (List) newPercentileArrays);
ImmutableList.Builder<NamedExpression> newProjectOutputExpressions = ImmutableList.builder();
newProjectOutputExpressions.addAll((List<NamedExpression>) (List) notChangeForProject);
Map<Expression, Alias> existsAliasMap = Maps.newHashMap();
Map<Expression, List<Alias>> existsAliasMap = Maps.newHashMap();
// existsAliasMap is used to keep upper plan refer the same expr
for (Alias alias : existsAliases) {
existsAliasMap.put(alias.child(), alias);
existsAliasMap.computeIfAbsent(alias.child(), k -> new ArrayList<>()).add(alias);
}
Map<DistinctAndExpr, Slot> slotMap = Maps.newHashMap();
// slotMap is used to find the correspondence
Expand All @@ -169,20 +169,22 @@ private Plan doMerge(LogicalAggregate<Plan> aggregate) {
for (Map.Entry<DistinctAndExpr, List<AggregateFunction>> entry : funcMap.entrySet()) {
for (int i = 0; i < entry.getValue().size(); i++) {
AggregateFunction aggFunc = entry.getValue().get(i);
Alias originAlias = existsAliasMap.get(aggFunc);
DistinctAndExpr distinctAndExpr = new DistinctAndExpr(aggFunc.child(0), aggFunc.isDistinct());
Alias newAlias = new Alias(originAlias.getExprId(), new ElementAt(slotMap.get(distinctAndExpr),
new IntegerLiteral(i + 1)), originAlias.getName());
newProjectOutputExpressions.add(newAlias);
List<Alias> originAliases = existsAliasMap.get(aggFunc);
for (Alias originAlias : originAliases) {
DistinctAndExpr distinctAndExpr = new DistinctAndExpr(aggFunc.child(0), aggFunc.isDistinct());
Alias newAlias = new Alias(originAlias.getExprId(), new ElementAt(slotMap.get(distinctAndExpr),
new IntegerLiteral(i + 1)), originAlias.getName());
newProjectOutputExpressions.add(newAlias);
}
}
}
newProjectOutputExpressions.addAll(groupBySlots);
return new LogicalProject(newProjectOutputExpressions.build(), newAggregate);
return new LogicalProject<>(newProjectOutputExpressions.build(), newAggregate);
}

private static class DistinctAndExpr {
private Expression expression;
private boolean isDistinct;
private final Expression expression;
private final boolean isDistinct;

public DistinctAndExpr(Expression expression, boolean isDistinct) {
this.expression = expression;
Expand All @@ -193,10 +195,6 @@ public Expression getExpression() {
return expression;
}

public boolean isDistinct() {
return isDistinct;
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,15 @@
7.0 \N \N
7.0 7.0 7

-- !same_percentile --
52 1.0 1.0 2.0

-- !same_percentile_group_by --
\N 6.0 6.0 6.0
2 3.0 3.0 3.0
25 3.0 3.0 3.0
4 2.0 2.0 2.0
5 1.0 1.0 1.6
7 6.0 6.0 6.0
9 1.2 1.2 1.8

Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,8 @@ suite("merge_percentile_to_array") {
percentile(abs(a), 0.55) as c2 from test_merge_percentile group by a) t;
"""

order_qt_same_percentile """select sum(a),percentile(pk, 0.1) as c1 , percentile(pk, 0.1) as c2 ,
percentile(pk, 0.4) as c2 from test_merge_percentile;"""
order_qt_same_percentile_group_by """select sum(a),percentile(pk, 0.1) as c1 , percentile(pk, 0.1) as c2 ,
percentile(pk, 0.4) as c2 from test_merge_percentile group by a;"""
}

0 comments on commit 70b0a08

Please sign in to comment.