Fix: Map functions crash on out of bounds cases #16203

krishvishal · 2025-05-28T03:08:58Z

Which issue does this PR close?

Closes List of structures crashes on out of bounds cases if struct has null value #16187

Rationale for this change

This PR fixes a panic crash in array indexing operations when accessing elements from map_values() results that contain structs with Null-typed fields.

What changes are included in this PR?

datafusion/functions-nested/src/extract.rs: Added null field detection and safe handling

Are these changes tested?

Yes - unit tests are added covering:

Main crash scenario: Struct with Null-typed fields
Mixed null/valid fields, out-of-bounds access

Are there any user-facing changes?

No, only behavior change.

Detect if array elements contain structs with null-typed fields and emit a proper null array instead of trying to extract malformed data. This prevents Arrow's NullArray buffer validation from panicking.

1. Reproduce the state in the issue and test the fix has worked. 2. Case where the array has mixed null fields. 3. Case with out-of-bounds array access.

Copilot

Pull Request Overview

This PR resolves a crash occurring during array indexing when map functions encounter structs with Null-typed fields. Key changes include adding detection of structs containing Null fields in extract.rs and returning a properly typed null array, along with comprehensive tests for the crash scenario, mixed null/valid fields, and out-of-bounds access.

Copilot · 2025-05-28T15:15:16Z

datafusion/functions-nested/src/extract.rs

+            let mut null_array_data = Vec::with_capacity(array.len());
+            for _ in 0..array.len() {
+                null_array_data.push(None::<i32>);
+            }
+


The variable 'null_array_data' is initialized and populated but never used, which may lead to confusion. Consider removing this unused code to simplify the function.

Suggested change

let mut null_array_data = Vec::with_capacity(array.len());

for _ in 0..array.len() {

null_array_data.push(None::<i32>);

}

comphead · 2025-05-28T15:19:43Z

datafusion/functions-nested/src/extract.rs

@@ -225,6 +225,23 @@ where
        return Ok(Arc::new(NullArray::new(array.len())));
    }

