Copilot commented on code in PR #63479:
URL: https://github.com/apache/doris/pull/63479#discussion_r3279819741
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/MultiDistinctCount.java:
##########
@@ -57,6 +57,9 @@ private MultiDistinctCount(boolean distinct, List<Expression>
children) {
if (super.children().size() > 1) {
throw new AnalysisException("MultiDistinctCount's children size
must be 1");
}
+ for (Expression argument : super.children()) {
+ Count.checkDistinctArgument(argument, "COUNT DISTINCT " +
argument.toSql());
+ }
Review Comment:
`checkDistinctArgument` is called with `functionSql` built as `"COUNT
DISTINCT " + argument.toSql()`, which results in user-facing messages like
"COUNT DISTINCT does not support VARIANT argument in COUNT DISTINCT v"
(duplicated phrase and not valid SQL). Consider passing a SQL-like string such
as `COUNT(DISTINCT <arg>)`, or reusing the actual function `toSql()` if
available, so the error clearly points to what the user wrote.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Count.java:
##########
@@ -92,13 +92,24 @@ public void checkLegalityBeforeTypeCoercion() {
public void checkLegalityAfterRewrite() {
// after rewrite, count(distinct bitmap_column) should be rewritten to
bitmap_union_count(bitmap_column)
for (Expression argument : getArguments()) {
- if (distinct && (argument.getDataType().isComplexType()
- || argument.getDataType().isObjectType() ||
argument.getDataType().isJsonType())) {
- throw new AnalysisException("COUNT DISTINCT could not process
type " + this.toSql());
+ if (distinct) {
+ checkDistinctArgument(argument, this.toSql());
}
}
}
+ static void checkDistinctArgument(Expression argument, String functionSql)
{
+ DataType argumentType = argument.getDataType();
+ if (argumentType.isVariantType()) {
+ throw new AnalysisException("COUNT DISTINCT does not support
VARIANT argument in " + functionSql
+ + ". Cast the VARIANT expression to STRING or another
supported scalar type before using "
+ + "COUNT DISTINCT.");
Review Comment:
PR description says it closes #25672, but the linked issue #25672 (in this
PR metadata) is about supporting the JDBC catalog of GBase, which appears
unrelated to rejecting `COUNT(DISTINCT ...)` on VARIANT. Please verify the
issue number/reference is correct (or update the closing keyword to the right
issue).
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/MultiDistinctCount.java:
##########
@@ -67,6 +70,9 @@ protected MultiDistinctCount(AggregateFunctionParams
functionParams) {
@Override
public MultiDistinctCount withDistinctAndChildren(boolean distinct,
List<Expression> children) {
Preconditions.checkArgument(children.size() == 1,
"MultiDistinctCount's children size must be 1");
+ for (Expression argument : children) {
+ Count.checkDistinctArgument(argument, "COUNT DISTINCT " +
argument.toSql());
+ }
Review Comment:
Same as above: building `functionSql` as `"COUNT DISTINCT " +
argument.toSql()` makes the thrown error message awkward ("... in COUNT
DISTINCT v") and not valid SQL. Prefer `COUNT(DISTINCT <arg>)` (or similar) so
the message is clearer and consistent with `Count`'s `toSql()` formatting.
--
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]
