From 20e29911657d4673ec0a72399b2e2e6684e6afa8 Mon Sep 17 00:00:00 2001 From: packy92 <110370499+packy92@users.noreply.github.com> Date: Mon, 2 Sep 2024 18:59:52 +0800 Subject: [PATCH] [BugFix] skip add the alias into fieldMapping when order by expr not referenced this alias (#50546) Signed-off-by: packy92 (cherry picked from commit 885c5009be6b9323a8b0926c9a344104c52cd692) --- .../transformer/QueryTransformer.java | 42 +++++++++++++---- .../com/starrocks/sql/plan/OrderByTest.java | 47 +++++++++++++++++-- 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/fe/fe-core/src/main/java/com/starrocks/sql/optimizer/transformer/QueryTransformer.java b/fe/fe-core/src/main/java/com/starrocks/sql/optimizer/transformer/QueryTransformer.java index 7f34a150337bf..8a38316db9b2b 100644 --- a/fe/fe-core/src/main/java/com/starrocks/sql/optimizer/transformer/QueryTransformer.java +++ b/fe/fe-core/src/main/java/com/starrocks/sql/optimizer/transformer/QueryTransformer.java @@ -26,6 +26,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; @@ -110,10 +111,10 @@ public LogicalPlan plan(SelectRelation queryBlock, ExpressionMapping outer) { Iterables.concat(queryBlock.getOutputExpression(), 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 @@ -121,10 +122,10 @@ public LogicalPlan plan(SelectRelation queryBlock, ExpressionMapping outer) { // 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()); } } @@ -164,10 +165,12 @@ private OptExprBuilder planFrom(Relation node, CTETransformerContext cteContext) private OptExprBuilder projectForOrder(OptExprBuilder subOpt, Iterable outputExpression, - List outputExprInOrderByScope, + SelectRelation queryBlock, List outputNames, - List sourceExpression, Scope scope, + List sourceExpression, boolean withAggregation) { + List outputExprInOrderByScope = queryBlock.getOutputExprInOrderByScope(); + Scope scope = queryBlock.getOrderScope(); ExpressionMapping outputTranslations = new ExpressionMapping(scope); Map projections = Maps.newHashMap(); @@ -185,10 +188,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++; } diff --git a/fe/fe-core/src/test/java/com/starrocks/sql/plan/OrderByTest.java b/fe/fe-core/src/test/java/com/starrocks/sql/plan/OrderByTest.java index 7fa94cac69705..7ef48d90b544c 100644 --- a/fe/fe-core/src/test/java/com/starrocks/sql/plan/OrderByTest.java +++ b/fe/fe-core/src/test/java/com/starrocks/sql/plan/OrderByTest.java @@ -676,12 +676,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 allOrderBySql() { return Stream.concat(successToStrictSql(), failToStrictSql()); } private static Stream successToStrictSql() { List list = Lists.newArrayList(); + list.add(Arguments.of("select *, v1, abs(v1) v1 from t0 order by 1", "order by: 1: v1 ASC")); + list.add(Arguments.of("select distinct *, v1, abs(v1) v1 from t0 order by 1", "order by: 1: v1 ASC")); + list.add(Arguments.of("select distinct abs(v1) v1, * from t0 order by 1", "order by: 4: abs ASC")); list.add(Arguments.of("select * from t0 order by 1", "order by: 1: v1 ASC")); list.add(Arguments.of("select abs(v1) v1, * from t0 order by 1", "order by: 4: abs ASC")); list.add(Arguments.of("select distinct * from t0 order by 1", "order by: 1: v1 ASC")); @@ -703,9 +713,6 @@ private static Stream successToStrictSql() { private static Stream failToStrictSql() { List list = Lists.newArrayList(); - list.add(Arguments.of("select *, v1, abs(v1) v1 from t0 order by 1", "order by: 1: v1 ASC")); - list.add(Arguments.of("select distinct *, v1, abs(v1) v1 from t0 order by 1", "order by: 1: v1 ASC")); - list.add(Arguments.of("select distinct abs(v1) v1, * from t0 order by 1", "order by: 4: abs ASC")); list.add(Arguments.of("select distinct *, v1, abs(v1) v1 from t0 order by v1", "order by: 1: v1 ASC")); list.add(Arguments.of("select v1, max(v2) v1 from t0 group by v1 order by abs(v1)", "order by: 5: abs ASC")); list.add(Arguments.of("select max(v2) v1, v1 from t0 group by v1 order by abs(v1)", "order by: 5: abs ASC")); @@ -713,10 +720,42 @@ private static Stream failToStrictSql() { list.add(Arguments.of("select max(v2) v2, v2 from t0 group by v2 order by max(v2)", "order by: 4: max ASC")); list.add(Arguments.of("select upper(v1) v1, *, v1 from t0 order by v1", "order by: 4: upper ASC")); list.add(Arguments.of("select *, v1, upper(v1) v1 from t0 order by v1", "order by: 1: v1 ASC")); - list.add(Arguments.of("select distinct upper(v1) v1, *, v1 from t0 order by v1", "order by: 1: v1 ASC")); + list.add(Arguments.of("select distinct v1, *, upper(v1) v1 from t0 order by v1", "order by: 1: v1 ASC")); + list.add(Arguments.of("select distinct upper(v1) v1, *, v1 from t0 order by v1", "order by: 4: upper ASC")); list.add(Arguments.of("select distinct *, v1, upper(v1) v1 from t0 order by v1", "order by: 1: v1 ASC")); list.add(Arguments.of("select distinct abs(v1) v1, v1 from t0 order by v1", "order by: 4: abs ASC")); return list.stream(); } + + private static Stream duplicateAliasSql() { + List list = Lists.newArrayList(); + list.add(Arguments.of("select *, v1 cc, v2 cc from t0 order by v1", "order by: 1: v1 ASC")); + list.add(Arguments.of("select v1 cc, abs(v2) cc from t0 order by v1 ", "order by: 1: v1 ASC")); + list.add(Arguments.of("select distinct v1 cc, v2 cc from t0 order by v1", "order by: 1: v1 ASC")); + list.add(Arguments.of("select v1 cc, v2 cc, abs(v3) ccc from t0 order by abs(ccc)", "order by: 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: 6: abs ASC\n" + + " | offset: 0\n" + + " | \n" + + " 2:Project\n" + + " | : 4: count\n" + + " | : 5: count\n" + + " | : 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: 3: v3 ASC\n" + + " | offset: 0\n" + + " | \n" + + " 1:Project\n" + + " | : 3: v3\n" + + " | : abs(1: v1)\n" + + " | : 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: 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: 3: v3 ASC")); + return list.stream(); + } } \ No newline at end of file