You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2023/01/19 16:33:49 UTC
[GitHub] [iceberg] jackye1995 opened a new issue, #6625: Improve nullability check in Iceberg codebase
jackye1995 opened a new issue, #6625:
URL: https://github.com/apache/iceberg/issues/6625
### Feature Request / Improvement
Based on discussion in #6474
We should consolidate some consistent way to check nulls, currently both `checkNoNull` and `checkArgument(xxx != null)` are used in different places.
Also there is little indication in the codebase of which field could potentially be null. This causes a lot of confusions for external engine integrations like Trino.
### Query engine
None
--
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: issues-unsubscribe@iceberg.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on issue #6625: Improve nullability check in Iceberg codebase
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on issue #6625:
URL: https://github.com/apache/iceberg/issues/6625#issuecomment-1398577729
@nastra any thoughts?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on issue #6625: Improve nullability check in Iceberg codebase
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on issue #6625:
URL: https://github.com/apache/iceberg/issues/6625#issuecomment-1404235165
This sounds like a very cool plugin to add, any thoughts about that? @rdblue
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] findepi commented on issue #6625: Improve nullability check in Iceberg codebase
Posted by GitBox <gi...@apache.org>.
findepi commented on issue #6625:
URL: https://github.com/apache/iceberg/issues/6625#issuecomment-1398096721
> Also there is little indication in the codebase of which field could potentially be null. This causes a lot of confusions for external engine integrations like Trino.
I am happy to contribute annotations for nullable or non-null elements, if that's what we agree to do.
This could look e.g. like this https://github.com/apache/iceberg/pull/4978, or maybe someone has better suggestions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] github-actions[bot] commented on issue #6625: Improve nullability check in Iceberg codebase
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on issue #6625:
URL: https://github.com/apache/iceberg/issues/6625#issuecomment-1648785722
This issue has been automatically marked as stale because it has been open for 180 days with no activity. It will be closed in next 14 days if no further activity occurs. To permanently prevent this issue from being considered stale, add the label 'not-stale', but commenting on the issue is preferred when possible.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on issue #6625: Improve nullability check in Iceberg codebase
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on issue #6625:
URL: https://github.com/apache/iceberg/issues/6625#issuecomment-1399656418
The reason why we don't use `checkNonNull` is that it throws `NullPointerException`, which mostly makes people think that something tried to dereference it. We use `checkArgument` so the reader gets the message "you passed the wrong thing" rather than "there's a null dereferenced somewhere in Iceberg". This distinction is minor since NPE is a subclass of `ArgumentException`.
As I mentioned on #6474, the main problem with this was that we have a convention for error messages and this needlessly changed the error message to be inconsistent.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on issue #6625: Improve nullability check in Iceberg codebase
Posted by GitBox <gi...@apache.org>.
nastra commented on issue #6625:
URL: https://github.com/apache/iceberg/issues/6625#issuecomment-1398623955
I also like annotations like `@Nullable` to indicate that certain things in the API can be nullable as this makes it easier to consume that particular API and reason about it.
Maybe we don't need something like `@NonNull` (given that we'd have to agree from which library we'd want to use that annotation) if we could imply that whatever doesn't have `@Nullable` is automatically non-null.
An alternative would be the usage of `Optional`, but `@Nullable` would be my preferred option.
However, I'd like to hear opinions & thoughts from other people as well on that topic.
@jackye1995 should we maybe raise this discussion topic on the mailing list in order to increase visibility for people?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] XN137 commented on issue #6625: Improve nullability check in Iceberg codebase
Posted by "XN137 (via GitHub)" <gi...@apache.org>.
XN137 commented on issue #6625:
URL: https://github.com/apache/iceberg/issues/6625#issuecomment-1403346772
since the build is already using errorprone i think NullAway would be something to consider:
https://github.com/uber/NullAway/
https://www.uber.com/en-US/blog/nullaway/
newer versions of the gradle-baseline plugins support configuring it more conveniently:
https://github.com/palantir/gradle-baseline/tree/develop/baseline-null-away
the basic idea is that (below a certain package) all reference types are assumed to be non-null unless they are annotated with `@Nullable`
(which means that `@NonNull` is not needed, since it is the default)
so these things become compile time errors:
- returning `null` (or a nullable type) from a method that is not annotated
- passing `null` (or a nullable type) into a method whose parameter is not annotated
- assigning `null` (or a nullable type) to a field that is not annotated
- leaving an unannotated field uninitialized after all constructor/initializer paths
- (... others, you get the idea...)
- most importantly: unconditionally de-referencing a `@Nullable` reference type
inside an `if (x != null)` block, x of course is no longer considered to have a nullable type
once these initial errors are fixed, you will see transitive errors and iterate on fixing them until you reach a stable state, where the use of null is "consistent" according to these rules.
one could start with iceberg api, then do it for core and later all other downstream iceberg modules.
there is a convenient way of getting started with these "1st level" rules by using the following errorprone checks:
https://errorprone.info/bugpattern/ReturnMissingNullable
https://errorprone.info/bugpattern/FieldMissingNullable
https://errorprone.info/bugpattern/ParameterMissingNullable
https://errorprone.info/bugpattern/EqualsMissingNullable
note: you can run errorprone in a way so all these missing `@Nullable` annotations are added automatically
so even if NullAway would not be used eventually, applying + enforcing those "1st level" errorprone checks, would help documenting (a subset of) the APIs in terms of nullability.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 commented on issue #6625: Improve nullability check in Iceberg codebase
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on issue #6625:
URL: https://github.com/apache/iceberg/issues/6625#issuecomment-1398630489
> should we maybe raise this discussion topic on the mailing list in order to increase visibility for people?
Yes agree, let's do that so we can reach a consensus and proceed to implementation. If there is no response we can raise it during the next sync meeting.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org