[GitHub] [calcite] zabetak commented on a diff in pull request #2813: [CALCITE-5127] Error when executing query with subquery in select lis…

[GitBox <gi...@apache.org>]

zabetak commented on code in PR #2813:
URL: https://github.com/apache/calcite/pull/2813#discussion_r963398493


##########
core/src/main/java/org/apache/calcite/rel/core/Project.java:
##########
@@ -137,20 +163,27 @@ protected Project(RelInput input) {
    * @param input Input
    * @param projects Project expressions
    * @param rowType Output row type
+   * @param variableSet The variable set.
    * @return New {@code Project} if any parameter differs from the value of this
    *   {@code Project}, or just {@code this} if all the parameters are
    *   the same
    *
    * @see #copy(RelTraitSet, List)
    */
   public abstract Project copy(RelTraitSet traitSet, RelNode input,
-      List<RexNode> projects, RelDataType rowType);
+      List<RexNode> projects, RelDataType rowType, Set<CorrelationId> variableSet);
+
+  @Deprecated // to be removed before 2.0
+  public Project copy(RelTraitSet traitSet, RelNode input,
+      List<RexNode> projects, RelDataType rowType) {
+    return copy(traitSet, input, projects, rowType, ImmutableSet.of());
+  }

Review Comment:
   At the moment we are not using anywhere the new copy method with the extra argument (i.e., `variableSet`) so maybe we can revert this change for now.
   
   This is a breaking change since everybody extending `Project` will have to adapt their code in order the project to compile thus it would be better to avoid it if it is not an absolute must.
   
   If in the future we find out that we need to copy a projection and change the variables then we can introduce the extra argument.



##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -1817,7 +1817,24 @@ public RelBuilder project(Iterable<? extends RexNode> nodes,
    */
   public RelBuilder project(Iterable<? extends RexNode> nodes,
       Iterable<? extends @Nullable String> fieldNames, boolean force) {
-    return project_(nodes, fieldNames, ImmutableList.of(), force);
+    return project(nodes, fieldNames, force, ImmutableSet.of());
+  }
+
+  /**
+   * The same with {@link #project(Iterable, Iterable, boolean)}, with additional
+   * variablesSet param.
+   *
+   * @param nodes Expressions
+   * @param fieldNames Suggested field names
+   * @param force create project even if it is identity
+   * @param variablesSet Correlating variables that are set when reading a row
+   *                     from the input, and which may be referenced from the
+   *                     projection expressions
+   */
+  public RelBuilder project(Iterable<? extends RexNode> nodes,
+      Iterable<? extends @Nullable String> fieldNames, boolean force,
+      Iterable<CorrelationId> variablesSet) {
+    return project_(nodes, fieldNames, ImmutableList.of(), force, variablesSet);

Review Comment:
   I am again skeptical about the decision to introduce `variablesSet` in the project in particular due to the second point you mention above. I added some relevant comments under the JIRA case. We can come back to this if what I wrote does not make sense.



##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -1908,9 +1927,11 @@ private RelBuilder project_(
       fieldNameList.add(null);
     }
 
+    // Do not merge projection when top projection has correlation variables
     bloat:
     if (frame.rel instanceof Project
-        && config.bloat() >= 0) {
+        && config.bloat() >= 0
+        && variables.isEmpty()) {

Review Comment:
   Based on my latest comments under the JIRA about not adding variablesSet in `Project`, I was thinking that something like the snippet below could be used here and similarly in other places.
   
   ```java
   if (StreamSupport.stream(nodes.spliterator(), false).anyMatch(RexUtil::containsCorrelation)) {
     break bloat;
   }
   ```



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