kennknowles commented on code in PR #30488:
URL: https://github.com/apache/beam/pull/30488#discussion_r1516651448
##########
runners/flink/src/main/java/org/apache/beam/runners/flink/adapter/FlinkInput.java:
##########
@@ -71,7 +71,7 @@ public String getUrn() {
}
@Override
- @SuppressWarnings("nullness") //
TODO(https://github.com/apache/beam/issues/20497)
+ @SuppressWarnings("nullness") //
TODO(https://github.com/apache/beam/issues/20497)
Review Comment:
Ah that's a bummer.
... But wait this is actually our own code and the nullness in the types is
as intended. The return value is optional.
https://github.com/apache/beam/blob/2f3fac6e14017f8d1d2d6cae42c6c3366d9065bb/sdks/java/core/src/main/java/org/apache/beam/sdk/util/construction/PTransformTranslation.java#L620
##########
runners/flink/src/main/java/org/apache/beam/runners/flink/adapter/BeamAdapterUtils.java:
##########
@@ -76,7 +76,8 @@ Map<String, DataSetOrStreamT> applyBeamPTransformInternal(
new FlinkInput<>(
key,
BeamAdapterCoderUtils.typeInformationToCoder(
-
getTypeInformation.apply(Preconditions.checkNotNull(flinkInput)),
coderRegistry)))));
+
getTypeInformation.apply(Preconditions.checkNotNull(flinkInput)),
Review Comment:
I mean, checkerframework does have a type system for tracking keys that
exist in maps but you don't want to go there :-)
The basic truth is that you generally should always check map return results
for null. Even if today there is a clear invariant that it will be in there,
that will get broken by people down the line. A message saying what is
corrupted is great.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
