mrhhsg commented on code in PR #68503:
URL: https://github.com/apache/doris/pull/68503#discussion_r4269573708
##########
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:
Fixed in 215f9102acb.
- `ReplaceExpressionByChildOutput.buildOutputAliasMap` now skips every
constant alias (`!alias.child().isConstant()`) instead of only `Literal` ones,
so `1 + 1 AS two` and `CAST(5 - 2 AS BIGINT) AS three` no longer put
`Add`/`Cast` into the replacement map and the NTILE bucket stays a constant
expression that the configured folding turns into a literal. Replacing a
constant with a slot never saves work (it is folded once anyway), so
non-constant aliases such as `RAND() AS r` or `k1 + 1 AS x` keep being reused
exactly as before. The positive-bucket checks after rewrite and at translation
are unchanged, and project merging across the repeated `RAND()` is not touched.
- Tests:
-
`ReplaceExpressionByChildOutputTest.testSortProjectDoesNotReplaceConstantSubExpression`
covers both a literal alias and a `1 + 1` alias.
- `test_ntile_function` adds the ORDER-BY-only reproduction with a
matching selected constant and a repeatedly used volatile projection for `2`,
`1 + 1` and `CAST(5 - 2 AS BIGINT)`: `select <const> as two, k1, k2, k3,
floor(rand()) as r from t order by ntile(<const>) over (order by k1,k2,k3), r +
r, k1, k2, k3`. `floor(rand())` is still volatile (it contains `rand()`, so
`canMergeWithProjections` still refuses to merge the projection) but is always
0, which keeps the generated `.out` stable. The window is no longer selected,
so whole-window substitution cannot short-circuit the bucket visit.
- Verified locally: both targeted FE UTs pass (16/16), the regression suite
passes with regenerated output, and your two original SQL statements with a
real `RAND()` return all rows on a local cluster; `EXPLAIN` shows `functions:
[ntile(2)]` with `floor(random())` left in the projection below the window.
--
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]
