scwhittle commented on code in PR #40750:
URL: https://github.com/apache/beam/pull/40750#discussion_r4387233687
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/state/WindmillMapViaMultimap.java:
##########
@@ -62,19 +62,26 @@ public void put(KeyT key, ValueT value) {
multimap.put(key, value);
}
+ @SuppressWarnings("nullness")
@Override
public ReadableState<ValueT> computeIfAbsent(
KeyT key, Function<? super KeyT, ? extends ValueT> mappingFunction) {
// Note that computeIfAbsent comments indicate that the read is lazy but
this matches the
- // existing eager
- // behavior of WindmillMap.
+ // existing eager behavior of WindmillMap.
Iterable<ValueT> existingValues = multimap.get(key).read();
- if (Iterables.isEmpty(existingValues)) {
+ Iterator<ValueT> iterator = existingValues.iterator();
+ if (!iterator.hasNext()) {
ValueT inserted = mappingFunction.apply(key);
multimap.put(key, inserted);
return ReadableStates.immediate(inserted);
} else {
- return
ReadableStates.immediate(Iterables.getOnlyElement(existingValues));
+ ValueT existing = Iterators.getOnlyElement(iterator);
+ if (existing == null) {
Review Comment:
Hmm, there is some difficulty here because null is a valid value for some
coders within Beam
(https://beam.apache.org/releases/javadoc/2.0.0/org/apache/beam/sdk/coders/NullableCoder.html
for example)
So if the iterable exists it seems we should just return the value as it was.
However when evaluating mappingFunction above we need to decide how we
interpret null return values:
- are they real values that should be cached (as written here)
- or do they mean to not insert? in which case we shouldn't add the key and
value to the map
I think either can be valid interpretation, maybe we should see what the
existing implementations do and update the comment to match that behavior.
--
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]
