TheNeuralBit commented on code in PR #17477:
URL: https://github.com/apache/beam/pull/17477#discussion_r863064197
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoSchemaTranslator.java:
##########
@@ -139,6 +140,16 @@ class ProtoSchemaTranslator {
/** Option prefix for options on fields. */
public static final String SCHEMA_OPTION_FIELD_PREFIX =
"beam:option:proto:field:";
+ /**
+ * A HashMap containing the non-primitive schemas within another schema, to
prevent circular
+ * references.
Review Comment:
Could you document that this Map stores a sentinel value representing
"in-progress" schemas (null or empty schema).
##########
sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoSchemaTranslator.java:
##########
@@ -139,6 +140,16 @@ class ProtoSchemaTranslator {
/** Option prefix for options on fields. */
public static final String SCHEMA_OPTION_FIELD_PREFIX =
"beam:option:proto:field:";
+ /**
+ * A HashMap containing the non-primitive schemas within another schema, to
prevent circular
+ * references.
+ */
+ private static Map<Descriptors.Descriptor, Schema> alreadyVisitedSchemas =
+ new HashMap<Descriptors.Descriptor, Schema>();
Review Comment:
I think an alternative fix for this would be to annotate all values as
Nullable, e.g.:
```suggestion
new HashMap<Descriptors.Descriptor, @Nullable Schema>();
```
(and `@Nullable Schema existingSchema` below)
--
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]
