[CALCITE-6321] Add copy(List<RexLiteral>) method to Window class #3736
Conversation
| * @param constants List of constants that are additional inputs | ||
| * @return New {@code Window} | ||
| */ | ||
| public abstract Window copy(List<RexLiteral> constants); |
There was a problem hiding this comment.
I would actually call this method withConstants, in line with other naming conventions in Calcite.
I find the JavaDoc confusing, because these are not additional inputs, these replace the existing constants of the window.
There was a problem hiding this comment.
I agree with the confusing javadoc.
I have no strong opinion on the method name, but I have the impression that, in terms of RelNode "operators", in other similar cases we have named them copy .
Independently of the name, we could consider this a "breaking change" (any downstream project extending Window will be "broken" on the next Calcite upgrade, and will require a minor adjustment to provide the implementation of this new abstract method). So I'd propose to add the corresponding comment in history.md (in the commented out "breaking changes" section for next release).
There was a problem hiding this comment.
I can rename this, but copy is also used like this in other classes.
https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/rel/core/Filter.java#L125
https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/rel/logical/LogicalFilter.java#L148
Should I still rename this method?
There was a problem hiding this comment.
if that's the convention you can keep it this way.
There was a problem hiding this comment.
I've updated the Javadoc comment to better describe how constants is used.
There was a problem hiding this comment.
I've added a comment to the API changes for 1.37.0 in the history.md file.
normanj-bitquill
force-pushed
the
calcite-6321
branch
2 times, most recently
from
a0fb49e
to
f13484c
Compare
site/_docs/history.md
Outdated
| @@ -57,6 +57,9 @@ other software versions as specified in gradle.properties. | |||
| #### Bug-fixes, API changes and minor enhancements | |||
| {: #fixes-1-37-0} | |||
|
|
|||
| * [<a href="https://issues.apache.org/jira/browse/CALCITE-6321">CALCITE-6321</a>] | |||
| Add `copy(List<RexLiteral>)` method to `Window` class | |||
There was a problem hiding this comment.
IMO, it would be better to move this into the "Breaking Changes" section, and add a bit more info about the impact, e.g.: "any subclass of Window will need to implement this new method".
There was a problem hiding this comment.
That makes sense. I have moved this into the breaking changes section.
There was a problem hiding this comment.
The code looks fine.
Is there any existing part of Calcite that benefits from this additional capability that could be highlighted in a test?
@rubenada suggested a change for the history file, once that is done this can be merged from my pov.
normanj-bitquill
force-pushed
the
calcite-6321
branch
from
f13484c
to
5450c75
Compare
|
I see that the branch has conflicts, probably due to me merging a PR just a few minutes ago. |
mihaibudiu
added
the
LGTM-will-merge-soon
normanj-bitquill
force-pushed
the
calcite-6321
branch
from
5450c75
to
d90ab36
Compare
|
@mihaibudiu I have rebased and fixed the conflict. |
|
No description provided.