kennknowles commented on code in PR #30488:
URL: https://github.com/apache/beam/pull/30488#discussion_r1514700415
##########
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:
OK that sounds like a pain and I'm OK with it. Can you narrow the
suppression to one line in the body of the method instead of the whole method?
(per rationale in other comment)
##########
runners/flink/src/main/java/org/apache/beam/runners/flink/adapter/BeamFlinkDataSetAdapter.java:
##########
@@ -213,7 +216,8 @@ private <InputT>
FlinkBatchPortablePipelineTranslator.PTransformTranslator flink
// When we run into a FlinkInput operator, it "produces" the
corresponding input as its
// "computed result."
String inputId = t.getTransform().getSpec().getPayload().toStringUtf8();
- DataSet<InputT> flinkInput = Preconditions.checkNotNull(
(DataSet<InputT>) inputMap.get(inputId));
+ DataSet<InputT> flinkInput =
+ Preconditions.checkNotNull((DataSet<InputT>) inputMap.get(inputId));
Review Comment:
I agree with the philosophy. Just see a different balance. The user is going
to report the error, probably without stack trace, and (IMO correctly) view
that NPE == segfault in terms of how it reflects on the quality of the software
being built. So having a meaningful message and better exception accelerates
communication and debugging a lot.
For example, some of these checks should say IllegalState (or
IllegalArgument, debatable) with "corrupted portable pipeline proto" which is
much clearer than having to read the code to determine it.
##########
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:
Yes, that crux is just an argument about types in general, not specific to
null checks. In modern typing, `@Nullable Foo` and `Foo` are different types,
and you should know which one you are dealing with.
FWIW the nullness checker is simply a fully rigorous type system, and
significantly smarter than, say, Java generic inference. It can definitely
infer polymorphism of nullability (I haven't dug into this particular line of
code). The problem you are describing sounds like the fact that Java does not
implicitly extend type variables, so if you have, say, a `Collection<Foo>` and
you want to map over it, the `map` function has to accept `Function<X super
Foo, ...>` and otherwise won't be able to accept variations.
But either way, I know this is hard in general, especially interacting with
legacy codebases. So putting targeted suppressions with explanations is OK,
though adding checks so that the error is more localized is better.
Here is the biggest reason I push on suppression of type checking: it will
suppress it for the other lines that are not part of what you are trying to
deal with, and it will also suppress it for future code. And of course when
someone comes across the code they will think they should just go ahead and
suppress it whenever they want.
So ideally suppress would be on just one single line that adjusts types from
what is inferred to what you want them to be. But you can usually do that with
a check.
--
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]
