You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/07/17 08:01:57 UTC
[GitHub] [calcite] rubenada opened a new pull request #2071: [CALCITE-4127] Remove final from AbstractRelNode#getRelTypeName
rubenada opened a new pull request #2071:
URL: https://github.com/apache/calcite/pull/2071
Remove final from AbstractRelNode#getRelTypeName
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on pull request #2071: [CALCITE-4127] Remove final from AbstractRelNode#getRelTypeName
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #2071:
URL: https://github.com/apache/calcite/pull/2071#issuecomment-660242167
@rubenada I know the release manager is preparing the release for 1.24.0, please allow me to take the liberty to merge this pull request for you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2071: [CALCITE-4127] Remove final from AbstractRelNode#getRelTypeName
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2071:
URL: https://github.com/apache/calcite/pull/2071#discussion_r456285940
##########
File path: core/src/main/java/org/apache/calcite/rel/AbstractRelNode.java
##########
@@ -177,7 +177,8 @@ public void register(RelOptPlanner planner) {
Util.discard(planner);
}
- public final String getRelTypeName() {
+ // It is not recommended to override this method, but sub-classes can do it at their own risk.
+ public String getRelTypeName() {
Review comment:
It would be cool if we can also describe a little what the original purpose of this method too ~
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2071: [CALCITE-4127] Remove final from AbstractRelNode#getRelTypeName
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2071:
URL: https://github.com/apache/calcite/pull/2071#discussion_r456315360
##########
File path: core/src/main/java/org/apache/calcite/rel/AbstractRelNode.java
##########
@@ -177,7 +177,8 @@ public void register(RelOptPlanner planner) {
Util.discard(planner);
}
- public final String getRelTypeName() {
+ // It is not recommended to override this method, but sub-classes can do it at their own risk.
+ public String getRelTypeName() {
Review comment:
The original javadoc is the one from the interface (RelNode), which is a contract, so I think its documentation is still valid. Overridden implementations must respect this contract, but they can implement it in a different way, e.g. returning a String literal with the class name, instead of using the default mechanism of AbstractRelNode based on getClass().getName()
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #2071: [CALCITE-4127] Remove final from AbstractRelNode#getRelTypeName
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #2071:
URL: https://github.com/apache/calcite/pull/2071#discussion_r456468071
##########
File path: core/src/main/java/org/apache/calcite/rel/AbstractRelNode.java
##########
@@ -177,7 +177,8 @@ public void register(RelOptPlanner planner) {
Util.discard(planner);
}
- public final String getRelTypeName() {
+ // It is not recommended to override this method, but sub-classes can do it at their own risk.
Review comment:
nit: It might be better to 2 lines.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2071: [CALCITE-4127] Remove final from AbstractRelNode#getRelTypeName
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2071:
URL: https://github.com/apache/calcite/pull/2071#discussion_r456297775
##########
File path: core/src/main/java/org/apache/calcite/rel/AbstractRelNode.java
##########
@@ -177,7 +177,8 @@ public void register(RelOptPlanner planner) {
Util.discard(planner);
}
- public final String getRelTypeName() {
+ // It is not recommended to override this method, but sub-classes can do it at their own risk.
+ public String getRelTypeName() {
Review comment:
I think there is a very complete description in the [javadoc of the interface RelNode](https://github.com/apache/calcite/blob/a4fa05458840cfdd93fb5cba16d102b06197539d/core/src/main/java/org/apache/calcite/rel/RelNode.java#L367); we could copy-paste it in here, but I think it would be redundant and less simple to maintain.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2071: [CALCITE-4127] Remove final from AbstractRelNode#getRelTypeName
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2071:
URL: https://github.com/apache/calcite/pull/2071#issuecomment-660243092
Of course @hsyuan , thanks for taking care of this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2071: [CALCITE-4127] Remove final from AbstractRelNode#getRelTypeName
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2071:
URL: https://github.com/apache/calcite/pull/2071#discussion_r456308915
##########
File path: core/src/main/java/org/apache/calcite/rel/AbstractRelNode.java
##########
@@ -177,7 +177,8 @@ public void register(RelOptPlanner planner) {
Util.discard(planner);
}
- public final String getRelTypeName() {
+ // It is not recommended to override this method, but sub-classes can do it at their own risk.
+ public String getRelTypeName() {
Review comment:
The original document is not suitable now because it describes the default behavior, one can override it to a different one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan merged pull request #2071: [CALCITE-4127] Remove final from AbstractRelNode#getRelTypeName
Posted by GitBox <gi...@apache.org>.
hsyuan merged pull request #2071:
URL: https://github.com/apache/calcite/pull/2071
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org