mrhhsg commented on code in PR #68503:
URL: https://github.com/apache/doris/pull/68503#discussion_r4104163552
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/window/Ntile.java:
##########
@@ -80,12 +81,22 @@ 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. Evaluate it here so that any
constant expression yielding a
+ // positive integer is accepted, while a non-positive or unevaluable
bucket is still rejected.
+
checkPositiveBucket(FoldConstantRuleOnFE.evaluateWithoutContext(buckets));
Review Comment:
Fixed in 028caecd2ba.
- `Ntile.checkLegalityBeforeTypeCoercion` still evaluates the bucket with
`FoldConstantRuleOnFE.evaluateWithoutContext`. It now rejects the bucket early
only when FE produces a literal and that literal is not a positive integer (`1
- 1`, `cast('abc' as int)`, ...). If FE cannot evaluate the bucket (`5 % 3`,
`crc32('a') % 3 + 3`), the bucket is no longer rejected at binding time. It is
then folded by the configured `FoldConstantRule` path, including BE when
`enable_fold_constant_by_be=true`, and `checkLegalityAfterRewrite` requires a
positive integer literal after that.
- Unconditional guard: `ExpressionTranslator.visitNtile` calls
`checkLegalityAfterRewrite()` again before translating. This covers queries
that disable the rewrites which fold and check the window expression
(`disable_nereids_rules='REWRITE_PROJECT_EXPRESSION,REWRITE_WINDOW_EXPRESSION'`).
In that case a non-literal or zero bucket never reaches BE, where it would be
used as a divisor.
- Tests:
- `NtileBucketTest` now also checks that a `3 % 2` bucket is accepted at
binding time and rejected after rewrite if it is still unfolded. It also checks
that translation accepts a literal bucket and rejects an unfolded bucket and a
zero bucket.
- `test_ntile_function` adds `ntile(5 % 3)` and `ntile(crc32('a') % 3 +
3)` with `enable_fold_constant_by_be=true`, both with generated output. It adds
error cases for `ntile(3 % 3)` in that mode and for `ntile(5 % 3)` with FE-only
folding. It also runs a literal bucket and an unfolded bucket with the folding
rewrites disabled.
--
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]
