Skip to content

Fix REE JSON decoder nullability (#10478) #10485

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Vam-si-krish wants to merge 1 commit into
base: main
Choose a base branch
from
+29 −12
Open

Fix REE JSON decoder nullability (#10478) #10485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions arrow-json/src/reader/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,9 +845,9 @@ fn make_decoder(
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)?)),

@Jefffrey Jefffrey Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mzabaluev mzabaluev Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Jefffrey Jefffrey Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds a bit odd 🤔

so a non-nullable REE but with a values field as nullable is considered valid?

DataType::Int32 => Ok(Box::new(RunEndEncodedArrayDecoder::<Int32Type>::new(ctx, data_type, is_nullable)?)),
DataType::Int64 => Ok(Box::new(RunEndEncodedArrayDecoder::<Int64Type>::new(ctx, data_type, is_nullable)?)),
DataType::Int16 => Ok(Box::new(RunEndEncodedArrayDecoder::<Int16Type>::new(ctx, data_type)?)),
DataType::Int32 => Ok(Box::new(RunEndEncodedArrayDecoder::<Int32Type>::new(ctx, data_type)?)),
DataType::Int64 => Ok(Box::new(RunEndEncodedArrayDecoder::<Int64Type>::new(ctx, data_type)?)),
d => unreachable!("unsupported run end index type: {d}"),
},
_ => Err(ArrowError::NotYetImplemented(format!("Support for {data_type} in JSON reader")))
Expand Down Expand Up @@ -3584,6 +3584,23 @@ mod tests {
assert_eq!(values.value(1), "y");
}

#[test]
fn test_read_run_end_encoded_nulls_in_non_nullable_values() {
let ree_type = DataType::RunEndEncoded(
Arc::new(Field::new("run_ends", DataType::Int32, false)),
Arc::new(Field::new("values", DataType::Utf8, false)),
);
let schema = Arc::new(Schema::new(vec![Field::new("a", ree_type, true)]));

for buf in [r#"{"a": null}"#, r#"{}"#] {
let mut decoder = ReaderBuilder::new(schema.clone()).build_decoder().unwrap();
decoder.decode(buf.as_bytes()).unwrap();
decoder
.flush()
.expect_err("nullable REE parent must not make values nullable");
}
}

#[test]
fn test_read_run_end_encoded_consecutive_nulls() {
let buf = r#"
Expand Down
18 changes: 9 additions & 9 deletions arrow-json/src/reader/run_end_array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,27 +33,22 @@ use crate::reader::{ArrayDecoder, DecoderContext};
pub struct RunEndEncodedArrayDecoder<R> {
data_type: DataType,
decoder: Box<dyn ArrayDecoder>,
values_nullable: bool,
phantom: PhantomData<R>,
}

impl<R: RunEndIndexType> RunEndEncodedArrayDecoder<R> {
pub fn new(
ctx: &DecoderContext,
data_type: &DataType,
is_nullable: bool,
) -> Result<Self, ArrowError> {
pub fn new(ctx: &DecoderContext, data_type: &DataType) -> Result<Self, ArrowError> {
let values_field = match data_type {
DataType::RunEndEncoded(_, v) => v,
_ => unreachable!(),
};
let decoder = ctx.make_decoder(
values_field.data_type(),
values_field.is_nullable() || is_nullable,
)?;
let decoder = ctx.make_decoder(values_field.data_type(), values_field.is_nullable())?;

Ok(Self {
data_type: data_type.clone(),
decoder,
values_nullable: values_field.is_nullable(),
phantom: Default::default(),
})
}
Expand All @@ -67,6 +62,11 @@ impl<R: RunEndIndexType + Send> ArrayDecoder for RunEndEncodedArrayDecoder<R> {
}

let flat_array = self.decoder.decode(tape, pos)?;
if !self.values_nullable && flat_array.null_count() != 0 {
return Err(ArrowError::JsonError(
"Encountered nulls in non-nullable RunEndEncoded values field".to_string(),
));
}

let partitions = partition(from_ref(&flat_array))?;
let size = partitions.len();
Expand Down
Loading