[GitHub] [beam] kennknowles commented on a change in pull request #7316: [BEAM-6269] Cross-SDK transform expansion protocol.

[GitBox <gi...@apache.org>]

kennknowles commented on a change in pull request #7316:
URL: https://github.com/apache/beam/pull/7316#discussion_r501248667



##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/runners/TransformHierarchy.java
##########
@@ -629,13 +635,15 @@ private void visit(
       if (!isRootNode()) {
         // Visit inputs.
         for (PValue inputValue : inputs.values()) {
-          Node valueProducer = getProducer(inputValue);
-          if (!visitedNodes.contains(valueProducer)) {
-            valueProducer.visit(visitor, visitedValues, visitedNodes, skippedComposites);
-          }
-          if (visitedValues.add(inputValue)) {
-            LOG.debug("Visiting input value {}", inputValue);
-            visitor.visitValue(inputValue, valueProducer);
+          Node valueProducer = maybeGetProducer(inputValue);
+          if (valueProducer != null) {
+            if (!visitedNodes.contains(valueProducer)) {
+              valueProducer.visit(visitor, visitedValues, visitedNodes, skippedComposites);
+            }
+            if (visitedValues.add(inputValue)) {

Review comment:
       I was just looking into side inputs. I believe that this change means any value with a direct producer is not visited. So `PCollectionView` is only visited because it is produced by a transform. But in fact if we remove the deprecated `WritePCollectionView` primitive they are never visited. I think they should be visited still. I'm not exactly sure the ramifications. I'll post a PR if I figure it out but LMK if you know already whether this will break.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org