vlsi commented on a change in pull request #2623:
URL: https://github.com/apache/calcite/pull/2623#discussion_r768079938
##########
File path: core/src/main/java/org/apache/calcite/rel/core/Project.java
##########
@@ -89,25 +94,33 @@ protected Project(
List<RelHint> hints,
RelNode input,
List<? extends RexNode> projects,
- RelDataType rowType) {
+ RelDataType rowType,
+ Set<CorrelationId> variablesSet) {
super(cluster, traits, input);
assert rowType != null;
this.exps = ImmutableList.copyOf(projects);
this.hints = ImmutableList.copyOf(hints);
this.rowType = rowType;
+ this.variablesSet = ImmutableSet.copyOf(
+ Objects.requireNonNull(variablesSet, "variablesSet"));
assert isValid(Litmus.THROW, null);
}
+ protected Project(RelOptCluster cluster, RelTraitSet traits, List<RelHint>
hints,
+ RelNode input, List<? extends RexNode> projects, RelDataType rowType) {
+ this(cluster, traits, hints, input, projects, rowType, ImmutableSet.of());
+ }
Review comment:
In practice, this change breaks backward compatibility: you add a
"convenience" constructor, and now the clients do not know they **must**
somehow handle the new `variablesSet` parameter (e.g. by throwing on non-empty
set)
I would suggest the method to be removed or at least marked with
`@Deprecated` so the clients know which places to fix.
--
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]
