zabetak commented on code in PR #2911:
URL: https://github.com/apache/calcite/pull/2911#discussion_r974202268
##########
core/src/main/java/org/apache/calcite/rex/RexUtil.java:
##########
@@ -2232,6 +2232,16 @@ public static boolean containsCorrelation(RexNode
condition) {
}
}
+ /** Returns whether an expression contains a {@link RexSubQuery}. */
+ public static boolean containsSubQuery(RexNode node) {
Review Comment:
Maybe change the method to accept as a parameter an `Iterable<RexNode>`
since it will be more convenient for existing callers and it will slightly
reduce duplicate fragments.
##########
core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java:
##########
@@ -3383,6 +3383,16 @@ void checkCorrelatedMapSubQuery(boolean expand) {
sql(sql).withDecorrelate(true).ok();
}
+ /**
+ * Test case for <a
href="https://issues.apache.org/jira/browse/CALCITE-5127";>[CALCITE-5127]
+ * Error when executing query with subquery in select list that uses outer
column of array
+ * type</a>.
+ */
+ @Test void testCorrelationWithProjection() {
Review Comment:
Possibly more descriptive names for the test case:
* `testNotMergeProjectionsWhenCorrelationPresent`
* `testProjectionWithCorrelationNoMerge`
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -1908,9 +1908,19 @@ private RelBuilder project_(
fieldNameList.add(null);
}
+ // Do not merge projections when top project contains RexSubQuery
+ boolean containsSubQuery = false;
+ for (RexNode node : nodes) {
+ if (RexUtil.containsSubQuery(node)) {
+ containsSubQuery = true;
+ break;
+ }
+ }
+
Review Comment:
Also this is a breaking change since it changes the old behavior. It fixes a
bug but at the same time prevents some optimizations to take effect. I think it
is worth adding a few lines explaining the change in behavior in the respective
section in `history.md`.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelFieldTrimmer.java:
##########
@@ -481,6 +481,18 @@ public TrimResult trimFields(
final int fieldCount = rowType.getFieldCount();
final RelNode input = project.getInput();
+ boolean containsSubQuery = false;
+ for (RexNode node : project.getProjects()) {
+ if (RexUtil.containsSubQuery(node)) {
+ containsSubQuery = true;
+ break;
+ }
+ }
+ // Do not trim Project's fields before SubQueryRemoveRule applies.
+ if (containsSubQuery) {
+ return result(project, Mappings.createIdentity(fieldCount));
+ }
+
Review Comment:
Does this code has test coverage? I don't see any plan changes due to this.
Also the effects of this change are much broader than the changes in
`RelBuilder` thus I would suggest to treat this separately in another JIRA.
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -1908,9 +1908,19 @@ private RelBuilder project_(
fieldNameList.add(null);
}
+ // Do not merge projections when top project contains RexSubQuery
+ boolean containsSubQuery = false;
+ for (RexNode node : nodes) {
+ if (RexUtil.containsSubQuery(node)) {
+ containsSubQuery = true;
+ break;
+ }
+ }
+
Review Comment:
I would move the check inside the if block (maybe after line 1934/1944) so
that we don't spend time looking for subqueries if we don't need to.
--
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]
