This is an automated email from the ASF dual-hosted git repository.
morrySnow pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new fd9c932fb14 [fix](analyzer) Decide a GROUP BY / ORDER BY ordinal on
the value the user wrote (#68096)
fd9c932fb14 is described below
commit fd9c932fb14b42bbff6ca73f95db6fb256a84b88
Author: Ambuj Upadhyay <[email protected]>
AuthorDate: Sat Oct 10 09:10:35 2026 +0530
[fix](analyzer) Decide a GROUP BY / ORDER BY ordinal on the value the user
wrote (#68096)
### What problem does this PR solve?
Related PR: #67008
Problem Summary:
`bindWithOrdinal` decides whether a numeric literal in `GROUP BY` /
`ORDER BY` is a positional
reference or an ordinary constant. It read the ordinal with
`IntegerLikeLiteral.getIntValue()` —
which is `getNumber().intValue()` — and only then asked whether it was
within the select list. That
call truncates a `BIGINT` or `LARGEINT` literal to its low 32 bits, so
an ordinal congruent to a
valid one modulo 2^32 passed a test written to reject it:
```sql
-- t1(col1 date, col2 int)
select col1, count(*) from t1 group by 4294967297; -- 2^32 + 1 -> (int) 1
-> GROUP BY col1
select col1, count(*) from t1 group by 3; -- correctly treated
as a constant
```
4294967297 is not a valid ordinal for a two-item select list, so it
should be treated as a constant
exactly as `3` is. Instead it bound to the first select item, so the
statement planned as
`GROUP BY col1` instead of being rejected like `group by 3`. The `ORDER
BY` forms behave the same
way — `select col1, col2 from t1 order by 2, 8589934593` sorts by
`(col2, col1)` where only `col2`
was a real sort key, which can change which rows survive a `LIMIT` over
ties.
`bindWithOrdinal` has three call sites (`GROUP BY`, `ORDER BY` over a
set operation, and `ORDER BY`
over any other query), so all three are affected. The decision is made
entirely in the FE and the
original literal is gone before the BE sees the plan, so nothing
downstream can recover the intended
meaning.
**Fix:** compare the literal at full width, and narrow only inside the
branch where the value has
been proven to lie in `1..selectItems`. `getBigDecimalValue()` is the
accessor to use rather than
`getLongValue()`: `LargeIntLiteral` overrides `getBigDecimalValue` but
not `getLongValue`, so a
long-based test would still mis-bind `group by 18446744073709551617`
(2^64 + 1).
The same narrowing sat one step earlier for `DELETE` / `UPDATE ... ORDER
BY`. There is no select
list there, so `convertSortOrdinalsToUnboundSlot` in
`LogicalPlanBuilder` turns an integer sort key
into an `UnboundSlot` named after the literal — and named it after
`getIntValue()`, so
`order by 4294967297` became a reference to a column called `1`. The
slot is now named with the
literal's exact digits, through the same `getBigDecimalValue()`
accessor, so a `LARGEINT` keeps its
digits too.
This is the same shape as #67008, where a 64-bit `LIMIT`/`OFFSET` was
narrowed before its range check
rather than after.
### Release note
Fixed `GROUP BY` / `ORDER BY` binding the wrong column when the
positional ordinal is a `BIGINT` or
`LARGEINT` literal outside the select-list range.
---------
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
---
.../doris/nereids/parser/LogicalPlanBuilder.java | 10 ++--
.../nereids/rules/analysis/BindExpression.java | 15 ++++--
.../nereids/parser/LogicalPlanBuilderTest.java | 24 ++++++++++
.../nereids/rules/analysis/BindExpressionTest.java | 54 ++++++++++++++++++++++
4 files changed, 97 insertions(+), 6 deletions(-)
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java
index 8f8d7cc4b90..d51cada09b6 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java
@@ -4603,9 +4603,13 @@ public class LogicalPlanBuilder extends
DorisParserBaseVisitor<Object> {
List<OrderKey> newOrderKeys = sort.getOrderKeys().stream()
.map(key -> {
if (key.getExpr() instanceof IntegerLikeLiteral) {
- return key.withExpression(
- new UnboundSlot(String.valueOf(
- ((IntegerLikeLiteral)
key.getExpr()).getIntValue())));
+ // The slot is named after the literal, so
narrowing it renames it:
+ // getIntValue() truncates to the low 32 bits,
turning
+ // `order by 4294967297` into a reference to "1".
Keep the digits
+ // the user wrote.
+ return key.withExpression(new UnboundSlot(
+ ((IntegerLikeLiteral) key.getExpr())
+
.getBigDecimalValue().toBigIntegerExact().toString()));
}
return key;
})
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java
b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java
index 3ee5312f4b0..49833b0b5e1 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java
@@ -128,6 +128,7 @@ import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.jetbrains.annotations.NotNull;
+import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
@@ -1941,9 +1942,17 @@ public class BindExpression implements
AnalysisRuleFactory {
private Expression bindWithOrdinal(
Expression unbound, SimpleExprAnalyzer analyzer, List<? extends
Expression> boundSelectOutput) {
if (unbound instanceof IntegerLikeLiteral) {
- int ordinal = ((IntegerLikeLiteral) unbound).getIntValue();
- if (ordinal >= 1 && ordinal <= boundSelectOutput.size()) {
- Expression boundSelectItem = boundSelectOutput.get(ordinal -
1);
+ // Decide positional-vs-constant on the value the user actually
wrote.
+ // getIntValue() is getNumber().intValue(), which truncates a
BIGINT or a
+ // LARGEINT to its low 32 bits, so an ordinal congruent to a valid
one
+ // modulo 2^32 passed this test and bound to a real select item:
+ // `GROUP BY 4294967297` silently became `GROUP BY <select item
1>`.
+ // LargeIntLiteral overrides getBigDecimalValue but not
getLongValue, so
+ // the BigDecimal form is the only one that is width independent.
+ BigDecimal ordinal = ((IntegerLikeLiteral)
unbound).getBigDecimalValue();
+ if (ordinal.compareTo(BigDecimal.ONE) >= 0
+ &&
ordinal.compareTo(BigDecimal.valueOf(boundSelectOutput.size())) <= 0) {
+ Expression boundSelectItem =
boundSelectOutput.get(ordinal.intValue() - 1);
return boundSelectItem instanceof Alias ?
boundSelectItem.child(0) : boundSelectItem;
} else {
return unbound; // bound literal
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/nereids/parser/LogicalPlanBuilderTest.java
b/fe/fe-core/src/test/java/org/apache/doris/nereids/parser/LogicalPlanBuilderTest.java
index 9690e24ea85..59c413cf82c 100644
---
a/fe/fe-core/src/test/java/org/apache/doris/nereids/parser/LogicalPlanBuilderTest.java
+++
b/fe/fe-core/src/test/java/org/apache/doris/nereids/parser/LogicalPlanBuilderTest.java
@@ -18,6 +18,7 @@
package org.apache.doris.nereids.parser;
import org.apache.doris.nereids.analyzer.UnboundSlot;
+import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.literal.IntegerLikeLiteral;
import org.apache.doris.nereids.trees.plans.commands.DeleteFromCommand;
import org.apache.doris.nereids.trees.plans.commands.DeleteFromUsingCommand;
@@ -248,4 +249,27 @@ public class LogicalPlanBuilderTest {
// Second key: column name remains as UnboundSlot
Assertions.assertInstanceOf(UnboundSlot.class,
sort.getOrderKeys().get(1).getExpr());
}
+
+ @Test
+ public void testDeleteAndUpdateOrderByWideOrdinalKeepsItsDigits() {
+ // The ordinal becomes an UnboundSlot named after the literal, so
narrowing the literal
+ // renames the slot. 4294967297 is 2^32 + 1, which getIntValue()
turned into "1".
+ // 18446744073709551617 is 2^64 + 1, a LARGEINT, which getLongValue()
also narrows to 1,
+ // so it pins the width-independent accessor rather than merely a
wider one.
+ for (String ordinal : new String[] {"4294967297",
"18446744073709551617"}) {
+ assertFirstOrderKeyIsSlotNamed("DELETE FROM t ORDER BY " + ordinal
+ " LIMIT 10", ordinal);
+ assertFirstOrderKeyIsSlotNamed("UPDATE t SET c1 = 10 ORDER BY " +
ordinal + " LIMIT 10", ordinal);
+ }
+ }
+
+ private void assertFirstOrderKeyIsSlotNamed(String sql, String
expectedName) {
+ LogicalPlan plan = parser.parseSingle(sql);
+ LogicalPlan query = plan instanceof UpdateCommand
+ ? ((UpdateCommand) plan).getLogicalQuery()
+ : ((DeleteFromUsingCommand) plan).getLogicalQuery();
+ LogicalSort<?> sort = (LogicalSort<?>) ((LogicalLimit<?>)
query).child();
+ Expression key = sort.getOrderKeys().get(0).getExpr();
+ Assertions.assertInstanceOf(UnboundSlot.class, key, sql);
+ Assertions.assertEquals(expectedName, ((UnboundSlot) key).getName(),
sql);
+ }
}
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindExpressionTest.java
b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindExpressionTest.java
index fd890d53233..56ab6bdb7a8 100644
---
a/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindExpressionTest.java
+++
b/fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindExpressionTest.java
@@ -17,18 +17,22 @@
package org.apache.doris.nereids.rules.analysis;
+import org.apache.doris.common.NereidsException;
import org.apache.doris.nereids.pattern.GeneratedPlanPatterns;
import org.apache.doris.nereids.rules.RulePromise;
import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.Slot;
import org.apache.doris.nereids.trees.expressions.functions.scalar.DayHourAdd;
import
org.apache.doris.nereids.trees.expressions.functions.scalar.HourSecondSub;
import
org.apache.doris.nereids.trees.expressions.functions.scalar.MicroSecondsAdd;
import
org.apache.doris.nereids.trees.expressions.functions.scalar.MicroSecondsSub;
import
org.apache.doris.nereids.trees.expressions.functions.scalar.YearMonthAdd;
import org.apache.doris.nereids.trees.plans.JoinType;
+import org.apache.doris.nereids.trees.plans.logical.LogicalSort;
import org.apache.doris.nereids.util.PlanChecker;
import org.apache.doris.utframe.TestWithFeService;
+import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
class BindExpressionTest extends TestWithFeService implements
GeneratedPlanPatterns {
@@ -45,6 +49,56 @@ class BindExpressionTest extends TestWithFeService
implements GeneratedPlanPatte
);
}
+ @Test
+ void testGroupByOrdinalIsNotNarrowed() {
+ // getIntValue() is getNumber().intValue(), so a BIGINT or LARGEINT
ordinal was truncated
+ // to its low 32 bits before the `>= 1 && <= selectItems` test.
4294967297 is 2^32 + 1 and
+ // 18446744073709551617 is 2^64 + 1: both truncated to 1 and silently
bound to the first
+ // select item, while the plainly out-of-range 3 did not. The LARGEINT
narrows to 1 through
+ // getLongValue() as well, so it pins the width-independent accessor,
not merely a wider one.
+ String outOfRange = "select col1, count(*) from t1 group by 3";
+
+ // As a constant, the literal leaves col1 ungrouped.
checkPlannerResult surfaces that as
+ // NereidsException, and every out-of-range ordinal must fail for that
same reason.
+ NereidsException expected =
Assertions.assertThrows(NereidsException.class,
+ () ->
PlanChecker.from(connectContext).checkPlannerResult(outOfRange),
+ "an ordinal past the end of the select list must not bind");
+ for (String ordinal : new String[] {"4294967297",
"18446744073709551617"}) {
+ String wrapsToOne = "select col1, count(*) from t1 group by " +
ordinal;
+ NereidsException actual =
Assertions.assertThrows(NereidsException.class,
+ () ->
PlanChecker.from(connectContext).checkPlannerResult(wrapsToOne),
+ ordinal + " must not be narrowed into range");
+ Assertions.assertEquals(expected.getMessage(), actual.getMessage(),
+ ordinal + " must be rejected for the same reason as the
out-of-range ordinal");
+ }
+ }
+
+ @Test
+ void testOrderByOrdinalIsNotNarrowed() {
+ // bindWithOrdinal also serves ORDER BY, on a plain select and on a
set operation. There an
+ // out-of-range ordinal does not throw -- it stays a constant sort key
-- so the check is on
+ // the analyzed plan: a wide ordinal must never turn into a sort on a
column.
+ String[] queries = {
+ "select col1, col2 from t1 order by %s",
+ "select col1, col2 from t1 union all select col1, col2 from t2
order by %s",
+ };
+ for (String query : queries) {
+ // The check can tell the two apart: a real ordinal does sort on a
column.
+ PlanChecker.from(connectContext)
+ .analyze(String.format(query, "1"))
+
.matches(logicalSort().when(BindExpressionTest::sortsOnASlot));
+ for (String ordinal : new String[] {"4294967297",
"18446744073709551617"}) {
+ PlanChecker.from(connectContext)
+ .analyze(String.format(query, ordinal))
+
.nonMatch(logicalSort().when(BindExpressionTest::sortsOnASlot));
+ }
+ }
+ }
+
+ private static boolean sortsOnASlot(LogicalSort<?> sort) {
+ return sort.getOrderKeys().stream().anyMatch(key -> key.getExpr()
instanceof Slot);
+ }
+
@Test
void testJoin() {
for (JoinType joinType : JoinType.values()) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
