You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "normanj-bitquill (via GitHub)" <gi...@apache.org> on 2024/03/18 17:36:00 UTC
[PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
normanj-bitquill opened a new pull request, #3736:
URL: https://github.com/apache/calcite/pull/3736
(no comment)
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "normanj-bitquill (via GitHub)" <gi...@apache.org>.
normanj-bitquill commented on code in PR #3736:
URL: https://github.com/apache/calcite/pull/3736#discussion_r1530716832
##########
core/src/main/java/org/apache/calcite/rel/core/Window.java:
##########
@@ -111,6 +111,14 @@ public Window(RelOptCluster cluster, RelTraitSet traitSet, RelNode input,
this(cluster, traitSet, Collections.emptyList(), input, constants, rowType, groups);
}
+ /**
+ * Creates a copy of this {@code Window}.
+ *
+ * @param constants List of constants that are additional inputs
+ * @return New {@code Window}
+ */
+ public abstract Window copy(List<RexLiteral> constants);
Review 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?
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "normanj-bitquill (via GitHub)" <gi...@apache.org>.
normanj-bitquill commented on code in PR #3736:
URL: https://github.com/apache/calcite/pull/3736#discussion_r1530808516
##########
core/src/main/java/org/apache/calcite/rel/core/Window.java:
##########
@@ -111,6 +111,14 @@ public Window(RelOptCluster cluster, RelTraitSet traitSet, RelNode input,
this(cluster, traitSet, Collections.emptyList(), input, constants, rowType, groups);
}
+ /**
+ * Creates a copy of this {@code Window}.
+ *
+ * @param constants List of constants that are additional inputs
+ * @return New {@code Window}
+ */
+ public abstract Window copy(List<RexLiteral> constants);
Review Comment:
I've updated the Javadoc comment to better describe how `constants` is used.
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3736:
URL: https://github.com/apache/calcite/pull/3736#issuecomment-2007784236
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3736) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3736&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3736&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3736&resolved=false&inNewCodePeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3736&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3736&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3736)
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "normanj-bitquill (via GitHub)" <gi...@apache.org>.
normanj-bitquill commented on code in PR #3736:
URL: https://github.com/apache/calcite/pull/3736#discussion_r1530824635
##########
core/src/main/java/org/apache/calcite/rel/core/Window.java:
##########
@@ -111,6 +111,14 @@ public Window(RelOptCluster cluster, RelTraitSet traitSet, RelNode input,
this(cluster, traitSet, Collections.emptyList(), input, constants, rowType, groups);
}
+ /**
+ * Creates a copy of this {@code Window}.
+ *
+ * @param constants List of constants that are additional inputs
+ * @return New {@code Window}
+ */
+ public abstract Window copy(List<RexLiteral> constants);
Review Comment:
I've added a comment to the API changes for 1.37.0 in the `history.md` file.
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3736:
URL: https://github.com/apache/calcite/pull/3736#issuecomment-2030169740
I see that the branch has conflicts, probably due to me merging a PR just a few minutes ago.
Can you please rebase again on main?
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3736:
URL: https://github.com/apache/calcite/pull/3736#discussion_r1530147098
##########
core/src/main/java/org/apache/calcite/rel/core/Window.java:
##########
@@ -111,6 +111,14 @@ public Window(RelOptCluster cluster, RelTraitSet traitSet, RelNode input,
this(cluster, traitSet, Collections.emptyList(), input, constants, rowType, groups);
}
+ /**
+ * Creates a copy of this {@code Window}.
+ *
+ * @param constants List of constants that are additional inputs
+ * @return New {@code Window}
+ */
+ public abstract Window copy(List<RexLiteral> constants);
Review 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).
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3736:
URL: https://github.com/apache/calcite/pull/3736#issuecomment-2030271330
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3736) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3736&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3736&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3736&resolved=false&inNewCodePeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3736&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3736&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3736)
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3736:
URL: https://github.com/apache/calcite/pull/3736#discussion_r1529445048
##########
core/src/main/java/org/apache/calcite/rel/core/Window.java:
##########
@@ -111,6 +111,14 @@ public Window(RelOptCluster cluster, RelTraitSet traitSet, RelNode input,
this(cluster, traitSet, Collections.emptyList(), input, constants, rowType, groups);
}
+ /**
+ * Creates a copy of this {@code Window}.
+ *
+ * @param constants List of constants that are additional inputs
+ * @return New {@code Window}
+ */
+ public abstract Window copy(List<RexLiteral> constants);
Review 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.
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3736:
URL: https://github.com/apache/calcite/pull/3736#discussion_r1530774009
##########
core/src/main/java/org/apache/calcite/rel/core/Window.java:
##########
@@ -111,6 +111,14 @@ public Window(RelOptCluster cluster, RelTraitSet traitSet, RelNode input,
this(cluster, traitSet, Collections.emptyList(), input, constants, rowType, groups);
}
+ /**
+ * Creates a copy of this {@code Window}.
+ *
+ * @param constants List of constants that are additional inputs
+ * @return New {@code Window}
+ */
+ public abstract Window copy(List<RexLiteral> constants);
Review Comment:
if that's the convention you can keep it this way.
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3736:
URL: https://github.com/apache/calcite/pull/3736
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3736:
URL: https://github.com/apache/calcite/pull/3736#discussion_r1538908575
##########
site/_docs/history.md:
##########
@@ -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
Review 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".
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "normanj-bitquill (via GitHub)" <gi...@apache.org>.
normanj-bitquill commented on code in PR #3736:
URL: https://github.com/apache/calcite/pull/3736#discussion_r1546600583
##########
site/_docs/history.md:
##########
@@ -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
Review Comment:
That makes sense. I have moved this into the breaking changes section.
--
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
Re: [PR] [CALCITE-6321] Add copy(List) method to Window class [calcite]
Posted by "normanj-bitquill (via GitHub)" <gi...@apache.org>.
normanj-bitquill commented on PR #3736:
URL: https://github.com/apache/calcite/pull/3736#issuecomment-2030206018
@mihaibudiu I have rebased and fixed the conflict.
--
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