From caec0af354abafb6141678a19f2057bcfe709e36 Mon Sep 17 00:00:00 2001 From: ffccites <99155080+PDGGK@users.noreply.github.com> Date: Fri, 14 Aug 2026 22:20:19 +1000 Subject: [PATCH 1/3] Render ARRAY and ITERABLE fields that are not backed by a List Row#toString throws for an ITERABLE field holding a plain Iterable: Schema s = Schema.builder().addStringField("k") .addIterableField("vals", FieldType.STRING).build(); Row.withSchema(s).attachValues("k1", () -> list.iterator()).toString(); IllegalArgumentException: value type is '...' for field type 'ITERABLE' toPrettyFieldValueString demanded a List before iterating. An ITERABLE field declares an Iterable, so the guard is stricter than the type it guards, and the branch only ever iterates and counts -- both of which Iterable supports once materialised. Losing a whole Row's rendering is a poor trade for a stricter check here, since this is what Row#toString calls: one unusual field takes out logging and debugger output for every field beside it. The guard now requires Iterable and materialises once for the size count. A value that is neither still throws the same exception. Two tests: a bare Iterable on an ITERABLE field, which fails on master, and an ordinary List array as a control that passes either way. The first also asserts the value really is not a List before rendering, so it cannot go green for the wrong reason if the value ever starts arriving materialised. --- .../apache/beam/sdk/schemas/SchemaUtils.java | 9 ++++-- .../beam/sdk/schemas/SchemaUtilsTest.java | 28 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java index 88809182efcd..1a23e927900b 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java @@ -24,6 +24,7 @@ import org.apache.beam.sdk.schemas.Schema.FieldType; import org.apache.beam.sdk.schemas.Schema.LogicalType; import org.apache.beam.sdk.values.Row; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.Lists; /** A set of utility functions for schemas. */ @SuppressWarnings({ @@ -270,7 +271,11 @@ static String toPrettyFieldValueString(Schema.FieldType fieldType, Object value, case ARRAY: case ITERABLE: { - if (!(value instanceof List)) { + // An ITERABLE field declares an Iterable, and an ARRAY value need not be a List + // either, so require only what is actually iterated below. This is reached from + // Row#toString, where refusing to render one field takes out logging and debugger + // output for the entire row. + if (!(value instanceof Iterable)) { throw new IllegalArgumentException( String.format( "value type is '%s' for field type '%s'", @@ -279,7 +284,7 @@ static String toPrettyFieldValueString(Schema.FieldType fieldType, Object value, FieldType elementType = Objects.requireNonNull(fieldType.getCollectionElementType()); @SuppressWarnings("unchecked") - List list = (List) value; + List list = Lists.newArrayList((Iterable) value); if (list.isEmpty()) { return "[]"; } diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/SchemaUtilsTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/SchemaUtilsTest.java index c34832f58fb8..71b6406ae630 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/SchemaUtilsTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/SchemaUtilsTest.java @@ -147,4 +147,32 @@ public void testToPrettyStringRendersNullInsideArrayOfInts() { assertTrue(rendered, rendered.contains("1")); assertTrue(rendered, rendered.contains("null")); } + + @Test + public void testToPrettyStringRendersIterableThatIsNotAList() { + Schema schema = + Schema.builder().addStringField("k").addIterableField("vals", FieldType.STRING).build(); + // A bare Iterable, which is exactly what an ITERABLE field declares. + Iterable bare = () -> Arrays.asList("p", "q").iterator(); + Row row = Row.withSchema(schema).attachValues("k1", bare); + + // The precondition that makes this test meaningful: if the value ever starts arriving as a + // List, the assertion below would pass for the wrong reason. + assertTrue(!(row.getValue("vals") instanceof java.util.List)); + + String rendered = row.toString(); + assertTrue(rendered, rendered.contains("p")); + assertTrue(rendered, rendered.contains("q")); + } + + @Test + public void testToPrettyStringStillRendersAListArray() { + // Control: the ordinary List case must be unchanged. + Schema schema = Schema.builder().addArrayField("vals", FieldType.STRING).build(); + Row row = Row.withSchema(schema).attachValues((Object) Arrays.asList("p", "q")); + + String rendered = row.toString(); + assertTrue(rendered, rendered.contains("p")); + assertTrue(rendered, rendered.contains("q")); + } } From f297bcfc35878efd968274e69856ebbd0c34d35a Mon Sep 17 00:00:00 2001 From: Zihan Dai <99155080+PDGGK@users.noreply.github.com> Date: Sat, 15 Aug 2026 08:16:59 +1000 Subject: [PATCH 2/3] Render the collection in one pass instead of copying it Review feedback from @ahmedabu98: keep the original iterable rather than recreating it as a list. The copy was load-bearing as the block stood, because it asked the collection for isEmpty(), then size(), then iterated it -- three traversals. A List absorbs that for free; an Iterable that can only be read once does not, and that is exactly the kind of value this PR widens support to. Doing it in a single pass gets both: no copy, and no assumption that the collection can be traversed more than once. Output is unchanged -- elements are separated by ",\n" as before, and an empty collection still renders as "[]". Two tests added. The one-shot case hands out its iterator exactly once and throws on a second attempt; with Iterables.isEmpty plus Iterables.size over the raw iterable it fails with "iterated more than once", and it is the only one of the thirteen that does. --- .../apache/beam/sdk/schemas/SchemaUtils.java | 30 +++++++------- .../beam/sdk/schemas/SchemaUtilsTest.java | 39 +++++++++++++++++++ 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java index 1a23e927900b..c882870ffbb5 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java @@ -18,13 +18,11 @@ package org.apache.beam.sdk.schemas; import java.util.Arrays; -import java.util.List; import java.util.Map; import java.util.Objects; import org.apache.beam.sdk.schemas.Schema.FieldType; import org.apache.beam.sdk.schemas.Schema.LogicalType; import org.apache.beam.sdk.values.Row; -import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.Lists; /** A set of utility functions for schemas. */ @SuppressWarnings({ @@ -284,24 +282,26 @@ static String toPrettyFieldValueString(Schema.FieldType fieldType, Object value, FieldType elementType = Objects.requireNonNull(fieldType.getCollectionElementType()); @SuppressWarnings("unchecked") - List list = Lists.newArrayList((Iterable) value); - if (list.isEmpty()) { - return "[]"; - } + Iterable elements = (Iterable) value; + + // Rendered in a single pass. The previous shape asked the collection for isEmpty(), + // then size(), then iterated it -- three traversals, which a List absorbs for free but + // which an Iterable that can only be read once would not survive. StringBuilder sb = new StringBuilder(); sb.append("[\n"); - int size = list.size(); - int index = 0; - for (Object element : list) { - sb.append(nextPrefix) - .append(toPrettyFieldValueString(elementType, element, nextPrefix)); - if (index++ < size - 1) { + boolean empty = true; + for (Object element : elements) { + if (!empty) { sb.append(",\n"); - } else { - sb.append("\n"); } + sb.append(nextPrefix) + .append(toPrettyFieldValueString(elementType, element, nextPrefix)); + empty = false; + } + if (empty) { + return "[]"; } - sb.append(prefix).append("]"); + sb.append("\n").append(prefix).append("]"); return sb.toString(); } case MAP: diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/SchemaUtilsTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/SchemaUtilsTest.java index 71b6406ae630..6a5312fcc48d 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/SchemaUtilsTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/SchemaUtilsTest.java @@ -21,7 +21,9 @@ import static org.junit.Assert.assertTrue; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; +import java.util.Iterator; import java.util.Map; import org.apache.beam.sdk.schemas.Schema.FieldType; import org.apache.beam.sdk.values.Row; @@ -175,4 +177,41 @@ public void testToPrettyStringStillRendersAListArray() { assertTrue(rendered, rendered.contains("p")); assertTrue(rendered, rendered.contains("q")); } + + @Test + public void testToPrettyStringRendersAnIterableThatCanOnlyBeReadOnce() { + Schema schema = + Schema.builder().addStringField("k").addIterableField("vals", FieldType.STRING).build(); + // Hands out its iterator exactly once and fails loudly on a second attempt. Rendering asks + // the collection for one traversal now; asking for isEmpty(), then size(), then iterating + // would trip the guard below. + Iterable onceOnly = + new Iterable() { + private boolean taken = false; + + @Override + public Iterator iterator() { + if (taken) { + throw new IllegalStateException("iterated more than once"); + } + taken = true; + return Arrays.asList("p", "q").iterator(); + } + }; + Row row = Row.withSchema(schema).attachValues("k1", onceOnly); + + String rendered = row.toString(); + assertTrue(rendered, rendered.contains("p")); + assertTrue(rendered, rendered.contains("q")); + } + + @Test + public void testToPrettyStringRendersAnEmptyIterableAsEmptyBrackets() { + Schema schema = Schema.builder().addIterableField("vals", FieldType.STRING).build(); + Iterable empty = Collections::emptyIterator; + Row row = Row.withSchema(schema).attachValues((Object) empty); + + String rendered = row.toString(); + assertTrue(rendered, rendered.contains("[]")); + } } From 051fdc11ae718252d1a8d4e3bbe1f18bc6c9841e Mon Sep 17 00:00:00 2001 From: Zihan Dai Date: Tue, 20 Oct 2026 16:30:25 +1100 Subject: [PATCH 3/3] Remove comment --- .../src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java index c882870ffbb5..d45454be90fa 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java @@ -284,9 +284,6 @@ static String toPrettyFieldValueString(Schema.FieldType fieldType, Object value, @SuppressWarnings("unchecked") Iterable elements = (Iterable) value; - // Rendered in a single pass. The previous shape asked the collection for isEmpty(), - // then size(), then iterated it -- three traversals, which a List absorbs for free but - // which an Iterable that can only be read once would not survive. StringBuilder sb = new StringBuilder(); sb.append("[\n"); boolean empty = true;