You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "bvolpato (via GitHub)" <gi...@apache.org> on 2026/03/09 20:11:45 UTC
[PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
bvolpato opened a new pull request, #4829:
URL: https://github.com/apache/calcite/pull/4829
## Summary
RelToSqlConverter emitted an ambiguous GROUP BY key after semi-join rewrites for a DISTINCT query over LEFT JOIN ... USING.
## Reproduction query
```sql
WITH product_keys AS (
SELECT p."product_id",
(SELECT MAX(p3."product_id")
FROM "foodmart"."product" p3
WHERE p3."product_id" = p."product_id") AS "mx"
FROM "foodmart"."product" p
)
SELECT DISTINCT pk."product_id"
FROM product_keys pk
LEFT JOIN "foodmart"."product" p2 USING ("product_id")
WHERE pk."product_id" IN (
SELECT p4."product_id"
FROM "foodmart"."product" p4
)
```
## Error before this PR
```text
org.apache.calcite.runtime.CalciteContextException:
From line 12, column 10 to line 12, column 21: Column 'product_id' is ambiguous
```
## Fix
When building GROUP BY keys (and corresponding SELECT keys), qualify one-part identifiers against the SQL join side alias when the aggregate source is a join-shaped input.
## Test
- Added `RelToSqlConverterTest.testPostgresqlRoundTripDistinctLeftJoinInSubqueryWithSemiJoinRules`
- Assert that generated SQL contains `GROUP BY "t2"."product_id"`
- Ran: `./gradlew :core:test --tests org.apache.calcite.rel.rel2sql.RelToSqlConverterTest`
- Result: 563 passed, 0 failed
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4850413126
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with semi-join rewrite [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #4829:
URL: https://github.com/apache/calcite/pull/4829
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with semi-join rewrite [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-5195451844
@caicancai said he will review over the week-end, but he didn't say which week-end.
Promising a review and not delivering is worse than not promising, because no one else will try to review.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with semi-join rewrite [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-5188884819
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [84.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with semi-join rewrite [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4829:
URL: https://github.com/apache/calcite/pull/4829#discussion_r3722854328
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -12711,6 +12758,138 @@ public Sql schema(CalciteAssert.SchemaSpec schemaSpec) {
sql(sql).schema(CalciteAssert.SchemaSpec.JDBC_SCOTT).ok(expected);
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-7439">[CALCITE-7439]
+ * RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with
+ * semi-join rewrite.</a>. */
+ @Test void testPostgresqlRoundTripDistinctLeftJoinInSubqueryWithSemiJoinRules() {
+ final String generated =
+ postgresqlDistinctJoinSql("\"product_id\"", "LEFT", "USING (\"product_id\")");
+ assertPostgresqlSqlValid(generated);
+ }
+
+ @Test void testDistinctRightJoinUsing() {
+ final String generated =
+ postgresqlDistinctJoinSql("\"product_id\"", "RIGHT", "USING (\"product_id\")");
+ assertThat(
+ generated, isLinux("SELECT \"product1\".\"product_id\"\n"
+ + "FROM (SELECT \"$cor0\".\"product_id\", \"t1\".\"EXPR$0\" AS \"mx\"\n"
+ + "FROM \"foodmart\".\"product\" AS \"$cor0\",\n"
+ + "LATERAL (SELECT MAX(\"product_id\") AS \"EXPR$0\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "WHERE \"product_id\" = \"$cor0\".\"product_id\") AS \"t1\") AS \"t2\"\n"
+ + "RIGHT JOIN \"foodmart\".\"product\" AS \"product1\""
+ + " ON \"t2\".\"product_id\" = \"product1\".\"product_id\"\n"
+ + "WHERE EXISTS (SELECT 1\n"
+ + "FROM (SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\") AS \"t3\"\n"
+ + "WHERE \"t2\".\"product_id\" = \"t3\".\"product_id\")\n"
+ + "GROUP BY \"product1\".\"product_id\""));
+ assertPostgresqlSqlValid(generated);
+ }
+
+ @Test void testDistinctFullJoinUsing() {
+ final String generated =
+ postgresqlDistinctJoinSql("\"product_id\"", "FULL", "USING (\"product_id\")");
+ assertThat(generated,
+ isLinux("SELECT COALESCE(\"t2\".\"product_id\","
+ + " \"product1\".\"product_id\") AS \"product_id\"\n"
+ + "FROM (SELECT \"$cor0\".\"product_id\", \"t1\".\"EXPR$0\" AS \"mx\"\n"
+ + "FROM \"foodmart\".\"product\" AS \"$cor0\",\n"
+ + "LATERAL (SELECT MAX(\"product_id\") AS \"EXPR$0\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "WHERE \"product_id\" = \"$cor0\".\"product_id\") AS \"t1\") AS \"t2\"\n"
+ + "FULL JOIN \"foodmart\".\"product\" AS \"product1\""
+ + " ON \"t2\".\"product_id\" = \"product1\".\"product_id\"\n"
+ + "WHERE EXISTS (SELECT 1\n"
+ + "FROM (SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\") AS \"t3\"\n"
+ + "WHERE \"t2\".\"product_id\" = \"t3\".\"product_id\")\n"
+ + "GROUP BY COALESCE(\"t2\".\"product_id\","
+ + " \"product1\".\"product_id\")"));
+ assertPostgresqlSqlValid(generated);
+ }
+
+ @Test void testDistinctFullJoinOnKeepsSelectedSide() {
+ final String condition = "ON pk.\"product_id\" = p2.\"product_id\"";
+ final String generated =
+ postgresqlDistinctJoinSql("pk.\"product_id\"", "FULL", condition);
+ assertThat(
+ generated, isLinux("SELECT \"t2\".\"product_id\"\n"
+ + "FROM (SELECT \"$cor0\".\"product_id\", \"t1\".\"EXPR$0\" AS \"mx\"\n"
+ + "FROM \"foodmart\".\"product\" AS \"$cor0\",\n"
+ + "LATERAL (SELECT MAX(\"product_id\") AS \"EXPR$0\"\n"
+ + "FROM \"foodmart\".\"product\"\n"
+ + "WHERE \"product_id\" = \"$cor0\".\"product_id\") AS \"t1\") AS \"t2\"\n"
+ + "FULL JOIN \"foodmart\".\"product\" AS \"product1\""
+ + " ON \"t2\".\"product_id\" = \"product1\".\"product_id\"\n"
+ + "WHERE EXISTS (SELECT 1\n"
+ + "FROM (SELECT \"product_id\"\n"
+ + "FROM \"foodmart\".\"product\") AS \"t3\"\n"
+ + "WHERE \"t2\".\"product_id\" = \"t3\".\"product_id\")\n"
+ + "GROUP BY \"t2\".\"product_id\""));
+ assertPostgresqlSqlValid(generated);
+ }
+
+ @Test void testDistinctOverSemiJoinAndCorrelate() {
+ final Function<RelBuilder, RelNode> relFn = b -> {
+ final Holder<RexCorrelVariable> v = Holder.empty();
+ b.values(new String[]{"id"}, 1)
+ .variable(v::set);
+ b.values(new String[]{"id"}, 1)
+ .filter(
+ b.equals(b.field("id"),
+ b.getRexBuilder().makeFieldAccess(v.get(), 0)));
+ final RexNode correlateId = b.field(2, 0, 0);
+ b.correlate(JoinRelType.INNER, v.get().id, correlateId);
+ b.values(new String[]{"id"}, 1);
+ final RexNode leftId = b.field(2, 0, 0);
+ return b.join(JoinRelType.SEMI,
+ b.equals(leftId, b.field(2, 1, 0)))
+ .project(leftId)
+ .distinct()
+ .build();
+ };
+ relFn(relFn).ok("SELECT \"$cor0\".\"id\"\n"
Review Comment:
I like "assertPostgresSqlValid", this one is harder to tell by naked eye.
Why do you think this result is fine?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with semi-join rewrite [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-5195437003
I will approve based on the fact tests pass, but this is subtle stuff, so I am not 100% confident. On the other hand, I expect that if this was wrong some other tests would break.
Please fix the conflict.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with semi-join rewrite [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4829:
URL: https://github.com/apache/calcite/pull/4829#discussion_r3715075237
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -942,6 +949,80 @@ private List<SqlNode> generateGroupList(Builder builder,
}
}
+ /** Qualifies a group key when its aggregate input renders as a SQL join
+ * whose columns would otherwise be ambiguous. */
+ private SqlNode maybeQualifyJoinKey(SqlNode field, int key,
+ @Nullable SqlJoin fromJoin, RelNode input) {
+ if (fromJoin == null) {
+ return field;
+ }
+ final @Nullable SqlNode qualified = joinField(input, key, fromJoin);
+ return qualified != null ? qualified : field;
+ }
+
+ /** Resolves a field through row-preserving nodes to the join input that
+ * supplies it. */
+ private static @Nullable SqlNode joinField(RelNode input, int field,
+ SqlJoin fromJoin) {
+ if (input instanceof Project) {
+ final Project project = (Project) input;
+ return joinExpression(project.getInput(), project.getProjects().get(field),
+ fromJoin);
+ }
+ if (input instanceof Filter) {
+ return joinField(((Filter) input).getInput(), field, fromJoin);
+ }
+ if (!(input instanceof Join)) {
Review Comment:
are these all the cases that need to be considered?
How about Correlate?
Or Set operations?
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -12661,6 +12691,176 @@ public Sql schema(CalciteAssert.SchemaSpec schemaSpec) {
sql(sql).schema(CalciteAssert.SchemaSpec.JDBC_SCOTT).ok(expected);
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-7439">[CALCITE-7439]
+ * RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with
+ * semi-join rewrite.</a>. */
+ @Test void testPostgresqlRoundTripDistinctLeftJoinInSubqueryWithSemiJoinRules() {
+ final String query = "WITH product_keys AS (\n"
+ + " SELECT p.\"product_id\",\n"
+ + " (SELECT MAX(p3.\"product_id\")\n"
+ + " FROM \"foodmart\".\"product\" p3\n"
+ + " WHERE p3.\"product_id\" = p.\"product_id\") AS \"mx\"\n"
+ + " FROM \"foodmart\".\"product\" p\n"
+ + ")\n"
+ + "SELECT DISTINCT \"product_id\"\n"
+ + "FROM product_keys pk\n"
+ + "LEFT JOIN \"foodmart\".\"product\" p2 USING (\"product_id\")\n"
+ + "WHERE pk.\"product_id\" IN (\n"
+ + " SELECT p4.\"product_id\"\n"
+ + " FROM \"foodmart\".\"product\" p4\n"
+ + ")";
+
+ final RuleSet rules = semiJoinRules();
+
+ final String generated = sql(query).withPostgresql().optimize(rules, null).exec();
+ assertThat(generated, containsString("GROUP BY \"t2\".\"product_id\""));
Review Comment:
These tests for substrings do not look particularly robust.
Do you find them valuable? Maybe you can add a comment about what they are checking.
Isn't the round-trip test enough?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with semi-join rewrite [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-5182914591
This code is very subtle - as a proof, the fact that it has quite a few issues.
This looks like a strict improvement, so we can perhaps take it even if you cannot answer all my questions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-5175704067
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [93.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4040571151
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [78.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4829:
URL: https://github.com/apache/calcite/pull/4829#discussion_r3190835164
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -12106,6 +12129,211 @@ public Sql schema(CalciteAssert.SchemaSpec schemaSpec) {
sql(sql).schema(CalciteAssert.SchemaSpec.JDBC_SCOTT).ok(expected);
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-7439">[CALCITE-7439]
+ * RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with
+ * semi-join rewrite.</a>. */
+ @Test void testPostgresqlRoundTripDistinctLeftJoinInSubqueryWithSemiJoinRules() {
+ final String query = "WITH product_keys AS (\n"
+ + " SELECT p.\"product_id\",\n"
+ + " (SELECT MAX(p3.\"product_id\")\n"
+ + " FROM \"foodmart\".\"product\" p3\n"
+ + " WHERE p3.\"product_id\" = p.\"product_id\") AS \"mx\"\n"
+ + " FROM \"foodmart\".\"product\" p\n"
+ + ")\n"
+ + "SELECT DISTINCT pk.\"product_id\"\n"
+ + "FROM product_keys pk\n"
+ + "LEFT JOIN \"foodmart\".\"product\" p2 USING (\"product_id\")\n"
+ + "WHERE pk.\"product_id\" IN (\n"
+ + " SELECT p4.\"product_id\"\n"
+ + " FROM \"foodmart\".\"product\" p4\n"
+ + ")";
+
+ final RuleSet rules =
+ RuleSets.ofList(CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE,
+ CoreRules.FILTER_SUB_QUERY_TO_CORRELATE,
+ CoreRules.JOIN_SUB_QUERY_TO_CORRELATE,
+ CoreRules.PROJECT_SUB_QUERY_TO_MARK_CORRELATE,
+ CoreRules.FILTER_SUB_QUERY_TO_MARK_CORRELATE,
+ CoreRules.MARK_TO_SEMI_OR_ANTI_JOIN_RULE,
+ CoreRules.PROJECT_TO_SEMI_JOIN);
+
+ final String generated = sql(query).withPostgresql().optimize(rules, null).exec();
+ assertThat(generated,
+ matchesPattern("(?s).*GROUP BY\\s+\"[^\"]+\"\\.\"product_id\".*"));
+ assertThat(generated, not(containsString("GROUP BY \"product_id\"")));
+ assertPostgresqlSqlValid(generated);
+ }
+
+ @Test void testPostgresqlRoundTripDistinctLeftJoinUsingTwoKeysWithSemiJoinRules() {
+ final String query = "WITH product_keys AS (\n"
+ + " SELECT p.\"product_id\",\n"
+ + " p.\"net_weight\",\n"
+ + " (SELECT MAX(p3.\"product_id\")\n"
+ + " FROM \"foodmart\".\"product\" p3\n"
+ + " WHERE p3.\"product_id\" = p.\"product_id\") AS \"mx\"\n"
+ + " FROM \"foodmart\".\"product\" p\n"
+ + ")\n"
+ + "SELECT DISTINCT pk.\"product_id\", pk.\"net_weight\"\n"
+ + "FROM product_keys pk\n"
+ + "LEFT JOIN \"foodmart\".\"product\" p2 USING (\"product_id\", \"net_weight\")\n"
+ + "WHERE pk.\"product_id\" IN (\n"
+ + " SELECT p4.\"product_id\"\n"
+ + " FROM \"foodmart\".\"product\" p4\n"
+ + ")";
+
+ final RuleSet rules =
+ RuleSets.ofList(CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE,
+ CoreRules.FILTER_SUB_QUERY_TO_CORRELATE,
+ CoreRules.JOIN_SUB_QUERY_TO_CORRELATE,
+ CoreRules.PROJECT_SUB_QUERY_TO_MARK_CORRELATE,
+ CoreRules.FILTER_SUB_QUERY_TO_MARK_CORRELATE,
+ CoreRules.MARK_TO_SEMI_OR_ANTI_JOIN_RULE,
+ CoreRules.PROJECT_TO_SEMI_JOIN);
+
+ final String generated = sql(query).withPostgresql().optimize(rules, null).exec();
+ assertThat(generated,
+ matchesPattern("(?s).*GROUP BY\\s+\"[^\"]+\"\\.\"product_id\",\\s*"
+ + "\"[^\"]+\"\\.\"net_weight\".*"));
+ assertThat(generated,
+ not(containsString("GROUP BY \"product_id\", \"net_weight\"")));
+ assertPostgresqlSqlValid(generated);
+ }
+
+ @Test void testPostgresqlRoundTripDistinctLeftJoinOnEqualKeysWithSemiJoinRules() {
+ final String query = "WITH product_keys AS (\n"
+ + " SELECT p.\"product_id\",\n"
+ + " (SELECT MAX(p3.\"product_id\")\n"
+ + " FROM \"foodmart\".\"product\" p3\n"
+ + " WHERE p3.\"product_id\" = p.\"product_id\") AS \"mx\"\n"
+ + " FROM \"foodmart\".\"product\" p\n"
+ + ")\n"
+ + "SELECT DISTINCT pk.\"product_id\"\n"
+ + "FROM product_keys pk\n"
+ + "LEFT JOIN \"foodmart\".\"product\" p2\n"
+ + " ON pk.\"product_id\" = p2.\"product_id\"\n"
+ + "WHERE pk.\"product_id\" IN (\n"
+ + " SELECT p4.\"product_id\"\n"
+ + " FROM \"foodmart\".\"product\" p4\n"
+ + ")";
+
+ final RuleSet rules =
Review Comment:
can you pull this into a helper function to avoid repeating it?
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -905,6 +916,115 @@ private List<SqlNode> generateGroupList(Builder builder,
}
}
+ private SqlNode maybeQualifyJoinKey(SqlNode field, int key,
+ @Nullable SqlJoin fromJoin, int leftFieldCount) {
+ if (!isSimpleIdentifier(field) || fromJoin == null) {
+ return field;
+ }
+
+ final String fieldName = ((SqlIdentifier) field).getSimple();
+ if (leftFieldCount >= 0) {
+ final SqlNode side = key < leftFieldCount ? fromJoin.getLeft() : fromJoin.getRight();
+ return qualifyJoinField(SqlValidatorUtil.alias(side), fieldName, field);
+ }
+ return maybeQualifyJoinKeyWithoutInputJoin(field, fromJoin, fieldName);
+ }
+
+ private static boolean isSimpleIdentifier(SqlNode node) {
+ return node instanceof SqlIdentifier
+ && ((SqlIdentifier) node).names.size() == 1;
+ }
+
+ private SqlNode maybeQualifyJoinKeyWithoutInputJoin(SqlNode field,
+ SqlJoin fromJoin, String fieldName) {
+ final String leftAlias = SqlValidatorUtil.alias(fromJoin.getLeft());
+ final String rightAlias = SqlValidatorUtil.alias(fromJoin.getRight());
+ if (!isMergedJoinKey(fromJoin, leftAlias, rightAlias, fieldName)) {
+ return field;
+ }
+ switch (fromJoin.getJoinType()) {
+ case RIGHT:
+ return qualifyJoinField(rightAlias, fieldName, field);
+ case FULL:
+ if (leftAlias != null && rightAlias != null) {
+ return SqlStdOperatorTable.COALESCE.createCall(POS,
+ new SqlIdentifier(ImmutableList.of(leftAlias, fieldName), POS),
+ new SqlIdentifier(ImmutableList.of(rightAlias, fieldName), POS));
+ }
+ return qualifyJoinField(leftAlias != null ? leftAlias : rightAlias, fieldName, field);
+ case LEFT:
+ case LEFT_SEMI_JOIN:
+ case LEFT_ANTI_JOIN:
+ case INNER:
+ case CROSS:
+ case COMMA:
+ case ASOF:
+ case LEFT_ASOF:
+ default:
+ return qualifyJoinField(leftAlias, fieldName, field);
+ }
+ }
+
+ private static boolean isMergedJoinKey(SqlJoin fromJoin,
Review Comment:
although these methods are private having some JavaDoc would help maintain them in the future and review them now.
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -905,6 +916,115 @@ private List<SqlNode> generateGroupList(Builder builder,
}
}
+ private SqlNode maybeQualifyJoinKey(SqlNode field, int key,
+ @Nullable SqlJoin fromJoin, int leftFieldCount) {
+ if (!isSimpleIdentifier(field) || fromJoin == null) {
+ return field;
+ }
+
+ final String fieldName = ((SqlIdentifier) field).getSimple();
+ if (leftFieldCount >= 0) {
+ final SqlNode side = key < leftFieldCount ? fromJoin.getLeft() : fromJoin.getRight();
+ return qualifyJoinField(SqlValidatorUtil.alias(side), fieldName, field);
+ }
+ return maybeQualifyJoinKeyWithoutInputJoin(field, fromJoin, fieldName);
+ }
+
+ private static boolean isSimpleIdentifier(SqlNode node) {
+ return node instanceof SqlIdentifier
Review Comment:
SqlIdentifier has a method isSimple
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -905,6 +916,115 @@ private List<SqlNode> generateGroupList(Builder builder,
}
}
+ private SqlNode maybeQualifyJoinKey(SqlNode field, int key,
+ @Nullable SqlJoin fromJoin, int leftFieldCount) {
+ if (!isSimpleIdentifier(field) || fromJoin == null) {
+ return field;
+ }
+
+ final String fieldName = ((SqlIdentifier) field).getSimple();
+ if (leftFieldCount >= 0) {
+ final SqlNode side = key < leftFieldCount ? fromJoin.getLeft() : fromJoin.getRight();
+ return qualifyJoinField(SqlValidatorUtil.alias(side), fieldName, field);
+ }
+ return maybeQualifyJoinKeyWithoutInputJoin(field, fromJoin, fieldName);
+ }
+
+ private static boolean isSimpleIdentifier(SqlNode node) {
+ return node instanceof SqlIdentifier
+ && ((SqlIdentifier) node).names.size() == 1;
+ }
+
+ private SqlNode maybeQualifyJoinKeyWithoutInputJoin(SqlNode field,
+ SqlJoin fromJoin, String fieldName) {
+ final String leftAlias = SqlValidatorUtil.alias(fromJoin.getLeft());
+ final String rightAlias = SqlValidatorUtil.alias(fromJoin.getRight());
+ if (!isMergedJoinKey(fromJoin, leftAlias, rightAlias, fieldName)) {
+ return field;
+ }
+ switch (fromJoin.getJoinType()) {
+ case RIGHT:
+ return qualifyJoinField(rightAlias, fieldName, field);
+ case FULL:
+ if (leftAlias != null && rightAlias != null) {
+ return SqlStdOperatorTable.COALESCE.createCall(POS,
+ new SqlIdentifier(ImmutableList.of(leftAlias, fieldName), POS),
+ new SqlIdentifier(ImmutableList.of(rightAlias, fieldName), POS));
+ }
+ return qualifyJoinField(leftAlias != null ? leftAlias : rightAlias, fieldName, field);
+ case LEFT:
+ case LEFT_SEMI_JOIN:
+ case LEFT_ANTI_JOIN:
+ case INNER:
+ case CROSS:
+ case COMMA:
+ case ASOF:
+ case LEFT_ASOF:
+ default:
+ return qualifyJoinField(leftAlias, fieldName, field);
+ }
+ }
+
+ private static boolean isMergedJoinKey(SqlJoin fromJoin,
+ @Nullable String leftAlias, @Nullable String rightAlias, String fieldName) {
+ final @Nullable SqlNode condition = fromJoin.getCondition();
+ if (fromJoin.getConditionType() == JoinConditionType.USING) {
+ if (!(condition instanceof SqlNodeList)) {
+ return false;
+ }
+ for (SqlNode node : ((SqlNodeList) condition).getList()) {
+ if (node != null
+ && isSimpleIdentifier(node)
+ && fieldName.equals(((SqlIdentifier) node).getSimple())) {
+ return true;
+ }
+ }
+ return false;
+ }
+ return isMergedJoinKeyCondition(condition, leftAlias, rightAlias, fieldName);
+ }
+
+ private static boolean isMergedJoinKeyCondition(@Nullable SqlNode condition,
+ @Nullable String leftAlias, @Nullable String rightAlias, String fieldName) {
+ if (!(condition instanceof SqlCall)) {
+ return false;
+ }
+ final SqlCall call = (SqlCall) condition;
+ switch (call.getKind()) {
+ case AND:
+ return call.getOperandList().stream()
+ .filter(Objects::nonNull)
+ .anyMatch(node ->
+ isMergedJoinKeyCondition(node, leftAlias, rightAlias, fieldName));
+ case EQUALS:
+ return isQualifiedJoinField(call.operand(0), leftAlias, fieldName)
+ && isQualifiedJoinField(call.operand(1), rightAlias, fieldName)
+ || isQualifiedJoinField(call.operand(0), rightAlias, fieldName)
+ && isQualifiedJoinField(call.operand(1), leftAlias, fieldName);
+ default:
+ return false;
+ }
+ }
+
+ private static boolean isQualifiedJoinField(@Nullable SqlNode node,
Review Comment:
I wonder what happens if you have a ROW field and in the join you use a qualified field deeper than 2. Maybe you can add a test case for that?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4032534098
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [78.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4068898098
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [65.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4068415656
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [64.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "bvolpato (via GitHub)" <gi...@apache.org>.
bvolpato commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4378335563
[review-prs-verification]
Head: 9c845171d70859fe27e02ce0517da030377eb972
Ready to merge: yes
Files reviewed.
- core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
- core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
Tests/checks run and results.
- Local: ./gradlew --no-daemon :core:checkstyleMain :core:checkstyleTest :core:compileJava :core:compileTestJava :core:test --tests org.apache.calcite.rel.rel2sql.RelToSqlConverterTest (PASS, BUILD SUCCESSFUL)
- GitHub CI on head 9c845171d70859fe27e02ce0517da030377eb972: all listed checks PASS; Linux (JDK 8) Slow Tests SKIPPED by workflow.
Fixes committed/pushed, if any.
- None.
Final CI status.
- PASS (no failing checks reported).
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4028579900
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [80.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4321432002
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [65.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4321340140
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [65.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4318058244
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with semi-join rewrite [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-5189423488
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [84.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4105408492
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [65.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4031497554
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [76.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4127750332
I will conduct the review over the weekend.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] Qualify GROUP BY keys for DISTINCT over joins [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-4067994417
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [68.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with semi-join rewrite [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4829:
URL: https://github.com/apache/calcite/pull/4829#issuecomment-5195790613
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4829&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4829&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [84.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4829&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4829)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7439] RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with semi-join rewrite [calcite]
Posted by "bvolpato (via GitHub)" <gi...@apache.org>.
bvolpato commented on code in PR #4829:
URL: https://github.com/apache/calcite/pull/4829#discussion_r3712998529
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -905,6 +916,115 @@ private List<SqlNode> generateGroupList(Builder builder,
}
}
+ private SqlNode maybeQualifyJoinKey(SqlNode field, int key,
+ @Nullable SqlJoin fromJoin, int leftFieldCount) {
+ if (!isSimpleIdentifier(field) || fromJoin == null) {
+ return field;
+ }
+
+ final String fieldName = ((SqlIdentifier) field).getSimple();
+ if (leftFieldCount >= 0) {
+ final SqlNode side = key < leftFieldCount ? fromJoin.getLeft() : fromJoin.getRight();
+ return qualifyJoinField(SqlValidatorUtil.alias(side), fieldName, field);
+ }
+ return maybeQualifyJoinKeyWithoutInputJoin(field, fromJoin, fieldName);
+ }
+
+ private static boolean isSimpleIdentifier(SqlNode node) {
+ return node instanceof SqlIdentifier
Review Comment:
removed this heuristic. Group keys now follow relational input lineage, so the `isSimple()` check is no longer needed.
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -12106,6 +12129,211 @@ public Sql schema(CalciteAssert.SchemaSpec schemaSpec) {
sql(sql).schema(CalciteAssert.SchemaSpec.JDBC_SCOTT).ok(expected);
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-7439">[CALCITE-7439]
+ * RelToSqlConverter emits ambiguous GROUP BY after LEFT JOIN USING with
+ * semi-join rewrite.</a>. */
+ @Test void testPostgresqlRoundTripDistinctLeftJoinInSubqueryWithSemiJoinRules() {
+ final String query = "WITH product_keys AS (\n"
+ + " SELECT p.\"product_id\",\n"
+ + " (SELECT MAX(p3.\"product_id\")\n"
+ + " FROM \"foodmart\".\"product\" p3\n"
+ + " WHERE p3.\"product_id\" = p.\"product_id\") AS \"mx\"\n"
+ + " FROM \"foodmart\".\"product\" p\n"
+ + ")\n"
+ + "SELECT DISTINCT pk.\"product_id\"\n"
+ + "FROM product_keys pk\n"
+ + "LEFT JOIN \"foodmart\".\"product\" p2 USING (\"product_id\")\n"
+ + "WHERE pk.\"product_id\" IN (\n"
+ + " SELECT p4.\"product_id\"\n"
+ + " FROM \"foodmart\".\"product\" p4\n"
+ + ")";
+
+ final RuleSet rules =
+ RuleSets.ofList(CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE,
+ CoreRules.FILTER_SUB_QUERY_TO_CORRELATE,
+ CoreRules.JOIN_SUB_QUERY_TO_CORRELATE,
+ CoreRules.PROJECT_SUB_QUERY_TO_MARK_CORRELATE,
+ CoreRules.FILTER_SUB_QUERY_TO_MARK_CORRELATE,
+ CoreRules.MARK_TO_SEMI_OR_ANTI_JOIN_RULE,
+ CoreRules.PROJECT_TO_SEMI_JOIN);
+
+ final String generated = sql(query).withPostgresql().optimize(rules, null).exec();
+ assertThat(generated,
+ matchesPattern("(?s).*GROUP BY\\s+\"[^\"]+\"\\.\"product_id\".*"));
+ assertThat(generated, not(containsString("GROUP BY \"product_id\"")));
+ assertPostgresqlSqlValid(generated);
+ }
+
+ @Test void testPostgresqlRoundTripDistinctLeftJoinUsingTwoKeysWithSemiJoinRules() {
+ final String query = "WITH product_keys AS (\n"
+ + " SELECT p.\"product_id\",\n"
+ + " p.\"net_weight\",\n"
+ + " (SELECT MAX(p3.\"product_id\")\n"
+ + " FROM \"foodmart\".\"product\" p3\n"
+ + " WHERE p3.\"product_id\" = p.\"product_id\") AS \"mx\"\n"
+ + " FROM \"foodmart\".\"product\" p\n"
+ + ")\n"
+ + "SELECT DISTINCT pk.\"product_id\", pk.\"net_weight\"\n"
+ + "FROM product_keys pk\n"
+ + "LEFT JOIN \"foodmart\".\"product\" p2 USING (\"product_id\", \"net_weight\")\n"
+ + "WHERE pk.\"product_id\" IN (\n"
+ + " SELECT p4.\"product_id\"\n"
+ + " FROM \"foodmart\".\"product\" p4\n"
+ + ")";
+
+ final RuleSet rules =
+ RuleSets.ofList(CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE,
+ CoreRules.FILTER_SUB_QUERY_TO_CORRELATE,
+ CoreRules.JOIN_SUB_QUERY_TO_CORRELATE,
+ CoreRules.PROJECT_SUB_QUERY_TO_MARK_CORRELATE,
+ CoreRules.FILTER_SUB_QUERY_TO_MARK_CORRELATE,
+ CoreRules.MARK_TO_SEMI_OR_ANTI_JOIN_RULE,
+ CoreRules.PROJECT_TO_SEMI_JOIN);
+
+ final String generated = sql(query).withPostgresql().optimize(rules, null).exec();
+ assertThat(generated,
+ matchesPattern("(?s).*GROUP BY\\s+\"[^\"]+\"\\.\"product_id\",\\s*"
+ + "\"[^\"]+\"\\.\"net_weight\".*"));
+ assertThat(generated,
+ not(containsString("GROUP BY \"product_id\", \"net_weight\"")));
+ assertPostgresqlSqlValid(generated);
+ }
+
+ @Test void testPostgresqlRoundTripDistinctLeftJoinOnEqualKeysWithSemiJoinRules() {
+ final String query = "WITH product_keys AS (\n"
+ + " SELECT p.\"product_id\",\n"
+ + " (SELECT MAX(p3.\"product_id\")\n"
+ + " FROM \"foodmart\".\"product\" p3\n"
+ + " WHERE p3.\"product_id\" = p.\"product_id\") AS \"mx\"\n"
+ + " FROM \"foodmart\".\"product\" p\n"
+ + ")\n"
+ + "SELECT DISTINCT pk.\"product_id\"\n"
+ + "FROM product_keys pk\n"
+ + "LEFT JOIN \"foodmart\".\"product\" p2\n"
+ + " ON pk.\"product_id\" = p2.\"product_id\"\n"
+ + "WHERE pk.\"product_id\" IN (\n"
+ + " SELECT p4.\"product_id\"\n"
+ + " FROM \"foodmart\".\"product\" p4\n"
+ + ")";
+
+ final RuleSet rules =
Review Comment:
fixed. The repeated rule setup now lives in `semiJoinRules()`.
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -905,6 +916,115 @@ private List<SqlNode> generateGroupList(Builder builder,
}
}
+ private SqlNode maybeQualifyJoinKey(SqlNode field, int key,
+ @Nullable SqlJoin fromJoin, int leftFieldCount) {
+ if (!isSimpleIdentifier(field) || fromJoin == null) {
+ return field;
+ }
+
+ final String fieldName = ((SqlIdentifier) field).getSimple();
+ if (leftFieldCount >= 0) {
+ final SqlNode side = key < leftFieldCount ? fromJoin.getLeft() : fromJoin.getRight();
+ return qualifyJoinField(SqlValidatorUtil.alias(side), fieldName, field);
+ }
+ return maybeQualifyJoinKeyWithoutInputJoin(field, fromJoin, fieldName);
+ }
+
+ private static boolean isSimpleIdentifier(SqlNode node) {
+ return node instanceof SqlIdentifier
+ && ((SqlIdentifier) node).names.size() == 1;
+ }
+
+ private SqlNode maybeQualifyJoinKeyWithoutInputJoin(SqlNode field,
+ SqlJoin fromJoin, String fieldName) {
+ final String leftAlias = SqlValidatorUtil.alias(fromJoin.getLeft());
+ final String rightAlias = SqlValidatorUtil.alias(fromJoin.getRight());
+ if (!isMergedJoinKey(fromJoin, leftAlias, rightAlias, fieldName)) {
+ return field;
+ }
+ switch (fromJoin.getJoinType()) {
+ case RIGHT:
+ return qualifyJoinField(rightAlias, fieldName, field);
+ case FULL:
+ if (leftAlias != null && rightAlias != null) {
+ return SqlStdOperatorTable.COALESCE.createCall(POS,
+ new SqlIdentifier(ImmutableList.of(leftAlias, fieldName), POS),
+ new SqlIdentifier(ImmutableList.of(rightAlias, fieldName), POS));
+ }
+ return qualifyJoinField(leftAlias != null ? leftAlias : rightAlias, fieldName, field);
+ case LEFT:
+ case LEFT_SEMI_JOIN:
+ case LEFT_ANTI_JOIN:
+ case INNER:
+ case CROSS:
+ case COMMA:
+ case ASOF:
+ case LEFT_ASOF:
+ default:
+ return qualifyJoinField(leftAlias, fieldName, field);
+ }
+ }
+
+ private static boolean isMergedJoinKey(SqlJoin fromJoin,
Review Comment:
added JavaDocs for the lineage helpers.
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -905,6 +916,115 @@ private List<SqlNode> generateGroupList(Builder builder,
}
}
+ private SqlNode maybeQualifyJoinKey(SqlNode field, int key,
+ @Nullable SqlJoin fromJoin, int leftFieldCount) {
+ if (!isSimpleIdentifier(field) || fromJoin == null) {
+ return field;
+ }
+
+ final String fieldName = ((SqlIdentifier) field).getSimple();
+ if (leftFieldCount >= 0) {
+ final SqlNode side = key < leftFieldCount ? fromJoin.getLeft() : fromJoin.getRight();
+ return qualifyJoinField(SqlValidatorUtil.alias(side), fieldName, field);
+ }
+ return maybeQualifyJoinKeyWithoutInputJoin(field, fromJoin, fieldName);
+ }
+
+ private static boolean isSimpleIdentifier(SqlNode node) {
+ return node instanceof SqlIdentifier
+ && ((SqlIdentifier) node).names.size() == 1;
+ }
+
+ private SqlNode maybeQualifyJoinKeyWithoutInputJoin(SqlNode field,
+ SqlJoin fromJoin, String fieldName) {
+ final String leftAlias = SqlValidatorUtil.alias(fromJoin.getLeft());
+ final String rightAlias = SqlValidatorUtil.alias(fromJoin.getRight());
+ if (!isMergedJoinKey(fromJoin, leftAlias, rightAlias, fieldName)) {
+ return field;
+ }
+ switch (fromJoin.getJoinType()) {
+ case RIGHT:
+ return qualifyJoinField(rightAlias, fieldName, field);
+ case FULL:
+ if (leftAlias != null && rightAlias != null) {
+ return SqlStdOperatorTable.COALESCE.createCall(POS,
+ new SqlIdentifier(ImmutableList.of(leftAlias, fieldName), POS),
+ new SqlIdentifier(ImmutableList.of(rightAlias, fieldName), POS));
+ }
+ return qualifyJoinField(leftAlias != null ? leftAlias : rightAlias, fieldName, field);
+ case LEFT:
+ case LEFT_SEMI_JOIN:
+ case LEFT_ANTI_JOIN:
+ case INNER:
+ case CROSS:
+ case COMMA:
+ case ASOF:
+ case LEFT_ASOF:
+ default:
+ return qualifyJoinField(leftAlias, fieldName, field);
+ }
+ }
+
+ private static boolean isMergedJoinKey(SqlJoin fromJoin,
+ @Nullable String leftAlias, @Nullable String rightAlias, String fieldName) {
+ final @Nullable SqlNode condition = fromJoin.getCondition();
+ if (fromJoin.getConditionType() == JoinConditionType.USING) {
+ if (!(condition instanceof SqlNodeList)) {
+ return false;
+ }
+ for (SqlNode node : ((SqlNodeList) condition).getList()) {
+ if (node != null
+ && isSimpleIdentifier(node)
+ && fieldName.equals(((SqlIdentifier) node).getSimple())) {
+ return true;
+ }
+ }
+ return false;
+ }
+ return isMergedJoinKeyCondition(condition, leftAlias, rightAlias, fieldName);
+ }
+
+ private static boolean isMergedJoinKeyCondition(@Nullable SqlNode condition,
+ @Nullable String leftAlias, @Nullable String rightAlias, String fieldName) {
+ if (!(condition instanceof SqlCall)) {
+ return false;
+ }
+ final SqlCall call = (SqlCall) condition;
+ switch (call.getKind()) {
+ case AND:
+ return call.getOperandList().stream()
+ .filter(Objects::nonNull)
+ .anyMatch(node ->
+ isMergedJoinKeyCondition(node, leftAlias, rightAlias, fieldName));
+ case EQUALS:
+ return isQualifiedJoinField(call.operand(0), leftAlias, fieldName)
+ && isQualifiedJoinField(call.operand(1), rightAlias, fieldName)
+ || isQualifiedJoinField(call.operand(0), rightAlias, fieldName)
+ && isQualifiedJoinField(call.operand(1), leftAlias, fieldName);
+ default:
+ return false;
+ }
+ }
+
+ private static boolean isQualifiedJoinField(@Nullable SqlNode node,
Review Comment:
added `testDistinctJoinOnNestedField`, covering a four-part qualified field through nested `ROW` values.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org