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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
