Fix REE JSON decoder nullability (#10478) - #10485
Open
Vam-si-krish wants to merge 1 commit into
Open
Conversation
Jefffrey
reviewed
Jul 30, 2026
| DataType::BinaryView => Ok(Box::new(BinaryViewDecoder::default())), | ||
| DataType::Map(_, _) => Ok(Box::new(MapArrayDecoder::new(ctx, data_type, is_nullable)?)), | ||
| DataType::RunEndEncoded(ref r, _) => match r.data_type() { | ||
| DataType::Int16 => Ok(Box::new(RunEndEncodedArrayDecoder::<Int16Type>::new(ctx, data_type, is_nullable)?)), |
Contributor
There was a problem hiding this comment.
this ignoring of the passed in is_nullable seems significant; is it safe to do this? 🤔
cant help but feel this could lead to another edge case
Contributor
There was a problem hiding this comment.
Yes; the declared nullability of the REE-typed field has no bearing on whether the values array is allowed to carry nulls. That's determined by the schema of the "values" field.
Contributor
|
Here's the full test case from my unsubmitted branch (so you may want to adjust the error strings here or in your code). Disclaimier: written by Claude. #[test]
fn test_read_run_end_encoded_nulls_in_non_nullable_values() {
// A `RunArray` has no validity buffer of its own, so unlike a struct child
// there is no parent-level mask that could make these nulls representable.
// Declaring the enclosing field nullable does not change that.
let ree_type = DataType::RunEndEncoded(
Arc::new(Field::new("run_ends", DataType::Int32, false)),
Arc::new(Field::new("values", DataType::Utf8, false)),
);
for outer_nullable in [false, true] {
let schema = Arc::new(Schema::new(vec![Field::new(
"a",
ree_type.clone(),
outer_nullable,
)]));
// An explicit null, and a missing field, are both rejected.
for buf in [
r#"
{"a": "x"}
{"a": null}
{"a": "y"}
"#,
r#"
{"a": "x"}
{}
{"a": "y"}
"#,
] {
let mut decoder = ReaderBuilder::new(schema.clone()).build_decoder().unwrap();
let err = decoder
.decode(buf.as_bytes())
.and_then(|_| decoder.flush())
.expect_err("nulls in non-nullable REE values field");
assert!(
err.to_string()
.contains("Encountered nulls in non-nullable values of RunEndEncoded"),
"unexpected error: {err}"
);
}
}
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AI-generated contribution disclosure: I used OpenAI Codex to help prepare the implementation and regression test. I reviewed and understand the changes, verified them against the issue, and ran the project checks below. The generated portions are the decoder nullability guard, constructor wiring, and regression test.
Summary
This intentionally tightens schema enforcement as described in the issue.
Closes #10478
Testing
cargo test -p arrow-json test_read_run_end_encoded_nulls_in_non_nullable_valuescargo clippy -p arrow-json --all-targets -- -D warningscargo fmt --all