PDGGK commented on code in PR #39750:
URL: https://github.com/apache/beam/pull/39750#discussion_r3787504616
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/SchemaUtils.java:
##########
@@ -279,7 +284,7 @@ static String toPrettyFieldValueString(Schema.FieldType
fieldType, Object value,
FieldType elementType =
Objects.requireNonNull(fieldType.getCollectionElementType());
@SuppressWarnings("unchecked")
- List<Object> list = (List<Object>) value;
+ List<Object> list = Lists.newArrayList((Iterable<Object>) value);
Review Comment:
Good instinct, and I've taken it — but not with `Iterables.isEmpty` /
`Iterables.size`, because those would introduce a bug here. Details, since the
reason is easy to miss:
As the block stood it touched the collection **three** times — `isEmpty()`,
then `size()`, then the `for`. A `List` absorbs that for free, which is why it
was fine before. An `Iterable` that can only be read once does not, and that is
exactly the kind of value this PR widens support to. So the copy was
load-bearing rather than gratuitous.
I added a test that hands out its iterator exactly once and throws on a
second attempt. With your literal suggestion applied:
```
testToPrettyStringRendersAnIterableThatCanOnlyBeReadOnce
java.lang.IllegalStateException: iterated more than once
```
1 of 13 failing, and it's that one.
What does satisfy the point is rendering in a **single pass**, which drops
both the copy and the multi-traversal assumption:
```java
StringBuilder sb = new StringBuilder();
sb.append("[\n");
boolean empty = true;
for (Object element : elements) {
if (!empty) {
sb.append(",\n");
}
sb.append(nextPrefix).append(toPrettyFieldValueString(elementType,
element, nextPrefix));
empty = false;
}
if (empty) {
return "[]";
}
sb.append("\n").append(prefix).append("]");
```
Output is unchanged — same `",\n"` separators, and an empty collection still
renders as `[]`, which now has its own test too. `Lists` is no longer imported.
`spotlessJavaCheck`, `checkstyleMain` and `checkstyleTest` are clean, and
`SchemaUtilsTest` plus `RowTest` pass.
Pushed in f297bcf. Thanks for the look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
