Skip to content

Commit

Permalink
[BugFix] skip add the alias into fieldMapping when order by expr not …
Browse files Browse the repository at this point in the history 
…referenced this alias (#50546)

Signed-off-by: packy92 <[email protected]>
(cherry picked from commit 885c500)

# Conflicts:
#	fe/fe-core/src/test/java/com/starrocks/sql/plan/OrderByTest.java
  • Loading branch information
@packy92 @mergify
packy92 authored and mergify[bot] committed Sep 2, 2024
1 parent fe99d55 commit 7b28773
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 13 deletions.
42 changes: 33 additions & 9 deletions fe/fe-core/src/main/java/com/starrocks/sql/optimizer/transformer/QueryTransformer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import com.starrocks.analysis.LimitElement;
import com.starrocks.analysis.OrderByElement;
import com.starrocks.analysis.SlotRef;
import com.starrocks.analysis.TableName;
import com.starrocks.catalog.Type;
import com.starrocks.common.Pair;
import com.starrocks.common.TreeNode;
Expand Down Expand Up @@ -89,21 +90,21 @@ public LogicalPlan plan(SelectRelation queryBlock, ExpressionMapping outer) {
Iterables.concat(queryBlock.getOutputExpr(),
queryBlock.getOrderSourceExpressions(),
queryBlock.getOrderByAnalytic()),
queryBlock.getOutputExprInOrderByScope(),
queryBlock,
outputNames,
builder.getFieldMappings(),
queryBlock.getOrderScope(), true);
true);

} else {
// requires both output and source fields to be visible if there is no aggregation or distinct
// if there is a distinct, we still no need to project the orderSourceExpressions because it must
// in the outputExpression.
builder = projectForOrder(builder,
Iterables.concat(queryBlock.getOutputExpression(), queryBlock.getOrderByAnalytic()),
queryBlock.getOutputExprInOrderByScope(),
queryBlock,
queryBlock.getColumnOutputNames(),
builder.getFieldMappings(),
queryBlock.getOrderScope(), queryBlock.isDistinct());
queryBlock.isDistinct());
}
}

Expand Down Expand Up @@ -146,10 +147,12 @@ private OptExprBuilder planFrom(Relation node, CTETransformerContext cteContext)

private OptExprBuilder projectForOrder(OptExprBuilder subOpt,
Iterable<Expr> outputExpression,
List<Integer> outputExprInOrderByScope,
SelectRelation queryBlock,
List<String> outputNames,
List<ColumnRefOperator> sourceExpression, Scope scope,
List<ColumnRefOperator> sourceExpression,
boolean withAggregation) {
List<Integer> outputExprInOrderByScope = queryBlock.getOutputExprInOrderByScope();
Scope scope = queryBlock.getOrderScope();
ExpressionMapping outputTranslations = new ExpressionMapping(scope);
Map<ColumnRefOperator, ScalarOperator> projections = Maps.newHashMap();

Expand All @@ -167,10 +170,31 @@ private OptExprBuilder projectForOrder(OptExprBuilder subOpt,
projections.put(columnRefOperator, scalarOperator);

if (outputExprInOrderByScope.contains(outputExprIdx)) {
outputTranslations.putWithSymbol(expression,
new SlotRef(null, outputNames.get(outputExprIdx)), columnRefOperator);
outputTranslations.put(expression, columnRefOperator);
TableName resolveTableName = queryBlock.getResolveTableName();
if (expression instanceof SlotRef) {
resolveTableName = queryBlock.getRelation().getResolveTableName();
}
SlotRef alias = new SlotRef(resolveTableName, outputNames.get(outputExprIdx));
// order by expr may reference the alias. We need put the alias into the fieldMappings or the
// expressionToColumns.
// if the alias not be used in the order by expr like:
// select v1 cc, v2 cc from tbl order by v3. We just add the slotRef(cc) to columnRefOperator in the
// expressionToColumns to ensure this projection can output v1 with alias cc, v2 with alias cc and v3.
// We don't need to ensure the uniqueness of the alias in the fieldMappings.
// if the alias used in the order by expr like:
// select v1 v3, v2 cc from (select abs(v1) v1, abs(v2) v2, v3 from t0) t1 order by t1.v3.
// order by expr t1.v3 referenced the v1 with alias v3, we need set the fieldMappings to ensure order by
// expr can be resolved.
if (scope.getRelationFields().resolveFields(alias).size() > 1) {
outputTranslations.getExpressionToColumns()
.put(new SlotRef(resolveTableName, outputNames.get(outputExprIdx)), columnRefOperator);
} else {
outputTranslations.put(alias, columnRefOperator);
}

} else {
outputTranslations.putWithSymbol(expression, expression, columnRefOperator);
outputTranslations.put(expression, columnRefOperator);
}
outputExprIdx++;
}
Expand Down
51 changes: 47 additions & 4 deletions fe/fe-core/src/test/java/com/starrocks/sql/plan/OrderByTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,12 +519,22 @@ void testNotStrictOrderByExpression(String sql, String expectedPlan) throws Exce
assertContains(plan, expectedPlan);
}

