Repository navigation
[#20502] Remove nullness suppressions from the direct-java runner - #40295
Merged
stankiewicz merged 12 commits intoSep 28, 2026
Merged
Conversation
nitinware
marked this pull request as ready for review
September 27, 2026 04:57
Contributor
Author
|
R: @kennknowles |
Contributor
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
Contributor
Author
|
Run Java PreCommit |
…iles Remove blanket @SuppressWarnings("nullness") with root-cause fixes: - DisplayDataValidator: accept @nullable HasDisplayData and no-op on null (only the root node's transform is null). - DirectGraph: checkStateNotNull on the producers/viewWriters/stepNames lookups, preserving the non-null contract without changing the ExecutableGraph interface or its callers. RootProviderRegistry keeps "rawtypes" (apache#20447); DirectGraph keeps "keyfor". CommittedResult (AutoValue) needed no code change.
…actories Remove blanket @SuppressWarnings("nullness") with root-cause fixes: - ImmutableListBundleFactory: root bundles legitimately carry a null PCollection, so mark the field/param/getter @nullable to match the already-@nullable UncommittedBundle.getPCollection() contract. - CloningBundleFactory / ImmutabilityCheckingBundleFactory: annotate the getter @nullable (interface contract) and checkStateNotNull the PCollection where a non-null value is required -- these wrappers are only ever applied to non-root bundles.
… and evaluator factories Remove blanket @SuppressWarnings("nullness") with root-cause fixes: - KeyedPValueTrackingVisitor / DirectGraphVisitor / DirectWriteViewVisitor: TransformHierarchy.Node.getTransform() is @nullable only for the root node. Capture it into a local and checkStateNotNull where a non-root transform is guaranteed, rather than calling the nullable getter twice across an instanceof. - DirectWriteViewVisitor: initialize viewsToWrite at declaration; checkStateNotNull on the @nullable PCollectionView.getPCollection() and PCollection.getTypeDescriptor(). - GroupByKeyOnlyEvaluatorFactory: make getKeyCoder static so it is not invoked on a partially-initialized receiver from the constructor. Keep "rawtypes" (apache#20447) and "keyfor" where present.
…n context and evaluator dispatch Remove blanket @SuppressWarnings("nullness") with root-cause fixes: - DirectExecutionContext: mark the lazily-initialized DirectStepContext fields @nullable, and annotate commitState() @nullable to match its documented "returns null if no state was accessed" contract. - StepTransformResult: mark the builder's state field @nullable (matches the already-@nullable TransformResult.getState()); replace TimerUpdate.builder(null).build() with the equivalent TimerUpdate.empty() instead of passing null to a non-null builder param. - ImmutabilityEnforcementFactory: checkStateNotNull on the input PCollection and the per-element MutationDetector lookup (both guaranteed by the ModelEnforcement lifecycle). - ParDoEvaluatorFactory: checkStateNotNull on the input bundle's PCollection (a ParDo always has a real main input). - TransformEvaluatorRegistry: checkStateNotNull on the throwable that is provably assigned inside the non-empty-exceptions block. Keep "rawtypes" (apache#20447) where present.
… stream evaluator factories Remove blanket @SuppressWarnings("nullness") with root-cause fixes: - BoundedReadEvaluatorFactory: type the dynamic-split future as Future<@nullable BoundedSource<...>> (a null result means "no residual split"), propagate @nullable through the GenerateSplitAtHalfwayPoint Callable, and checkStateNotNull the ExecutionException cause before rewrapping. - UnboundedReadEvaluatorFactory: capture shard.getExistingReader() into a local before the null-check/dereference. The nullable type-variable checkpoint property hits AutoValue bug google/auto#1320 (the generated constructor drops @nullable on a type-variable property), so the of() factory carries a narrow method-scoped suppression citing that bug -- matching the precedent already in sdk core Read.java. - TestStreamEvaluatorFactory: no code change needed once the blanket suppression was removed. - SplittableProcessElementsEvaluatorFactory: checkStateNotNull the input bundle's PCollection; replace the constructor's hashCode() call on the still-initializing receiver with a static AtomicInteger sequence for the unique checkpoint-executor thread name. Keep "rawtypes" (apache#20447) where present.
…aluators Remove blanket @SuppressWarnings("nullness") with root-cause fixes: - ParDoEvaluator: mark the commitState() result @nullable (already guarded by an explicit null-check) and replace the containsKey/get pair in BundleOutputManager.output with checkArgumentNotNull, which the checker recognizes as establishing non-null. - StatefulParDoEvaluatorFactory: checkStateNotNull the input bundle's PCollection and the NavigableSet.pollFirst() result (guarded by the surrounding non-empty loop); capture the delegate state once and checkStateNotNull the commitState() derefs in the timer-hold branches (a declared timer has accessed state via setWatermarkHold). - GroupAlsoByWindowEvaluatorFactory: checkStateNotNull the input PCollection and the commitState() result (processElement always accesses state internals); render the possibly-null generic key via String.valueOf for the WindowTracing.debug varargs, which require non-null. Keep "keyfor". - StepTransformResult: widen Builder.withState to accept @nullable (the backing field is already @nullable; the state-only branch of the stateful evaluator can pass a null delegate state). Keep "rawtypes" (apache#20447) and "keyfor" where present.
… and scheduling utilities Remove blanket nullness suppressions from QuiescenceDriver, TransformExecutorServices, and WatermarkCallbackExecutor. - TransformExecutorServices: type currentlyEvaluating as AtomicReference<@nullable TransformExecutor>; it is initialized to null and cleared via compareAndSet(completed, null). - WatermarkCallbackExecutor: replace get/putIfAbsent/re-get with a putIfAbsent-and-pick idiom; guard peek()/poll() results with checkStateNotNull inside the isEmpty()-checked loop. - QuiescenceDriver: checkStateNotNull for poll()/get()/getPCollection() results; replace the compute() remapping (which the checker cannot type as nullable-returning) with explicit get/remove under the already-held inflightBundles lock. Keeps rawtypes (apache#20447) and keyfor suppressions.
…uts, metrics, and executor Remove blanket nullness suppressions from SideInputContainer, DirectMetrics, and ExecutorServiceParallelExecutor. - SideInputContainer: type the view-contents AtomicReference as AtomicReference<@nullable Iterable<...>> (null is its unset sentinel, set via compareAndSet(null, ...)); guard the get() in the CAS loop with checkStateNotNull. - DirectMetrics: bound DirectMetric's UpdateT as @nonnull (aggregate values are stored in a ConcurrentMap and are never null); accept a @nullable filter in maybeExtractResult and short-circuit null before the non-null MetricFiltering.matches call. - ExecutorServiceParallelExecutor: make the cache-loader and removal-listener factories static so they are not called on an under-initialization receiver; capture nullable peek()/getNewState() results before dereferencing; type the nullable local update and exception. Annotate PipelineExecutor.waitUntilFinish (and the override) @nullable — it returns null on timeout. Keeps the rawtypes suppression on SideInputContainer (apache#20447).
…and state internals Remove the blanket "nullness" suppression from CopyOnAccessInMemoryStateInternals and MultiStepCombine, keeping the "rawtypes" suppression (apache#20447). CopyOnAccessInMemoryStateInternals: bound the key type variable @nonnull (state keys are always a StructuralKey), and annotate the @nullable underlying-container parameters on withUnderlying, the private constructor, and the state table. MultiStepCombine: the CombineInputs DoFn extends DoFn<KV<K,InputT>, KV<K,AccumT>>. KV's type parameters carry a @nonnull upper bound, so reads via element().getKey()/ getValue() and the output(KV<K,AccumT>) contract require non-null components. Drop the @nullable bounds on the CombineInputs type variables so they align with KV's contract, and assert the accumulator invariant with checkStateNotNull: every key in the timestamps map was written to accumulators in the same processElement call, and a null accumulator could not be encoded by the accumulator coder downstream.
…nd evaluation context Remove the blanket "nullness" suppression from DirectRunner and EvaluationContext (EvaluationContext keeps "rawtypes"/apache#20447 and "keyfor"). EvaluationContext: break the EvaluationContext <-> SideInputContainer construction cycle by setting sideInputContainer in the create() factory after the context is fully initialized (field is now @MonotonicNonNull, reads guarded with checkStateNotNull), instead of passing an under-initialization `this` from the constructor. Assert the output-bundle invariant in commitBundles: committed bundles are transform outputs and always target a PCollection. DirectExecutionContext: existingState is @nullable -- there is no prior state on the first execution, and it flows into CopyOnAccessInMemoryStateInternals.withUnderlying, whose underlying parameter is @nullable. DirectRunner: a caught UserCodeException always carries a non-null cause (wrap() calls super(t)), so assert it with checkStateNotNull before rethrowing as PipelineExecutionException. waitUntilFinish(Duration) keeps a narrow, documented nullness suppression: PipelineResult.waitUntilFinish(Duration) is documented to return null on timeout but its interface return type is not annotated @nullable, and annotating it would ripple across every runner's override.
nitinware
force-pushed
the
nullness-direct-java-20502
branch
from
September 27, 2026 17:34
4db0f27 to
5f5a3f5
Compare
Contributor
Author
|
Run Java PreCommit |
1 similar comment
Contributor
Author
|
Run Java PreCommit |
The earlier cleanup narrowed CombineInputs' type parameters to @nonnull and wrapped accumulators.get(key) in checkStateNotNull. That was incorrect: MultiStepCombine is generic over a nullable AccumT (e.g. an empty HllCount sketch has a null accumulator), so the accumulator value is legitimately null in normal operation. The guard turned that valid null into an IllegalStateException, breaking the zetasketch HllCount empty-sketch tests (:sdks:java:extensions:zetasketch:test, which runs on the DirectRunner). Revert the file to its null-tolerant form and retain the class-level "nullness" suppression, now documented as a deliberate, justified residual (the checker cannot express the nullable-accumulator invariant). This mirrors the accepted DirectPipelineResult residual for this issue. Verified: :runners:direct-java:compileJava --rerun-tasks BUILD SUCCESSFUL (checker clean); :sdks:java:extensions:zetasketch:test 50/50 passing.
Contributor
|
nice! |
stankiewicz
approved these changes
Sep 28, 2026
Contributor
Author
|
@stankiewicz thanks for the review and merge! Appreciate it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes the blanket
@SuppressWarnings("nullness")suppressions from the:runners:direct-javamodule, resolving the nullness portion of #20502 (umbrella #20497).All 36 files in the module now compile cleanly under the Checker Framework nullness checker with no blanket nullness suppression.
rawtypes(#20447) andkeyforsuppressions are preserved where present — onlynullnesswas removed.Approach
org.apache.beam.sdk.util.Preconditions.checkStateNotNull(guardedfirstEntry()/poll()calls, registered-executable lookups, timer keys known non-null by construction).@Nullableto make the contracts explicit.<... extends @NonNull Object>where the checker requires@NonNullkeys for map/collection lookups.WatermarkManagerby moving per-transform watermark population out of the constructor into aninitializeTransformWatermarks()method invoked from the staticcreate()factory after the instance is fully initialized.PerKeyHoldsto track the single keyless (null-key) hold in a dedicated field so itskeyedHoldsmap never requires a null key — O(1), with no suppression.Residual scoped suppression
Residual scoped suppressions
Two files retain a narrow, commented
@SuppressWarnings("nullness").DirectPipelineResult.waitUntilFinish(Duration)—PipelineResult.waitUntilFinish(Duration)insdk-coreis documented to returnnullon timeout, but its interface return type is not annotated@Nullable. Annotating the interface would affect overrides across multiple runners, so the timeout-null case is suppressed locally rather than changing thesdk-coreAPI. Same precedent asFlinkRunnerResultandJobServicePipelineResult.MultiStepCombine— this class is generic over a nullableAccumT(e.g. an emptyHllCountsketch has a null accumulator), so accumulator values, keys, and combine inputs are legitimately nullable in normal operation. The checker cannot express that invariant, and forcing@NonNullbounds converts valid nulls into runtime failures. The class-level suppression is retained and documented.Therefore, #20502 is resolved as no blanket nullness suppressions, rather than literally zero scoped nullness suppressions.
Testing
./gradlew :runners:direct-java:compileJava --rerun-tasks— BUILD SUCCESSFUL (Checker Framework nullness checker clean)./gradlew :runners:direct-java:spotlessJavaCheck :runners:direct-java:checkstyleMain— BUILD SUCCESSFULWatermarkManagerTest— 34 passed, 1 skipped, 0 failed./gradlew :sdks:java:extensions:zetasketch:test --rerun-tasks— 50 passed, 0 failed (DirectRunner-based; exercisesMultiStepCombinewith empty/null accumulators)Fixes #20502
R: @kennknowles
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.