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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
