minutechreview commented on issue #10478:
URL: https://github.com/apache/arrow-rs/issues/10478#issuecomment-5261669078
I'd like to pick this up, with one question on the intended shape of the fix.
My reading of `RunEndEncodedArrayDecoder::new`:
```rust
let decoder = ctx.make_decoder(
values_field.data_type(),
values_field.is_nullable() || is_nullable,
)?;
```
The `|| is_nullable` is the part that doesn't hold up. A `RunArray` has no
container-level validity buffer, so an outer "this REE field is nullable" can't
be represented at the REE level — propagating it downward silently relaxes the
values field's own nullability, which is exactly the smuggling you describe.
`values_field.is_nullable()` alone looks like the correct capability to hand
the child.
That covers the first half. The second half you raise is the part I'd rather
not guess at: even with the flag correctly `false`, decoders like
`StringArrayDecoder` ignore it, so nothing actually rejects a null that arrives
from the tape — whereas `StructArrayDecoder` catches it after the fact. So
dropping `|| is_nullable` alone would fix the schema-propagation bug but still
let a literal `null` in the JSON land in a non-nullable values array.
Which would you prefer?
1. **Minimal** — drop `|| is_nullable` so the child decoder gets the values
field's declared nullability, and add your repro as a regression test. Leaves
the "decoder ignores the flag" gap open as a separate concern.
2. **Also validate in the REE decoder** — additionally check the decoded
values array for nulls when `values_field.is_nullable()` is false and error
out, mirroring the post-hoc approach in `StructArrayDecoder`.
3. **Fix it at the child-decoder level** — make the primitive/string
decoders honour the flag they're given. Broader blast radius and presumably
affects more than REE, so I'd assume you'd want that separately.
I'd lean towards (2) as the self-contained fix, but this is your codebase's
convention rather than mine to set. Happy to start as soon as you point me at
one — and if someone else is already on it, no problem at all.
--
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]
