PDGGK opened a new pull request, #39750:
URL: https://github.com/apache/beam/pull/39750
Fixes #39749.
`Row#toString` throws for an `ITERABLE` field holding a plain `Iterable`:
```java
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 was stricter than the type it
guards, and the branch below only ever iterates and counts — both fine from an
`Iterable` once materialised.
This is reached from `Row#toString`, so refusing to render one field takes
out logging and debugger output for every field beside it. That is a poor trade
for a stricter check in a renderer.
The guard now requires `Iterable`; a value that is neither still throws the
same exception.
### On where this belongs
A reviewer could reasonably hold that a materialised `Row` should always
store a `List` for `ARRAY`/`ITERABLE`, and that a non-`List` reaching here
means a producer is at fault — `ByteBuddyUtils.transformContainer` hands back a
`Collections2.TransformedCollection` for a `Set`-typed POJO field, for
instance. I think that is worth looking at separately and have not touched it.
What makes this worth fixing on its own is that the reproducer needs only
`Schema.builder`, `attachValues` and `toString()` — no POJO, no registry, no
pipeline — so the renderer is reachable with a merely-`Iterable` value through
plain public API. And a `toString()` that throws is difficult to justify
whatever produced the value.
I should be straight about the blast radius rather than inflate it: I could
**not** construct a case where a non-`List` reaches user code inside a running
pipeline. Coder round-trips materialise, and `Convert.toRows()` / `@Element
Row` both yield `List`s. The demonstrated exposure is direct API use.
### Tests
| case | on master |
|---|---|
| bare `Iterable` on an `ITERABLE` field | **fails** with the
`IllegalArgumentException` above |
| ordinary `List` on an `ARRAY` field | passes — the control |
The first also asserts `!(row.getValue("vals") instanceof List)` **before**
rendering, so if the value ever starts arriving materialised the test fails
loudly rather than going green for the wrong reason.
11/11 in `SchemaUtilsTest`, and the wider `*schemas*` / `*RowTest*` suites
pass. `spotlessCheck`, `checkstyleMain`, `checkstyleTest` clean.
--
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]
