lets-order-some-fries commented on PR #68096:
URL: https://github.com/apache/doris/pull/68096#issuecomment-5873650171
Thanks @morrySnow for running buildall — green across the board.
Both of the automated review's requests are addressed in `5fd32d86`
(test-only):
1. **UPDATE / DELETE keep the ordinal's digits.** `LogicalPlanBuilderTest`
parses `ORDER BY 4294967297`
and `ORDER BY 18446744073709551617` in both statements and asserts the
`UnboundSlot` is named
exactly after the literal.
2. **LARGEINT width, and both ORDER BY paths.** The GROUP BY test now also
uses
`18446744073709551617` (2^64 + 1), which narrows to 1 through
`getLongValue()` as well. A new test
covers ordinary and set-operation ORDER BY on the analyzed plan: a wide
ordinal must never become a
sort on a column, with `ORDER BY 1` as the control that shows the check
can see the difference.
@yx-keith — (2) is also the plan-level sort-key check you asked for on 22
Sep. I held it back then
because I couldn't run it; I can now.
**This time it ran locally, on a full `fe-core` build:**
- `LogicalPlanBuilderTest` + `BindExpressionTest`: 30 tests, 0 failures;
`checkstyle:check`: 0 violations
- with the fix reverted to `getIntValue()`, all three tests fail —
`DELETE FROM t ORDER BY 4294967297 LIMIT 10 ==> expected: <4294967297> but
was: <1>`
- with `getLongValue()` in both places, all three still fail, on the
LARGEINT value only —
`expected: <18446744073709551617> but was: <1>` — which is the gap the
review pointed out
Could someone re-run `/review` so the blocking review reflects the new head,
and `run buildall` for the
added tests?
@CalvinKirs — thank you, that was the missing piece. The `automation`
package ships thrift 0.24.0,
which took the FE reactor from 25 modules to 51 here. The last blocker on
Apple Silicon turned out to
be `protoc-gen-grpc-java`: the `osx-aarch_64` binary Maven Central publishes
for it is actually an
x86_64 executable, so without Rosetta `fe-grpc` fails with *"program not
found or is not
executable"*. A native build (Homebrew's `protoc-gen-grpc-java`) passed in
through
`-Dgrpc.java.artifact=...` gets the whole FE building — in case it saves the
next arm64 contributor an
evening.
--
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]
