PDGGK opened a new pull request, #39587:
URL: https://github.com/apache/beam/pull/39587
`Row.toString()` throws `NullPointerException` whenever a null sits inside
an array, an iterable, a map or a nested row.
```java
Schema schema = Schema.builder().addArrayField("a",
FieldType.STRING.withNullable(true)).build();
Row.withSchema(schema).addValue(Arrays.asList("x", null)).build().toString();
// java.lang.NullPointerException: Cannot invoke "String.replace(...)"
because "string" is null
// at SchemaUtils.toPrettyFieldValueString(SchemaUtils.java:256)
```
`toPrettyRowString` skips a row's own null fields, but every recursive call
hands the raw element to `toPrettyFieldValueString`, which has no null check
and dereferences it — `string.replace` for `STRING`, `row.getValues()` for
`ROW`, `value.getClass()` for `ARRAY`/`MAP`. Only the numeric and boolean
branches survive, and only because `Objects.toString` already renders a null as
`"null"`.
These values are legal: `RowUtils` explicitly accepts null array elements
and null map values when the element type is nullable.
## The fix
Return `"null"` for a null value at the top of `toPrettyFieldValueString`.
That is not a new rendering — it is exactly what the numeric branches have
always produced, so this makes the remaining types consistent with the ones
that already worked. Seven lines, one file, no API or wire-format change.
`toPrettyRowString`'s deliberate skipping of top-level null fields is left
alone; that is the printer's intended terse style.
## Why this regressed
`Row.toString()` used to call `toString(true)`, whose private helper opens
with `if (value == null) { return "<null>"; }` and is therefore null-safe at
every depth. That method is still present. `Row.toString()` was rerouted to
`SchemaUtils.toPrettyString(this)` in #35150, and
`SchemaUtils.toPrettyString(this)` first appears in `Row.java` at `v2.69.0` —
it is absent from `v2.67.0` and `v2.68.0` — so every release from 2.69.0 on has
it.
`toString()` is called from logging, `PAssert` failure messages, exception
messages and debugger inspection, so in practice the NPE fires while a pipeline
is already reporting a different problem, and hides it.
`SchemaUtilsTest` is 107 lines and does not reference the pretty-printer at
all, which is why the 279 lines added in #35150 went unnoticed.
## Testing
Four tests added to `SchemaUtilsTest`: null inside an array of strings, a
null map value, a null row inside an array, and — as a control that pins the
existing behaviour — null inside an array of ints. The first three fail on
current `master` with the NPEs above and pass with this change; the control
passes both before and after, which is what makes the `"null"` rendering a
consistency fix rather than a new choice.
- `:sdks:java:core:test --tests "org.apache.beam.sdk.schemas.*Test"` with
`--rerun-tasks` — 522 tests, 0 failures, 0 errors
- `:sdks:java:core:spotlessJavaCheck` — passes
Fixes #21063
--
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]
