Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CALCITE-6894] ClickHouse dialect can support another form of writing aggFilterWhere #4247

Closed
wants to merge 1 commit into from
Closed

[CALCITE-6894] ClickHouse dialect can support another form of writing aggFilterWhere #4247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions core/src/main/java/org/apache/calcite/sql/dialect/ClickHouseSqlDialect.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.calcite.sql.SqlDataTypeSpec;
import org.apache.calcite.sql.SqlDateLiteral;
import org.apache.calcite.sql.SqlDialect;
import org.apache.calcite.sql.SqlFilterOperator;
import org.apache.calcite.sql.SqlLiteral;
import org.apache.calcite.sql.SqlNode;
import org.apache.calcite.sql.SqlTimeLiteral;
Expand All @@ -38,6 +39,8 @@

import org.checkerframework.checker.nullness.qual.Nullable;

import java.util.Locale;

import static org.apache.calcite.util.RelToSqlConverterUtil.unparseBoolLiteralToCondition;

import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -78,6 +81,10 @@ public ClickHouseSqlDialect(Context context) {
return false;
}

@Override public boolean supportsAggregateFunctionFilter() {
return true;
}

@Override public CalendarPolicy getCalendarPolicy() {
return CalendarPolicy.SHIFT;
}
Expand Down Expand Up @@ -181,6 +188,21 @@ private static SqlDataTypeSpec createSqlDataTypeSpecByName(String typeAlias,
return;
}

if (call.getOperator() instanceof SqlFilterOperator) {
SqlCall aggCall = call.operand(0);
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this work for all aggregate functions?
Is there a stddevif?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@caicancai caicancai Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the - If form does have performance improvements, it's not just a formal transformation. see ClickHouse/ClickHouse#17043 and others.

writer.print("(");
aggCall.operand(0).unparse(writer, 0, 0);
writer.print(", ");
writer.print(filterCondition.getOperandList().get(0).toString());
writer.print(")");
return;
}
}

switch (call.getKind()) {
case FLOOR:
if (call.operandCount() != 2) {
Expand Down
6 changes: 6 additions & 0 deletions core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add more tests for different types of aggregate functions?

+ " sumIf(`shelf_width` , `net_weight` > 0E0), SUM(`shelf_width`)\n"
+ "FROM `foodmart`.`product`\n"
+ "WHERE `product_id` > 0\n"
+ "GROUP BY `product_id`";
sql(query).ok(expectedDefault)
.withClickHouse().ok(expectedClickHouse)
.withBigQuery().ok(expectedBigQuery)
.withFirebolt().ok(expectedFirebolt)
.withMysql().ok(expectedMysql)
Expand Down