github-actions[bot] commented on code in PR #68254:
URL: https://github.com/apache/doris/pull/68254#discussion_r4056543613
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/TopNWeighted.java:
##########
@@ -105,8 +102,6 @@ public class TopNWeighted extends NullableAggregateFunction
.args(SmallIntType.INSTANCE, BigIntType.INSTANCE,
IntegerType.INSTANCE, IntegerType.INSTANCE),
FunctionSignature.ret(ArrayType.of(TinyIntType.INSTANCE))
.args(TinyIntType.INSTANCE, BigIntType.INSTANCE,
IntegerType.INSTANCE, IntegerType.INSTANCE),
- FunctionSignature.ret(ArrayType.of(BooleanType.INSTANCE))
Review Comment:
[P1] Finish aligning this signature table with BE
Removing BOOLEAN fixes one unsupported binding, but the same table still has
reachable correctness failures. In the default DecimalV3 mode there is no
DecimalV3 signature, so values bind to DOUBLE even though BE supports DecimalV3
natively; for example, `DECIMAL(20,0)` keys `9007199254740992` and
`9007199254740993` collapse to one DOUBLE key and their weights are merged.
With `enable_decimal_conversion=false`, DecimalV2 instead binds exactly but the
BE creator omits `TYPE_DECIMALV2`. The four-argument DATETIMEV2 entry also
declares scalar `VARCHAR` while BE returns an array, so result-type
verification rejects it. Please align these signatures with the BE contract and
add precision/type regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/TopNWeighted.java:
##########
@@ -68,8 +67,6 @@ public class TopNWeighted extends NullableAggregateFunction
.args(SmallIntType.INSTANCE, BigIntType.INSTANCE,
IntegerType.INSTANCE),
FunctionSignature.ret(ArrayType.of(TinyIntType.INSTANCE))
.args(TinyIntType.INSTANCE, BigIntType.INSTANCE,
IntegerType.INSTANCE),
- FunctionSignature.ret(ArrayType.of(BooleanType.INSTANCE))
Review Comment:
[P2] Add a regression for the replacement binding
These deletions make BOOLEAN succeed only because numeric compatibility
selects the first remaining DOUBLE signature and the analyzer inserts a cast,
but no committed test distinguishes that path from the old BE-unsupported
BOOLEAN signature. The current legality test never calls `topn_weighted`, and
the nearby regression uses only INT values. Please add analyzer assertions that
BOOLEAN becomes a DOUBLE child with an `ARRAY<DOUBLE>` result for both arities,
plus a BE-backed regression that executes both overloads; otherwise this exact
backend-initialization failure can return unnoticed.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/TopNWeighted.java:
##########
@@ -105,8 +102,6 @@ public class TopNWeighted extends NullableAggregateFunction
.args(SmallIntType.INSTANCE, BigIntType.INSTANCE,
IntegerType.INSTANCE, IntegerType.INSTANCE),
FunctionSignature.ret(ArrayType.of(TinyIntType.INSTANCE))
.args(TinyIntType.INSTANCE, BigIntType.INSTANCE,
IntegerType.INSTANCE, IntegerType.INSTANCE),
- FunctionSignature.ret(ArrayType.of(BooleanType.INSTANCE))
- .args(BooleanType.INSTANCE, BigIntType.INSTANCE,
IntegerType.INSTANCE, IntegerType.INSTANCE),
Review Comment:
[P1] Apply the BOOLEAN parity fix to the parallel top-N path
`TopNArray` still accepts BOOLEAN through `AnyDataType(0)`, which
concretizes both its child and result to BOOLEAN/`ARRAY<BOOLEAN>` without
adding a cast. Its BE creator uses the same allowed-type list as
`topn_weighted` and also omits `TYPE_BOOLEAN`, so `topn_array(TRUE, 1)` still
reaches a null creator and fails during backend initialization. Please give the
sibling path the same supported-type contract (conversion, rejection, or BE
implementation) and add an execution regression.
--
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]
