github-actions[bot] commented on code in PR #68096:
URL: https://github.com/apache/doris/pull/68096#discussion_r4119561069
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -4601,9 +4601,13 @@ private LogicalPlan
convertSortOrdinalsToUnboundSlot(LogicalPlan plan) {
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(
Review Comment:
Please add a parser test for UPDATE and DELETE with a value such as `ORDER
BY 4294967297`, and assert that the resulting `UnboundSlot` has the exact name
`4294967297`. The existing command parser tests use `ORDER BY 1` and check only
the node type, while the new binder test exercises GROUP BY. Reverting this
conversion to `getIntValue()` would make `4294967297` bind as slot `1` again
without failing either test.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/BindExpressionTest.java:
##########
@@ -45,6 +47,28 @@ protected void runBeforeAll() throws Exception {
);
}
+ @Test
+ void testOrdinalIsNotNarrowedTo32Bits() {
+ // 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, which truncated to 1 and silently bound to
the
+ // first select item, while the plainly out-of-range 3 did not. Both
are out
+ // of range for a two-item select list and must be treated the same
way.
+ String outOfRange = "select col1, count(*) from t1 group by 3";
+ String wrapsToOne = "select col1, count(*) from t1 group by
4294967297";
Review Comment:
Could this regression test also use a LARGEINT value such as
`18446744073709551617` (2^64 + 1), and cover wide ordinals in ordinary and
set-operation ORDER BY? The current `4294967297` is a BIGINT, so a change from
`getBigDecimalValue()` to `getLongValue()` would leave the test green even
though that LARGEINT value narrows to 1 and binds to select item 1. Those sort
paths also call `bindWithOrdinal`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