@ParameterizedTest
@MethodSource("duplicateAliasSql")
void testDuplicateAlias(String sql, String expectedPlan) throws Exception {
String plan = getFragmentPlan(sql);
assertContains(plan, expectedPlan);
}

private static Stream<Arguments> allOrderBySql() {
return Stream.concat(successToStrictSql(), failToStrictSql());
}

private static Stream<Arguments> successToStrictSql() {
List<Arguments> list = Lists.newArrayList();
list.add(Arguments.of("select *, v1, abs(v1) v1 from t0 order by 1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select distinct *, v1, abs(v1) v1 from t0 order by 1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select distinct abs(v1) v1, * from t0 order by 1", "order by: <slot 4> 4: abs ASC"));
list.add(Arguments.of("select * from t0 order by 1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select abs(v1) v1, * from t0 order by 1", "order by: <slot 4> 4: abs ASC"));
list.add(Arguments.of("select distinct * from t0 order by 1", "order by: <slot 1> 1: v1 ASC"));
Expand All @@ -546,20 +556,53 @@ private static Stream<Arguments> successToStrictSql() {

private static Stream<Arguments> failToStrictSql() {
List<Arguments> list = Lists.newArrayList();
list.add(Arguments.of("select *, v1, abs(v1) v1 from t0 order by 1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select distinct *, v1, abs(v1) v1 from t0 order by 1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select distinct abs(v1) v1, * from t0 order by 1", "order by: <slot 4> 4: abs ASC"));
list.add(Arguments.of("select distinct *, v1, abs(v1) v1 from t0 order by v1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select v1, max(v2) v1 from t0 group by v1 order by abs(v1)", "order by: <slot 5> 5: abs ASC"));
list.add(Arguments.of("select max(v2) v1, v1 from t0 group by v1 order by abs(v1)", "order by: <slot 5> 5: abs ASC"));
list.add(Arguments.of("select v2, max(v2) v2 from t0 group by v2 order by max(v2)", "order by: <slot 4> 4: max ASC"));
list.add(Arguments.of("select max(v2) v2, v2 from t0 group by v2 order by max(v2)", "order by: <slot 4> 4: max ASC"));
list.add(Arguments.of("select upper(v1) v1, *, v1 from t0 order by v1", "order by: <slot 4> 4: upper ASC"));
list.add(Arguments.of("select *, v1, upper(v1) v1 from t0 order by v1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select distinct upper(v1) v1, *, v1 from t0 order by v1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select distinct v1, *, upper(v1) v1 from t0 order by v1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select distinct upper(v1) v1, *, v1 from t0 order by v1", "order by: <slot 4> 4: upper ASC"));
list.add(Arguments.of("select distinct *, v1, upper(v1) v1 from t0 order by v1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select distinct abs(v1) v1, v1 from t0 order by v1", "order by: <slot 4> 4: abs ASC"));

return list.stream();
}
<<<<<<< HEAD
}
=======

private static Stream<Arguments> duplicateAliasSql() {
List<Arguments> list = Lists.newArrayList();
list.add(Arguments.of("select *, v1 cc, v2 cc from t0 order by v1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select v1 cc, abs(v2) cc from t0 order by v1 ", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select distinct v1 cc, v2 cc from t0 order by v1", "order by: <slot 1> 1: v1 ASC"));
list.add(Arguments.of("select v1 cc, v2 cc, abs(v3) ccc from t0 order by abs(ccc)", "order by: <slot 5> 5: abs ASC"));
list.add(Arguments.of("select count(v1) cnt, count(v2) cnt from t0 group by v3 order by abs(v3)",
"3:SORT\n" +
" | order by: <slot 6> 6: abs ASC\n" +
" | offset: 0\n" +
" | \n" +
" 2:Project\n" +
" | <slot 4> : 4: count\n" +
" | <slot 5> : 5: count\n" +
" | <slot 6> : abs(3: v3)"));
list.add(Arguments.of("select v1 cc, v2 cc from (select abs(v1) v1, abs(v2) v2, v3 from t0) t1 order by v3",
"2:SORT\n" +
" | order by: <slot 3> 3: v3 ASC\n" +
" | offset: 0\n" +
" | \n" +
" 1:Project\n" +
" | <slot 3> : 3: v3\n" +
" | <slot 4> : abs(1: v1)\n" +
" | <slot 5> : abs(2: v2)"));
list.add(Arguments.of("select v1 v3, v2 cc from (select abs(v1) v1, abs(v2) v2, v3 from t0) t1 order by t1.v3",
"order by: <slot 4> 4: abs ASC"));
list.add(Arguments.of("select v1, v2 cc from (select abs(v1) v1, abs(v2) v2, v3 from t0) t1 order by t1.v3",
"order by: <slot 3> 3: v3 ASC"));
return list.stream();
}
}
>>>>>>> 885c5009be ([BugFix] skip add the alias into fieldMapping when order by expr not referenced this alias (#50546))

0 comments on commit 7b28773

Please sign in to comment.