Skip to content

Commit 877547d

Browse files
committed
Address reviewer feedback: use TypeDescriptor API throughout
- Use TypeDescriptor.resolveType() instead of raw Type manipulation - Use hasUnresolvedParameters() instead of instanceof checks - Use isSubtypeOf() for collection type detection - Remove catch-all Exception block entirely Addresses @kennknowles code review comments
1 parent 390f421 commit 877547d

1 file changed

Lines changed: 29 additions & 48 deletions

File tree

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/DoFnSignatures.java

Lines changed: 29 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -2350,56 +2350,37 @@ private static void warnIfValueStateContainsCollection(
23502350
return;
23512351
}
23522352

2353-
try {
2354-
// Get the type directly and extract ValueState's type parameter
2355-
Type type = stateType.getType();
2356-
if (!(type instanceof ParameterizedType)) {
2357-
return;
2358-
}
2359-
2360-
// Find ValueState in the type hierarchy and get its type argument
2361-
Type valueType = null;
2362-
ParameterizedType pType = (ParameterizedType) type;
2363-
if (pType.getRawType() == ValueState.class) {
2364-
valueType = pType.getActualTypeArguments()[0];
2365-
} else {
2366-
// For subtypes of ValueState, we need to resolve the type parameter
2367-
return;
2368-
}
2353+
// Use TypeDescriptor.resolveType() to extract ValueState's type parameter
2354+
// This preserves generic type information better than raw Type manipulation
2355+
TypeDescriptor<?> valueTypeDescriptor =
2356+
stateType.resolveType(ValueState.class.getTypeParameters()[0]);
23692357

2370-
if (valueType == null
2371-
|| valueType instanceof java.lang.reflect.TypeVariable
2372-
|| valueType instanceof java.lang.reflect.WildcardType) {
2373-
// Cannot determine actual type, skip warning
2374-
return;
2375-
}
2376-
2377-
TypeDescriptor<?> valueTypeDescriptor = TypeDescriptor.of(valueType);
2378-
Class<?> rawType = valueTypeDescriptor.getRawType();
2379-
2380-
String recommendation = null;
2381-
if (Map.class.isAssignableFrom(rawType)) {
2382-
recommendation = "MapState";
2383-
} else if (List.class.isAssignableFrom(rawType)) {
2384-
recommendation = "BagState or OrderedListState";
2385-
} else if (java.util.Set.class.isAssignableFrom(rawType)) {
2386-
recommendation = "SetState";
2387-
}
2358+
// Skip if the type has unresolved parameters (e.g., TypeVariable, WildcardType)
2359+
if (valueTypeDescriptor.hasUnresolvedParameters()) {
2360+
return;
2361+
}
23882362

2389-
if (recommendation != null) {
2390-
LOG.info(
2391-
"DoFn {} declares ValueState '{}' with collection type {}. "
2392-
+ "ValueState reads/writes the entire collection on each access. "
2393-
+ "This is appropriate for small collections or atomic replacement. "
2394-
+ "For large collections or frequent appends, consider using {} instead "
2395-
+ "(if supported by your runner).",
2396-
fnClazz.getSimpleName(),
2397-
stateId,
2398-
rawType.getSimpleName(),
2399-
recommendation);
2400-
}
2401-
} catch (Exception e) {
2402-
// If we can't determine the type, don't warn - it's just an optimization hint
2363+
// Use TypeDescriptor.isSubtypeOf() for type checking - stays in TypeDescriptor API
2364+
String recommendation = null;
2365+
if (valueTypeDescriptor.isSubtypeOf(TypeDescriptor.of(Map.class))) {
2366+
recommendation = "MapState";
2367+
} else if (valueTypeDescriptor.isSubtypeOf(TypeDescriptor.of(List.class))) {
2368+
recommendation = "BagState or OrderedListState";
2369+
} else if (valueTypeDescriptor.isSubtypeOf(TypeDescriptor.of(java.util.Set.class))) {
2370+
recommendation = "SetState";
2371+
}
2372+
2373+
if (recommendation != null) {
2374+
LOG.info(
2375+
"DoFn {} declares ValueState '{}' with collection type {}. "
2376+
+ "ValueState reads/writes the entire collection on each access. "
2377+
+ "This is appropriate for small collections or atomic replacement. "
2378+
+ "For large collections or frequent appends, consider using {} instead "
2379+
+ "(if supported by your runner).",
2380+
fnClazz.getSimpleName(),
2381+
stateId,
2382+
valueTypeDescriptor.getRawType().getSimpleName(),
2383+
recommendation);
24032384
}
24042385
}
24052386

0 commit comments

Comments
 (0)