[
https://issues.apache.org/jira/browse/BEAM-10265?focusedWorklogId=763721&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-763721
]
ASF GitHub Bot logged work on BEAM-10265:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 28/Apr/22 17:08
Start Date: 28/Apr/22 17:08
Worklog Time Spent: 10m
Work Description: zhoufek commented on code in PR #17477:
URL: https://github.com/apache/beam/pull/17477#discussion_r860920916
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/utils/StaticSchemaInference.java:
##########
@@ -87,22 +88,33 @@ enum MethodType {
* public getter methods, or special annotations on the class.
*/
public static Schema schemaFromClass(
- Class<?> clazz, FieldValueTypeSupplier fieldValueTypeSupplier) {
+ Class<?> clazz,
+ FieldValueTypeSupplier fieldValueTypeSupplier,
+ HashSet<Class> alreadyVisitedSchemas) {
+ if (alreadyVisitedSchemas.contains(clazz)) {
+ throw new RuntimeException(
+ "Cannot infer schema with a circular reference. Class: " +
clazz.getTypeName());
+ }
+ alreadyVisitedSchemas.add(clazz);
Schema.Builder builder = Schema.builder();
for (FieldValueTypeInformation type : fieldValueTypeSupplier.get(clazz)) {
- Schema.FieldType fieldType = fieldFromType(type.getType(),
fieldValueTypeSupplier);
+ Schema.FieldType fieldType =
+ fieldFromType(type.getType(), fieldValueTypeSupplier,
alreadyVisitedSchemas);
if (type.isNullable()) {
builder.addNullableField(type.getName(), fieldType);
} else {
builder.addField(type.getName(), fieldType);
}
}
+ alreadyVisitedSchemas.remove(clazz);
Review Comment:
Looking at this, I'm wondering if we should instead track the type through a
`Map<Class, Schema>` instead of `Set<Class>`. It might be possible that
multiple types nested under the same type reference each other in a
non-circular way. We might be able to avoid repeating work if we keep a
reference to previously computed schemas. Basically, it would look like:
```
if (visited.containsKey(clazz)) {
Schema value = visited.get(clazz);
if (value == null) {
throw new IllegalArgumentException(...);
}
return value;
}
visited.put(clazz, null);
// Compute
visited.replace(clazz, schema);
```
Note: I haven't tested this out. I'm just throwing out an idea.
Issue Time Tracking
-------------------
Worklog Id: (was: 763721)
Time Spent: 1h 40m (was: 1.5h)
> GetterBasedSchemaProvider#schemaFor stack overflows when given a recursive
> schema
> ---------------------------------------------------------------------------------
>
> Key: BEAM-10265
> URL: https://issues.apache.org/jira/browse/BEAM-10265
> Project: Beam
> Issue Type: Bug
> Components: sdk-java-core
> Affects Versions: 2.21.0, 2.22.0
> Reporter: Reza ardeshir rokni
> Assignee: Andrei Gurau
> Priority: P3
> Labels: Clarified, starter
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> Proto:
> message TSFoo {
> string a = 1;
> string b = 2;
> TSFoo theOlderMe = 3;
> }
> new ProtoMessageSchema().schemaFor(TypeDescriptor.of(Foo.TSFoo.class));
> Causes a stackoverflow.
>
>
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
