This is an automated email from the ASF dual-hosted git repository.
ahmedabu98 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/beam.git
The following commit(s) were added to refs/heads/master by this push:
new 2fdcfb830db Render ARRAY and ITERABLE fields that are not backed by a
List (#39750)
2fdcfb830db is described below
commit 2fdcfb830db716bce9a6af5dd694ef57681e6ac7
Author: ZIHAN DAI <[email protected]>
AuthorDate: Wed Oct 21 05:01:22 2026 +1100
Render ARRAY and ITERABLE fields that are not backed by a List (#39750)
* 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.
* 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.
* Remove comment
---
.../org/apache/beam/sdk/schemas/SchemaUtils.java | 32 ++++++-----
.../apache/beam/sdk/schemas/SchemaUtilsTest.java | 67 ++++++++++++++++++++++
2 files changed, 84 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 88809182efc..d45454be90f 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,7 +18,6 @@
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;
@@ -270,7 +269,11 @@ public class SchemaUtils {
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,24 +282,23 @@ public class SchemaUtils {
FieldType elementType =
Objects.requireNonNull(fieldType.getCollectionElementType());
@SuppressWarnings("unchecked")
- List<Object> list = (List<Object>) value;
- if (list.isEmpty()) {
- return "[]";
- }
+ Iterable<Object> elements = (Iterable<Object>) value;
+
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 c34832f58fb..6a5312fcc48 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.assertEquals;
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;
@@ -147,4 +149,69 @@ public class SchemaUtilsTest {
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<String> 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"));
+ }
+
+ @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<String> onceOnly =
+ new Iterable<String>() {
+ private boolean taken = false;
+
+ @Override
+ public Iterator<String> 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<String> empty = Collections::emptyIterator;
+ Row row = Row.withSchema(schema).attachValues((Object) empty);
+
+ String rendered = row.toString();
+ assertTrue(rendered, rendered.contains("[]"));
+ }
}
