Skip to content

Commit ea57745

Browse files
committed
Warn for parameterized collection ValueState types
ValueState<Set<T>>, ValueState<List<T>> and ValueState<Map<K, V>> were silently skipped because the resolved value type has unresolved parameters (the element type is a type variable). The collection's own raw type is known in these cases, so the specialized-state recommendation (SetState / BagState or OrderedListState / MapState) can still be made. Drop the blanket hasUnresolvedParameters() early-return and match on the raw collection type instead; a payload that is itself a bare type variable (ValueState<T>) still produces no recommendation. Add unit tests covering the parameterized Set/List cases and the bare type-variable no-warning case.
1 parent 04f6c50 commit ea57745

2 files changed

Lines changed: 61 additions & 6 deletions

File tree

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2355,12 +2355,11 @@ private static void warnIfValueStateContainsCollection(
23552355
TypeDescriptor<?> valueTypeDescriptor =
23562356
stateType.resolveType(ValueState.class.getTypeParameters()[0]);
23572357

2358-
// Skip if the type has unresolved parameters (e.g., TypeVariable, WildcardType)
2359-
if (valueTypeDescriptor.hasUnresolvedParameters()) {
2360-
return;
2361-
}
2362-
2363-
// Use TypeDescriptor.isSubtypeOf() for type checking - stays in TypeDescriptor API
2358+
// Match on the collection's raw type. We intentionally do not skip types with unresolved
2359+
// parameters: for a parameterized collection such as ValueState<Set<T>> the collection's own
2360+
// raw type (Set) is known even though the element type T is a type variable, so we can still
2361+
// recommend SetState. A payload that is itself a bare type variable (e.g. ValueState<T>) simply
2362+
// matches none of the branches below and produces no recommendation.
23642363
String recommendation = null;
23652364
if (valueTypeDescriptor.isSubtypeOf(TypeDescriptor.of(Map.class))) {
23662365
recommendation = "MapState";

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/DoFnSignaturesTest.java

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
import org.apache.beam.sdk.state.TimerSpecs;
5252
import org.apache.beam.sdk.state.ValueState;
5353
import org.apache.beam.sdk.state.WatermarkHoldState;
54+
import org.apache.beam.sdk.testing.ExpectedLogs;
5455
import org.apache.beam.sdk.testing.SerializableMatchers;
5556
import org.apache.beam.sdk.transforms.DoFn;
5657
import org.apache.beam.sdk.transforms.Sum;
@@ -101,6 +102,8 @@ public class DoFnSignaturesTest {
101102

102103
@Rule public ExpectedException thrown = ExpectedException.none();
103104

105+
@Rule public ExpectedLogs expectedLogs = ExpectedLogs.none(DoFnSignatures.class);
106+
104107
@Test
105108
public void testBasicDoFnProcessContext() throws Exception {
106109
DoFnSignature sig =
@@ -1735,6 +1738,32 @@ private static class DoFnWithSimpleValueState extends DoFn<String, String> {
17351738
public void process() {}
17361739
}
17371740

1741+
private static class DoFnWithParameterizedSetValueState<T> extends DoFn<String, String> {
1742+
@StateId("parameterizedSetState")
1743+
private final StateSpec<ValueState<java.util.Set<T>>> parameterizedSetState =
1744+
StateSpecs.value();
1745+
1746+
@ProcessElement
1747+
public void process() {}
1748+
}
1749+
1750+
private static class DoFnWithParameterizedListValueState<T> extends DoFn<String, String> {
1751+
@StateId("parameterizedListState")
1752+
private final StateSpec<ValueState<java.util.List<T>>> parameterizedListState =
1753+
StateSpecs.value();
1754+
1755+
@ProcessElement
1756+
public void process() {}
1757+
}
1758+
1759+
private static class DoFnWithTypeVariableValueState<T> extends DoFn<String, String> {
1760+
@StateId("typeVariableState")
1761+
private final StateSpec<ValueState<T>> typeVariableState = StateSpecs.value();
1762+
1763+
@ProcessElement
1764+
public void process() {}
1765+
}
1766+
17381767
@Test
17391768
public void testValueStateWithMapLogsWarning() {
17401769
// This test verifies that the signature can be parsed for DoFns with collection ValueState.
@@ -1760,5 +1789,32 @@ public void testValueStateWithSimpleTypeNoWarning() {
17601789
// Simple types should not trigger any warning
17611790
DoFnSignature signature = DoFnSignatures.getSignature(DoFnWithSimpleValueState.class);
17621791
assertThat(signature.stateDeclarations().get("simpleState"), notNullValue());
1792+
expectedLogs.verifyNotLogged("with collection type");
1793+
}
1794+
1795+
@Test
1796+
public void testValueStateWithParameterizedSetLogsWarning() {
1797+
// A parameterized collection such as ValueState<Set<T>> still has a known raw collection type
1798+
// (Set) even though the element type is a type variable, so it should still recommend SetState.
1799+
DoFnSignature signature = DoFnSignatures.getSignature(DoFnWithParameterizedSetValueState.class);
1800+
assertThat(signature.stateDeclarations().get("parameterizedSetState"), notNullValue());
1801+
expectedLogs.verifyWarn("SetState");
1802+
}
1803+
1804+
@Test
1805+
public void testValueStateWithParameterizedListLogsWarning() {
1806+
DoFnSignature signature =
1807+
DoFnSignatures.getSignature(DoFnWithParameterizedListValueState.class);
1808+
assertThat(signature.stateDeclarations().get("parameterizedListState"), notNullValue());
1809+
expectedLogs.verifyWarn("BagState or OrderedListState");
1810+
}
1811+
1812+
@Test
1813+
public void testValueStateWithBareTypeVariableNoWarning() {
1814+
// A bare type variable payload (ValueState<T>) has no raw collection type to inspect and must
1815+
// not produce a collection recommendation.
1816+
DoFnSignature signature = DoFnSignatures.getSignature(DoFnWithTypeVariableValueState.class);
1817+
assertThat(signature.stateDeclarations().get("typeVariableState"), notNullValue());
1818+
expectedLogs.verifyNotLogged("with collection type");
17631819
}
17641820
}

0 commit comments

Comments
 (0)