924060929 commented on code in PR #68503:
URL: https://github.com/apache/doris/pull/68503#discussion_r4266016868
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/window/Ntile.java:
##########
@@ -80,12 +82,27 @@ public void checkLegalityBeforeTypeCoercion() {
throw new AnalysisException(
"The bucket of NTILE must be a constant value: " +
this.toSql());
}
- if (buckets instanceof Literal) {
- if (((Literal) buckets).getDouble() <= 0) {
- throw new AnalysisException(
- "The bucket parameter of NTILE must be a constant positive
integer: " + this.toSql());
- }
- } else {
+ // The bucket may be a constant expression such as `1 + 1`, which is
folded to a literal only by the
+ // rewrite phase after this check runs. Reject early a bucket that FE
already evaluates to a non-positive
+ // or NULL value. A bucket FE can not evaluate, e.g. `3 % 2`, may
still be folded by BE when
+ // enable_fold_constant_by_be is set, so it is left to
checkLegalityAfterRewrite.
+ Expression evaluated = ExpressionUtils.foldConstantArgument(buckets);
+ if (evaluated instanceof Literal) {
+ checkPositiveBucket(evaluated);
+ }
+ }
+
+ @Override
+ public void checkLegalityAfterRewrite() {
+ // The backend reads the bucket from the argument column and registers
ntile only for a non-nullable
+ // integer argument, so the bucket must have been folded to a literal
before the plan is translated.
+ // That does not happen when constant folding is skipped, e.g. with
debug_skip_fold_constant=true, or
+ // when the configured folding can not evaluate the bucket.
+ checkPositiveBucket(getArgument(0));
Review Comment:
Confirmed with actual SQL execution on a local native cluster. I kept the
same ASAN BE, table, data and query settings, and swapped only the FE packages
built from the commits below.
**Reproduction**
```sql
CREATE DATABASE IF NOT EXISTS pr68503_ab;
USE pr68503_ab;
DROP TABLE IF EXISTS t;
CREATE TABLE t (k1 INT NOT NULL)
DUPLICATE KEY(k1)
DISTRIBUTED BY HASH(k1) BUCKETS 1
PROPERTIES ("replication_num" = "1");
INSERT INTO t VALUES (1), (1), (2), (2), (3), (3);
SELECT 2 AS two, k1, RAND() AS r
FROM t
ORDER BY NTILE(2) OVER (ORDER BY k1), r + r;
```
**Observed results**
| FE source commit | Original query | `EXPLAIN VERBOSE` of the same query |
|---|---|---|
| PR merge-base on master: `44da2ee105878448b40705e1029861d4b9f71a8c` |
Success, 6 rows | Success |
| Master fetched for this experiment:
`1f3cd9092fbc9151a3cc038d36e32aef68bd8357` | Success, 6 rows | Success |
| PR head: `758ca77863596f098dd0bd5e2ae5564f106112b1` | Fails during FE
planning | Same failure |
The PR error is:
```text
ERROR 1105 (HY000): errCode = 2, detailMessage =
The bucket parameter of NTILE must be a constant positive integer: ntile(two)
```
Both master plans contain:
```text
VANALYTIC
functions: [ntile(two[#4])]
```
Thus the slot representation already exists on master and executes
successfully; the new literal-only guard introduces the rejection. Restoring
the PR FE package reproduced the same error.
As controls, changing only `2 AS two` to `3 AS three` makes the query
succeed on the PR with 6 rows, and a plain `NTILE(2)` query succeeds with
bucket values `1,1,1,2,2,2`. The random values themselves naturally differ
between executions. FE package identities were checked using source commits and
the JAR bytecode/class fingerprints; the generated build-version display string
was reused across checkouts.
**Suggested change**
Preserve already-validated constant NTILE arguments during ORDER BY alias
substitution, or restore proven constants at the window-normalization boundary
before the final legality check. Keep the unconditional positive-integer
validation, including the translation guard when rewrite rules are disabled. Do
not allow arbitrary slot arguments or force projection merging across repeated
uses of `RAND()`. Please add the reproduction above as a regression test
covering the matching selected constant and repeatedly used volatile projection.
This confirms a P2 query-planning regression: previously valid SQL is
rejected in FE. The observed path does not return incorrect rows or reach a BE
crash. These are manual end-to-end SQL runs; I did not run the UT/regression
suites.
--
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]
