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 2021/05/09 14:33:00 UTC
[GitHub] [calcite] tisonkun opened a new pull request #2411: Add generic info to Map & Array annotation
tisonkun opened a new pull request #2411:
URL: https://github.com/apache/calcite/pull/2411
Signed-off-by: tison <wa...@gmail.com>
straightforward
--
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 #2411: Add generic info to Map & Array annotation
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2411:
URL: https://github.com/apache/calcite/pull/2411#issuecomment-848536331
Thanks @tisonkun for the patch and for the detailed explanation!
--
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] zabetak commented on pull request #2411: Add generic info to Map & Array annotation
Posted by GitBox <gi...@apache.org>.
zabetak commented on pull request #2411:
URL: https://github.com/apache/calcite/pull/2411#issuecomment-847821775
@rubenada changes with small impact to end-users can go in without JIRA ;)
--
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] tisonkun commented on pull request #2411: Add generic info to Map & Array annotation
Posted by GitBox <gi...@apache.org>.
tisonkun commented on pull request #2411:
URL: https://github.com/apache/calcite/pull/2411#issuecomment-848410856
@rubenada @zabetak @amaliujia thanks for your time!
> Just out of curiosity: what is the benefit to have generic info?
@amaliujia I think adding generic parameter when there should be one is a general best practice. It is useful as linter in this case.
I created this pull request on a day reading the source code and linter complain. The community may or may not reject such pr and require to be included in a larger one, but I prefer we accept valid contributions at any level. If you turn a blind eye to generic parameter missing, it is possible we run into a case where Apache Hudi is, that it is hard to fix missing generic parameters that really hurt.
--
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] tisonkun edited a comment on pull request #2411: Add generic info to Map & Array annotation
Posted by GitBox <gi...@apache.org>.
tisonkun edited a comment on pull request #2411:
URL: https://github.com/apache/calcite/pull/2411#issuecomment-848410856
@rubenada @zabetak @amaliujia thanks for your time!
> Just out of curiosity: what is the benefit to have generic info?
@amaliujia I think adding generic parameter when there should be one is a general best practice. It is useful as linter in this case.
I created this pull request on a day reading the source code and linter complain. The community may or may not reject such pr and require to be included in a larger one, but I prefer we accept valid contributions at any level. If you turn a blind eye to generic parameter missing, it is possible we are gonna run into a case where Apache Hudi is, that it is hard to fix missing generic parameters that really hurt.
--
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] amaliujia commented on a change in pull request #2411: Add generic info to Map & Array annotation
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2411:
URL: https://github.com/apache/calcite/pull/2411#discussion_r639136087
##########
File path: core/src/main/java/org/apache/calcite/adapter/java/Map.java
##########
@@ -16,23 +16,22 @@
*/
package org.apache.calcite.adapter.java;
+import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
-import static java.lang.annotation.ElementType.FIELD;
-
/**
* Annotation that indicates that a field is a map type.
*/
-@Target({FIELD })
+@Target(ElementType.FIELD)
@Retention(RetentionPolicy.RUNTIME)
public @interface Map {
/** Key type. */
- Class key();
+ Class<?> key();
Review comment:
What is changed after having `Class<?>`?
--
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 #2411: Add generic info to Map & Array annotation
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2411:
URL: https://github.com/apache/calcite/pull/2411#issuecomment-847740202
This patch could be merged, but there is no associated Jira. Since it is a small change, perhaps we could make an exception, what do you think @zabetak ?
--
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] amaliujia commented on pull request #2411: Add generic info to Map & Array annotation
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2411:
URL: https://github.com/apache/calcite/pull/2411#issuecomment-848213486
Just out of curiosity: what is the benefit to have generic info?
--
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 #2411: Add generic info to Map & Array annotation
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2411:
URL: https://github.com/apache/calcite/pull/2411#issuecomment-847844526
Thanks for the confirmation @zabetak
--
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 merged pull request #2411: Add generic info to Map & Array annotation
Posted by GitBox <gi...@apache.org>.
rubenada merged pull request #2411:
URL: https://github.com/apache/calcite/pull/2411
--
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