924060929 commented on code in PR #68503:
URL: https://github.com/apache/doris/pull/68503#discussion_r4268991908
##########
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:
Re-tested at `8e0c8ccb358b6268d6ed9efb0242a137eec1c08d` on the same native
cluster and six-row table from the reproduction above.
The original `2 AS two` / `NTILE(2)` query is now fixed: it returns 6 rows,
and `EXPLAIN VERBOSE` shows `functions: [ntile(2)]`.
However, the protection still only covers literals. Matching foldable
constant-expression aliases fail:
```sql
USE pr68503_ab;
SELECT 1 + 1 AS two, k1, RAND() AS r
FROM t
ORDER BY NTILE(1 + 1) OVER (ORDER BY k1), r + r;
-- ERROR 1105:
-- The bucket parameter of NTILE must be a constant positive integer:
ntile(two)
SELECT CAST(5 - 2 AS BIGINT) AS three, k1, RAND() AS r
FROM t
ORDER BY NTILE(CAST(5 - 2 AS BIGINT)) OVER (ORDER BY k1), r + r;
-- ERROR 1105:
-- The bucket parameter of NTILE must be a constant positive integer:
ntile(three)
```
As a control, replacing the matching selected expression with a literal
makes `NTILE(1 + 1)` succeed with 6 rows. Therefore folding itself works; the
failure depends on the alias substitution.
In `ReplaceExpressionByChildOutput.buildOutputAliasMap`, `alias.child()
instanceof Literal` excludes literal `2`, but retains `Add` and `Cast`. The
matching bucket is replaced with a slot before folding. The repeatedly used
volatile projection still prevents project merging, and the final NTILE guard
rejects that slot.
Please extend the protection to the foldable constant expressions this PR
introduces, or preserve/restore constant-required function arguments at the
normalization boundary. Keep the unconditional positive-value checks and the
volatile projection protections.
The new regression also selects the same window expression that appears in
ORDER BY, which allows the rule to reuse the whole window output and
short-circuit before reaching its bucket. Please add the original ORDER-BY-only
window + repeated `RAND()` reproduction and the two expression-alias variants
above.
FE build/Checkstyle and the 16 focused tests passed (4
`ReplaceExpressionByChildOutputTest`, 12 `NtileBucketTest`; zero
failures/errors/skips). The original literal regression is fixed; the remaining
P2 is incomplete support for the newly accepted constant expressions, rather
than a regression against the old master behavior. The observed failures occur
during FE planning, with no incorrect result or BE crash.
--
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]
