[CALCITE-6894] ClickHouse dialect can support another form of writing aggFilterWhere #4247
Conversation
|
| if (call.getOperandList().size() > 1 && call.operand(1) != null) { | ||
| SqlCall filterCondition = call.operand(1); | ||
| String functionName = aggCall.getOperator().getName(); | ||
| writer.print(functionName.toLowerCase(Locale.ROOT) + "If"); |
There was a problem hiding this comment.
does this work for all aggregate functions?
Is there a stddevif?
There was a problem hiding this comment.
Thanks for review it. Clickhouse has such a logic. It will concatenate the aggregate function and -If instead of implementing a new xxxIf function. For example, whether stddevIf exists depends on whether Clickhouse supports stddev. I understand that the conversion of pr is correct. If more precision is needed, we may need to do a good job of mapping the functions from calcite to Clickhouse, but it seems that each dialect is being implemented one by one, which may be an area that needs improvement.
There was a problem hiding this comment.
Whether clickhouse supports stddev is what I am asking.
Calcite supports a set of aggregates, and clickhouse supports another set. If the Calcite set is included in the Clickhouse set, this change is fine.
There was a problem hiding this comment.
I think what you're saying is another issue, even if CASE WHEN is still used, if ClickHouse doesn't support STDDEV, then it can't also work. What do you think about creating another topic if needed?
There was a problem hiding this comment.
@xiedeyantu No, according to your writing above, all aggregate functions will execute the logic of your method, but you only described the sum aggregate function in the jira case, which seems a bit strange. If some functions of ClickHouse are not supported by Calcite, you at least need to record JavaDoc to explain it.
There was a problem hiding this comment.
Thanks, do I just need to add some extra comments? I want to confirm whether this conversion is needed. In fact, this conversion cannot solve the function mapping between the two dialects. This is actually similar to the syntax sugar of ClickHouse dialect.
There was a problem hiding this comment.
Whether clickhouse supports stddev is what I am asking. Calcite supports a set of aggregates, and clickhouse supports another set. If the Calcite set is included in the Clickhouse set, this change is fine.
@mihaibudiu After seeing your and cancai's replies, maybe I didn't explain clearly enough. Let me elaborate on the issue I mentioned:
ClickHouse doesn't support stddev. Converting stddev(x) filter(where) to stddevIf(x, condition) will throw an error in ClickHouse. What I meant in my previous reply was that using stddev(case when) would also throw an error. This PR is just making a formal transformation.
If we want to properly solve whether aggregate functions or other functions can be correctly translated, maybe creating a new JIRA ticket would be better. What do you think?
There was a problem hiding this comment.
Clickhouse supports stddev_samp and stddev_pop.
Converting unsupported code to unsupported code is not really helpful.
I think this code should handle correctly the supported functions and should report an error for the other cases
There was a problem hiding this comment.
Well, I understand. We may not need this PR. Correctly supporting function conversion may be a problem faced by all dialects. If this PR only handles some aggregation functions supported by ClickHouse, it may not be a good solution. So I will temporarily close this PR for now.
There was a problem hiding this comment.
But the - If form does have performance improvements, it's not just a formal transformation. see ClickHouse/ClickHouse#17043 and others.
| @@ -361,7 +361,13 @@ private static String toSql(RelNode root, SqlDialect dialect, | |||
| + "FROM `foodmart`.`product`\n" | |||
| + "WHERE `product_id` > 0\n" | |||
| + "GROUP BY `product_id`"; | |||
| final String expectedClickHouse = "SELECT" | |||
There was a problem hiding this comment.
Can you add more tests for different types of aggregate functions?
Jira link: CALCITE-6894