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/11/19 22:11:53 UTC
[GitHub] [calcite] vlsi opened a new pull request #2273: [CALCITE-4199] review-only: nullability annotations
vlsi opened a new pull request #2273:
URL: https://github.com/apache/calcite/pull/2273
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #2273: [CALCITE-4199] review-only: nullability annotations
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2273:
URL: https://github.com/apache/calcite/pull/2273#discussion_r528505146
##########
File path: core/src/main/java/org/apache/calcite/adapter/clone/ArrayTable.java
##########
@@ -243,9 +242,9 @@ public static List asList(final Representation representation,
/** Converts a value set into a compact representation. If
* {@code sources} is not null, permutes. */
- Object freeze(ColumnLoader.ValueSet valueSet, int[] sources);
+ Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources);
Review comment:
It seems some annotations appear before the type, while others appear after the type. We should make this consistent?
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #2273: [CALCITE-4199] review-only: nullability annotations
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2273:
URL: https://github.com/apache/calcite/pull/2273#discussion_r529231841
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java
##########
@@ -1049,19 +1063,19 @@ private boolean isOverlapped(Pair<Long, Long> a, Pair<Long, Long> b) {
@Override public void close() {
}
- private Object[] takeOne() {
- return list.pollFirst();
+ private @Nullable Object[] takeOne() {
+ return requireNonNull(list.pollFirst(), "list.pollFirst()");
Review comment:
This seems contradictive? `requireNonNull` indicates it should be non-nullable?
----------------------------------------------------------------
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] vlsi commented on a change in pull request #2273: [CALCITE-4199] review-only: nullability annotations
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2273:
URL: https://github.com/apache/calcite/pull/2273#discussion_r529344575
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumUtils.java
##########
@@ -1049,19 +1063,19 @@ private boolean isOverlapped(Pair<Long, Long> a, Pair<Long, Long> b) {
@Override public void close() {
}
- private Object[] takeOne() {
- return list.pollFirst();
+ private @Nullable Object[] takeOne() {
+ return requireNonNull(list.pollFirst(), "list.pollFirst()");
Review comment:
This is just fine. `pollFirst` can return `null`, and here `non-null` is required by `takeOne`
----------------------------------------------------------------
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] vlsi commented on a change in pull request #2273: [CALCITE-4199] review-only: nullability annotations
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2273:
URL: https://github.com/apache/calcite/pull/2273#discussion_r528532716
##########
File path: core/src/main/java/org/apache/calcite/adapter/clone/ArrayTable.java
##########
@@ -446,25 +451,30 @@ public static List asList(final Representation representation,
return Pair.of(codes, codeValues);
}
+ private static Pair<Object, @Nullable Comparable[]> unfreeze(Object value) {
Review comment:
The point of the PR is nullability annotations, not code refactoring. In my opinion, `Pair` should be replaced with properly named classes.
----------------------------------------------------------------
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] liyafan82 commented on a change in pull request #2273: [CALCITE-4199] review-only: nullability annotations
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2273:
URL: https://github.com/apache/calcite/pull/2273#discussion_r528506640
##########
File path: core/src/main/java/org/apache/calcite/adapter/clone/ArrayTable.java
##########
@@ -446,25 +451,30 @@ public static List asList(final Representation representation,
return Pair.of(codes, codeValues);
}
+ private static Pair<Object, @Nullable Comparable[]> unfreeze(Object value) {
Review comment:
Maybe it is helpful to replace Object with a generic type?
----------------------------------------------------------------
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] vlsi closed pull request #2273: [CALCITE-4199] review-only: nullability annotations
Posted by GitBox <gi...@apache.org>.
vlsi closed pull request #2273:
URL: https://github.com/apache/calcite/pull/2273
----------------------------------------------------------------
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] vlsi commented on a change in pull request #2273: [CALCITE-4199] review-only: nullability annotations
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2273:
URL: https://github.com/apache/calcite/pull/2273#discussion_r528531071
##########
File path: core/src/main/java/org/apache/calcite/adapter/clone/ArrayTable.java
##########
@@ -243,9 +242,9 @@ public static List asList(final Representation representation,
/** Converts a value set into a compact representation. If
* {@code sources} is not null, permutes. */
- Object freeze(ColumnLoader.ValueSet valueSet, int[] sources);
+ Object freeze(ColumnLoader.ValueSet valueSet, int @Nullable [] sources);
Review comment:
Please read https://docs.oracle.com/javase/specs/jls/se8/html/jls-9.html#jls-9.7.4 amd
https://github.com/vlsi/calcite/blob/checkerframework/site/develop/index.md#null-safety
`@Nullable int[]` and `int @Nullable []` are different.
----------------------------------------------------------------
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