github-actions[bot] commented on code in PR #64624:
URL: https://github.com/apache/doris/pull/64624#discussion_r3545457536
##########
be/src/exprs/aggregate/aggregate_function_percentile.h:
##########
@@ -201,7 +213,7 @@ class AggregateFunctionPercentileApproxBase
void check_input_columns_type(const IColumn** columns) const override {
for (size_t i = 0; i < this->argument_types.size(); ++i) {
- this->template
check_argument_column_type<ColumnFloat64>(columns[i]);
+ this->template
check_const_argument_column_type<ColumnFloat64>(columns[i]);
Review Comment:
`check_input_columns_type()` now calls `check_const_argument_column_type()`
for every `percentile_approx` argument, but `get_const_argument_indexes()` only
marks the quantile/compression positions as required constants. In the normal
aggregate path `_calc_argument_columns()` wraps only those indexes, so
`percentile_approx(col_double, 0.5)` still reaches this checker with
`columns[0]` as a plain `ColumnFloat64`; the weighted forms likewise pass
non-const value/weight columns at indexes 0/1. The helper then assert-casts
those ordinary columns to `ColumnConst` before `add()` can run. Please keep
source/weight inputs on `check_argument_column_type()` and apply the
const-column check only to the required constant positions.
##########
be/src/exprs/aggregate/aggregate_function_percentile.h:
##########
@@ -654,30 +682,33 @@ class AggregateFunctionPercentile final
DataTypePtr get_return_type() const override { return
std::make_shared<DataTypeFloat64>(); }
+ const std::vector<size_t>& get_const_argument_indexes() const override {
+ static const std::vector<size_t> indexes {1};
+ return indexes;
+ }
+
void add(AggregateDataPtr __restrict place, const IColumn** columns,
ssize_t row_num,
Arena&) const override {
const auto& sources =
assert_cast<const ColVecType&,
TypeCheckOnRelease::DISABLE>(*columns[0]);
- const auto& quantile =
- assert_cast<const ColumnFloat64&,
TypeCheckOnRelease::DISABLE>(*columns[1]);
+ const auto& quantile_column =
*check_and_get_column_with_const<ColumnFloat64>(*columns[1]);
AggregateFunctionPercentile::data(place).add(sources.get_data()[row_num],
- quantile.get_data(),
NullMap(1, 0), 1);
+
quantile_column.get_data()[0]);
}
void add_batch_single_place(size_t batch_size, AggregateDataPtr place,
const IColumn** columns,
Arena&) const override {
const auto& sources =
assert_cast<const ColVecType&,
TypeCheckOnRelease::DISABLE>(*columns[0]);
- const auto& quantile =
- assert_cast<const ColumnFloat64&,
TypeCheckOnRelease::DISABLE>(*columns[1]);
DCHECK_EQ(sources.get_data().size(), batch_size);
+ const auto& quantile_column =
*check_and_get_column_with_const<ColumnFloat64>(*columns[1]);
AggregateFunctionPercentile::data(place).add_batch(sources.get_data(),
-
quantile.get_data()[0]);
+
quantile_column.get_data()[0]);
}
void check_input_columns_type(const IColumn** columns) const override {
this->template check_argument_column_type<ColVecType>(columns[0]);
- this->template check_argument_column_type<ColumnFloat64>(columns[1]);
+ this->template
check_const_argument_column_type<ColumnFloat64>(columns[1]);
Review Comment:
The ordinary aggregate path now re-wraps required constants in
`_calc_argument_columns()`, but analytic/window aggregation never uses that
path. `AnalyticSinkOperatorX::_insert_range_column()` executes each aggregate
argument, immediately calls `convert_to_full_column_if_const()`, stores the
full mutable column, and `_execute_for_function()` later passes those stored
columns into `AggFnEvaluator::add_range_single_place()`. This new const-only
check therefore breaks existing window calls such as `percentile(b, 0.5)
over(...)` (and the same shape applies to `percentile_reservoir(..., 0.5)` and
the added EMA window case), because the quantile reaches this checker as
`ColumnFloat64`, not `ColumnConst`. Please preserve/re-create const columns for
required-const window aggregate arguments, or relax the window
checker/add-range path to accept the materialized constant value.
##########
regression-test/suites/query_p0/aggregate/support_type/percentile/percentile.groovy:
##########
@@ -103,4 +103,21 @@ suite("percentile") {
qt_percentile_all_nan """select percentile(v_double, 0.5) from
percentile_all_nan_t;"""
qt_percentile_array_all_nan """select percentile_array(v_double, [0.25,
0.5, 0.75]) from percentile_all_nan_t;"""
qt_percentile_approx_all_nan """select percentile_approx(v_double, 0.5)
from percentile_all_nan_t;"""
-}
\ No newline at end of file
+
+ test {
+ sql """select percentile(col_double, -0.1) from d_table;"""
+ exception "percentile quantile must be in [0, 1]"
+ }
+ test {
+ sql """select percentile(col_double, 1.1) from d_table;"""
+ exception "percentile quantile must be in [0, 1]"
+ }
+ test {
+ sql """select percentile_state(col_double, 1.1) from d_table;"""
+ exception "percentile quantile must be in [0, 1]"
+ check { result, exception, startTime, endTime ->
Review Comment:
These `_state` tests set an `exception` string, but adding a custom `check`
closure means `TestAction.run()` skips the built-in exception substring
assertion and calls only the closure. As written, any non-`INTERNAL_ERROR`
exception would pass, including a stale-nested/runtime failure instead of the
intended percentile legality error. Please either remove these custom `check`
blocks or also assert that `exception.toString()` contains the expected
percentile error in each new `_state` case.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java:
##########
@@ -144,4 +144,9 @@ public boolean canRollUp() {
public void checkLegalityBeforeTypeCoercion() {
nested.checkLegalityBeforeTypeCoercion();
}
+
+ @Override
+ public void checkLegalityAfterRewrite() {
+ nested.checkLegalityAfterRewrite();
Review Comment:
`StateCombinator.withChildren()` rebuilds the wrapper with rewritten
children but keeps the original `nested` aggregate. `CheckLegalityAfterRewrite`
rewrites children before calling this method, so for a call like
`percentile_state(k, 0.5 + 0.6)` the wrapper child can be folded to `1.1` while
`nested` still holds the old arithmetic expression. This delegation then checks
that stale nested aggregate; `Percentile.checkLegalityAfterRewrite()` returns
for a non-`Literal` quantile, so the invalid `_state` call bypasses the FE
error that direct `percentile(k, 1.1)` now gets. Please rebuild the nested
aggregate from the current `StateCombinator` children, or otherwise keep
`nested` synchronized before delegating legality checks.
--
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]
