mrhhsg commented on code in PR #68488:
URL: https://github.com/apache/doris/pull/68488#discussion_r4101116536
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoir.java:
##########
@@ -67,21 +70,42 @@ private PercentileReservoir(NullableAggregateFunctionParams
functionParams) {
@Override
public void checkLegalityBeforeTypeCoercion() {
+ checkLevel();
+ }
+
+ @Override
+ public void checkLegalityAfterRewrite() {
+ checkLevel();
+ }
+
+ /**
+ * The level must be a constant that folds to a literal in [0, 1]. It is
folded here instead of
+ * waiting for the rewrite phase because a constant expression such as
0.25 + 0.25 is only a
+ * literal after folding, some plans (INSERT ... VALUES, load column
mappings) never run the
+ * rewrite phase, and constant folding can be turned off by
debug_skip_fold_constant.
+ * The level is brought to DOUBLE with the same implicit cast that
signature coercion applies,
+ * so a level that is not a valid DOUBLE behaves like the coerced
expression: NULL under the
+ * default non-strict cast and an error under strict cast, for '' as well
as cast('' as double).
+ */
+ private void checkLevel() {
Expression levelArgument = getArgument(1);
- if (!levelArgument.isConstant()) {
+ Expression level = levelArgument.isConstant()
+ ? FoldConstantRuleOnFE.evaluateWithoutContext(
Review Comment:
Thanks. The divergence is real, but this PR does not introduce it, and the
suggested direction (make FE mirror BE) would regress normal queries. I am
keeping it out of this PR. Details:
- **Root cause.** The root cause is the general DECIMAL -> DOUBLE cast, not
the level check.
- FE `FractionalLiteral.uncheckedCastTo` uses `BigDecimal.doubleValue()`,
which is correctly rounded.
- BE `CastToFloat::_from_decimalv3` computes `(double)unscaled /
(double)10^scale`. That is not correctly rounded once the unscaled value
exceeds 2^53. For DECIMAL256 the value also goes through `long double`
(`wide::integer::operator double`), so the result depends on the platform.
- **The mismatch already exists at the merge base.** There,
`checkLegalityBeforeTypeCoercion` accepted the DECIMALV3 literal
`1.00000000000000009` through `getDouble()`, which is `1.0`. Likewise, a
literal level `0.12345678901234567` is folded by FE to `0.12345678901234566`
but evaluated by BE under `debug_skip_fold_constant` to `0.12345678901234568`.
So states built in the two folding modes could already fail to merge for any
high-precision literal level.
- **I tried the FE-side fix (local commit, not pushed).** Folding DECIMAL ->
DOUBLE with BE's formula made FE and BE agree for DECIMALV2 and
DECIMAL32/64/128. But it changes every folded decimal literal that is compared
with a DOUBLE:
- I tested on a local cluster with a DOUBLE column holding
`0.09090909090909091`. Today `WHERE d = 0.09090909090909091` returns 1 row.
With the change it returns 0 rows, because FE folds the literal to
`0.09090909090909093`.
- It still cannot match DECIMAL256, whose BE result depends on the
platform (x86_64 80-bit vs aarch64 binary128 `long double`). On x86_64, a
DECIMAL256 column value `0.481476460768815488307472416764355591160` is cast to
`0.48147646076881556`, while the correctly rounded value is
`0.4814764607688155`.
- **The right fix is on the BE side:** make BE's decimal -> double cast
correctly rounded, so every caller agrees with FE. That changes BE cast results
for all high-precision DECIMAL columns, so it should be its own BE change, not
part of this `percentile_reservoir` fix.
- **Impact on this function:**
- BE clamps the interpolation index to `[0, n - 1]`
(`ReservoirSampler::quantileInterpolated`), so a level that BE sees as
`1.0000000000000002` returns the same result as `1.0`.
- The cross-mode merge failure needs one state built with the debug switch
`debug_skip_fold_constant=true`, and a level with more than about 16
significant digits.
Resolving this thread for this PR. The BE-side cast rounding should be fixed
in a separate change.
--
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]
