Repository navigation
Nullness improvements for simple core transforms - #40022
Conversation
e6c8cf8 to
8deca9b
Compare
kennknowles
changed the title
TimestampedValue is immutable, so a TimestampedValue<V> is safe to use wherever a TimestampedValue of a supertype of V is expected. Saying so lets the nullness checker accept passing a TimestampedValue<T> where a TimestampedValue<@nullable T> is wanted, which combiners need when their accumulator starts out holding a null value. This only loosens checking, so it cannot invalidate existing callers.
8deca9b to
15f8c19
Compare
Replace the blanket class-level nullness suppression with narrow, documented suppressions on the methods that genuinely produce a null. LatestFn's accumulator holds a null value until the first input arrives, so it is now typed TimestampedValue<@nullable T>. That is invisible in the public API, since Latest.combineFn() erases AccumT to a wildcard. It relies on TimestampedValue being covariant in its value type, so that an input can still be passed where an accumulator is expected. No runtime changes beyond hoisting the timestamp comparison into a private helper so that mergeAccumulators no longer routes through addInput. The null-element check it relied on is retained.
Remove the blanket class-level nullness suppression. The representative type of WithRepresentativeValues is genuinely optional -- expand() already branches on it -- so annotate it rather than suppressing. No runtime changes.
Remove the blanket class-level nullness suppression. Reifying a view in the global window creates a single placeholder element, which is a null Void; spell that type out rather than casting. No runtime changes.
Remove the blanket class-level nullness suppression, keeping the unrelated rawtypes one. AnyValueCombineFn.extractOutput returns null when the input is empty, and OutputT is T in the public signature of anyValueCombineFn(), so that keeps a narrow suppression. No runtime changes.
15f8c19 to
c0ecc80
Compare
| @Test | ||
| public void testAddInputNullInput() { | ||
| thrown.expect(NullPointerException.class); | ||
| thrown.expect(IllegalArgumentException.class); |
There was a problem hiding this comment.
moving away from org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions.checkNotNull; to org.apache.beam.sdk.util.Preconditions.checkArgumentNotNull changes exception thrown, I'm curious if this is intended. It's not something someone would normally catch, just observation.
There was a problem hiding this comment.
It is intended: it will throw IllegalArgumentException which is accurate. NPE is too generic and doesn't say who did the wrong thing. It also reads like "segfault" to a user, indicating a bug always, but sometimes it is not a bug but is because of how it was passed.
Basically checkNotNull should almost never be used, and NPE should be thrown only when you actually dereference a null, aka when you wrote bad code.
|
Assigning reviewers: R: @Abacn for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
R: @stankiewicz |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
Remove class-level blanket suppressions with targeted suppressions and code improvements.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.