+    if let DataType::Struct(fields) = values.data_type() {
+        let has_null_fields = fields.iter().any(|field| field.data_type() == &Null);


Please help me to understand why we check by structure field types?

oh, now I realize the problem happens only if the struct has with with null as the value

comphead · 2025-05-28T15:20:27Z

datafusion/functions-nested/src/extract.rs

+        let has_null_fields = fields.iter().any(|field| field.data_type() == &Null);
+        if has_null_fields {
+            // Instead of trying to extract from malformed struct data and return appropriate nulls
+            let mut null_array_data = Vec::with_capacity(array.len());


to return I think we can return NullArray like here?

if values.data_type().is_null() { return Ok(Arc::new(NullArray::new(array.len()))); }

comphead

Thanks @krishvishal
Please add SQL tests as well to array.slt file
what comes to my mind is

select [named_struct('a', 1, 'b', null)][0];
select [named_struct('a', 1, 'b', null)][1];
select [named_struct('a', 1, 'b', null)][-1];
select [named_struct('a', 1, 'b', null)][2]
select map_values(map([named_struct('a', 1, 'b', null)], [named_struct('a', 2, 'b', null)]))[0] 
select map_values(map([named_struct('a', 1, 'b', null)], [named_struct('a', 2, 'b', null)]))[1]
select map_values(map([named_struct('a', 1, 'b', null)], [named_struct('a', 2, 'b', null)]))[-1]
select map_values(map([named_struct('a', 1, 'b', null)], [named_struct('a', 2, 'b', null)]))[2]   
select map_keys(map([named_struct('a', 1, 'b', null)], [named_struct('a', 2, 'b', null)]))[0] 
select map_keys(map([named_struct('a', 1, 'b', null)], [named_struct('a', 2, 'b', null)]))[1]
select map_keys(map([named_struct('a', 1, 'b', null)], [named_struct('a', 2, 'b', null)]))[-1]
select map_keys(map([named_struct('a', 1, 'b', null)], [named_struct('a', 2, 'b', null)]))[2]   
;

comphead · 2025-06-02T19:07:51Z

@krishvishal are you still planning to wrap this PR up?

returning all nulls Improves upon the previous fix that detected null-typed fields in structs but incorrectly returned all null results. Now properly extracts valid struct elements while handling null-typed fields gracefully. Changes: - Replace blanket null return with handle_struct_with_null_fields() - Process each struct field individually using MutableArrayData for valid fields - Create proper NullArrays only for null-typed fields - Maintain correct null buffer for the overall result This allows queries like [named_struct('a', 1, 'b', null)][1] to correctly return {a: 1, b: NULL} instead of NULL. Fixes: apache#16187

acess of struct arrays with null fields

krishvishal · 2025-06-03T03:47:48Z

@comphead I've changed the implementation a bit to handle nulls properly. Previous fix just outputs NULL for queries like select [named_struct('a', 1, 'b', null)][1]; instead of {a: 1, b: NULL} .

I've also added tests to .array.slt file.

I think its ready.

Apologies for the delay.

comphead · 2025-06-03T19:56:17Z

Thanks @krishvishal the latest version becomes much more complicated compared to prev one. This can be a subject to check the performance.

What is the reason for adding the specific handler, some tests not passing?

krishvishal · 2025-06-03T20:52:50Z

@comphead I've had to add the handler because the previous fix caused wrong behavior.

For example the following query currently returns:

> select [named_struct('a', 1, 'b', null)][1];

+-----------------------------------------------------------------------+
| make_array(named_struct(Utf8("a"),Int64(1),Utf8("b"),NULL))[Int64(1)] |
+-----------------------------------------------------------------------+
| {a: 1, b: NULL}                                                       |
+-----------------------------------------------------------------------+
1 row(s) fetched. 
Elapsed 0.007 seconds.

This is correct. But my previous fix (commit: d9a699f) returned NULL. So to fix the behavior while also handling out of bounds indexes, I've had to add this handler.

comphead · 2025-06-03T21:05:41Z

datafusion/functions-nested/src/extract.rs

    for (row_index, offset_window) in array.offsets().windows(2).enumerate() {
        let start = offset_window[0];
        let end = offset_window[1];
        let len = end - start;

-        // array is null
-        if len == O::usize_as(0) {
+        if array.is_null(row_index) || len == O::usize_as(0) {


seems these 2 if statements can be merged into 1?

From my understanding

is_null() checks if an element at an index is NULL.

2nd condition checks if the element is an empty list.

They are checking different things right?

right I had a feeling we can merge them like

if array.is_null(row_index) || len == O::usize_as(0) || indexes.is_null(row_index) { mutable.extend_nulls(1); continue; }

Oh, I understand what you mean. Yes they can be merged.

comphead · 2025-06-03T21:06:18Z

datafusion/functions-nested/src/extract.rs

+    i64: TryInto<O>,
+{
+    let index: O = index.try_into().map_err(|_| {
+        DataFusionError::Execution(format!("array_element got invalid index: {index}"))


Suggested change

DataFusionError::Execution(format!("array_element got invalid index: {index}"))

exec_err!("array_element got invalid index: {index}")

- merge conditionals - use `exec_err!` - Fix warning raised by clippy

krishvishal · 2025-06-04T08:20:32Z

@comphead I've pushed additional fixes you've asked. I think the PR is ready. Let me know if it is okay.

comphead · 2025-06-04T15:53:10Z

Thanks @krishvishal let me quickly double check if we can do it with less data massaging. Since this will happen on execution layer it would be called for every batch of data possibly hitting the performance

krishvishal · 2025-06-11T18:41:15Z

@comphead, can you please tell if there is something I can do to move this forward? Are there any relevant benches I could either run or adapt for this case?

comphead · 2025-06-17T20:18:05Z

@comphead, can you please tell if there is something I can do to move this forward? Are there any relevant benches I could either run or adapt for this case?

Sorry I didn't have a chance to look into that, I'm planning to do it this week

krishvishal added 3 commits May 28, 2025 08:14

Fix Arrow null validation panic by detecting null fields in structs

32e1bee

Detect if array elements contain structs with null-typed fields and emit a proper null array instead of trying to extract malformed data. This prevents Arrow's NullArray buffer validation from panicking.

Add tests for following scenarios:

3be9229

1. Reproduce the state in the issue and test the fix has worked. 2. Case where the array has mixed null fields. 3. Case with out-of-bounds array access.

Silence complaints about unnecessary qualifications.

d9a699f

krishvishal mentioned this pull request May 28, 2025

List of structures crashes on out of bounds cases if struct has null value #16187

Open

Merge branch 'main' into fix-issue-16187

1cd0f4c

comphead requested a review from Copilot May 28, 2025 15:14

Copilot AI reviewed May 28, 2025

View reviewed changes

comphead reviewed May 28, 2025

View reviewed changes

mbutrovich mentioned this pull request May 29, 2025

chore: enable map_values testing since we fall back on nested types for defa… apache/datafusion-comet#1813

Closed

krishvishal added 2 commits June 3, 2025 09:09

Add tests to array.slt that cover cases of out of the bounds

422149b

acess of struct arrays with null fields

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jun 3, 2025

comphead reviewed Jun 3, 2025

View reviewed changes

Fix issues raised by review:

863c4bd

- merge conditionals - use `exec_err!` - Fix warning raised by clippy

	DataFusionError::Execution(format!("array_element got invalid index: {index}"))
	exec_err!("array_element got invalid index: {index}")

Fix: Map functions crash on out of bounds cases #16203

Are you sure you want to change the base?

Fix: Map functions crash on out of bounds cases #16203

Conversation

krishvishal commented May 28, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

comphead commented Jun 2, 2025

Uh oh!

krishvishal commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comphead commented Jun 3, 2025

Uh oh!

krishvishal commented Jun 3, 2025

Uh oh!

comphead Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krishvishal Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krishvishal commented Jun 4, 2025

Uh oh!

comphead commented Jun 4, 2025

Uh oh!

krishvishal commented Jun 11, 2025

Uh oh!

comphead commented Jun 17, 2025

Uh oh!

Uh oh!

krishvishal commented Jun 3, 2025 •

edited

Loading

comphead Jun 3, 2025 •

edited

Loading

krishvishal Jun 3, 2025 •

edited

Loading