mihaibudiu commented on code in PR #4692:
URL: https://github.com/apache/calcite/pull/4692#discussion_r2663256455
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -133,11 +133,14 @@
*/
public class RelToSqlConverter extends SqlImplementor
implements ReflectiveVisitor {
+
private final ReflectUtil.MethodDispatcher<Result> dispatcher;
private final Deque<Frame> stack = new ArrayDeque<>();
- /** Creates a RelToSqlConverter. */
+ /**
Review Comment:
It looks like your IDE has reformatted the JavaDoc comments. Can you please
undo these changes when they are not doing anything new?
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -11550,4 +11551,118 @@ public Sql schema(CalciteAssert.SchemaSpec
schemaSpec) {
relFn, transforms);
}
}
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7279";>[CALCITE-7279]
+ * ClickHouse dialect should wrap nested JOINs with explicit aliasing</a>. */
+ @Test void testClickHouseNestedJoin() {
+ final String query = "SELECT e.empno, j.dname, j.loc\n"
+ + "FROM emp e\n"
+ + "LEFT JOIN (\n"
+ + " SELECT d1.deptno, d1.dname, d2.loc\n"
+ + " FROM dept d1\n"
+ + " INNER JOIN dept d2 ON d1.deptno = d2.deptno\n"
+ + ") AS j ON e.deptno = j.deptno";
+
+ final String sql = sql(query)
+ .schema(CalciteAssert.SchemaSpec.JDBC_SCOTT)
+ .withClickHouse()
+ .exec();
+
+ // 1. Verify that the inner join is wrapped in a subquery
+ assertThat(sql, containsString("LEFT JOIN (SELECT"));
+
+ // 2. Verify explicit aliasing which is crucial for ClickHouse scoping.
+ // We must see "source AS alias" to ensure the identifier is resolvable
outer scope.
+ assertThat(sql, containsString("`DEPTNO` AS `DEPTNO`"));
+ assertThat(sql, containsString("`DNAME` AS `DNAME`"));
+ assertThat(sql, containsString("`LOC` AS `LOC`"));
+
+ // 3. Ensure we don't have redundant double-wrapping like (SELECT * FROM
(SELECT...))
+ // The structure should be: LEFT JOIN (SELECT expr AS col FROM ...) AS j
+ assertFalse(sql.contains("SELECT *"),
+ "ClickHouse dialect should avoid SELECT * in nested joins");
+ }
+
+ /** Test that simple JOINs without nesting are not wrapped unnecessarily. */
+ @Test void testClickHouseSimpleJoinNotWrapped() {
+ final String query = "SELECT e.empno, d.dname\n"
+ + "FROM emp e\n"
+ + "LEFT JOIN dept d ON e.deptno = d.deptno";
+
+ // Simple joins should remain flat as standard SQL
+ final String expected = "SELECT `EMP`.`EMPNO`, `DEPT`.`DNAME`\n"
+ + "FROM `SCOTT`.`EMP`\n"
+ + "LEFT JOIN `SCOTT`.`DEPT` ON `EMP`.`DEPTNO` = `DEPT`.`DEPTNO`";
+
+ sql(query)
+ .schema(CalciteAssert.SchemaSpec.JDBC_SCOTT)
+ .withClickHouse()
+ .ok(expected);
+ }
+
+ /** Test nested JOIN with aggregation to ensure expressions like COUNT(*)
are aliased. */
+ @Test void testClickHouseNestedJoinWithAggregation() {
+ final String query = "SELECT e.empno, j.cnt\n"
+ + "FROM emp e\n"
+ + "LEFT JOIN (\n"
+ + " SELECT d1.deptno, COUNT(*) as cnt\n"
+ + " FROM dept d1\n"
+ + " INNER JOIN dept d2 ON d1.deptno = d2.deptno\n"
+ + " GROUP BY d1.deptno\n"
+ + ") AS j ON e.deptno = j.deptno";
+
+ final String sql = sql(query)
+ .schema(CalciteAssert.SchemaSpec.JDBC_SCOTT)
+ .withClickHouse()
+ .exec();
+
+ // Aggregation results must be explicitly aliased for ClickHouse visibility
+ assertThat(sql, containsString("COUNT(*) AS `CNT`"));
Review Comment:
aggregation was aliased in the source query as well, do you want to check
what happens if it's not?
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -1105,10 +1229,10 @@ public boolean hasTrickyRollup(Sort e, Aggregate
aggregate) {
return !dialect.supportsAggregateFunction(SqlKind.ROLLUP)
&& dialect.supportsGroupByWithRollup()
&& (aggregate.getGroupType() == Aggregate.Group.ROLLUP
- || aggregate.getGroupType() == Aggregate.Group.CUBE
- && aggregate.getGroupSet().cardinality() == 1)
+ || aggregate.getGroupType() == Aggregate.Group.CUBE
Review Comment:
same for whitespace formatting.
CI may not like this new indentation anyway.
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -11550,4 +11551,118 @@ public Sql schema(CalciteAssert.SchemaSpec
schemaSpec) {
relFn, transforms);
}
}
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7279";>[CALCITE-7279]
+ * ClickHouse dialect should wrap nested JOINs with explicit aliasing</a>. */
+ @Test void testClickHouseNestedJoin() {
+ final String query = "SELECT e.empno, j.dname, j.loc\n"
+ + "FROM emp e\n"
+ + "LEFT JOIN (\n"
+ + " SELECT d1.deptno, d1.dname, d2.loc\n"
+ + " FROM dept d1\n"
+ + " INNER JOIN dept d2 ON d1.deptno = d2.deptno\n"
+ + ") AS j ON e.deptno = j.deptno";
+
+ final String sql = sql(query)
+ .schema(CalciteAssert.SchemaSpec.JDBC_SCOTT)
+ .withClickHouse()
+ .exec();
+
+ // 1. Verify that the inner join is wrapped in a subquery
+ assertThat(sql, containsString("LEFT JOIN (SELECT"));
Review Comment:
Frankly, it may be simpler and easier to understand what you are testing if
you just supplied the full expected result.
If there was only one such test, I could understand checking just substrings.
--
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]
