Add tests that arrow IPC data is validated #7096

alamb · 2025-02-07T17:48:26Z

Which issue does this PR close?

Part of Improve Arrow-IPC performance by avoiding Unsafe Unchecked IPC Read RecordBatch #3287

Rationale for this change

To test disabling validation in the IPC reader I first need to show there is validation actually occuring.

It is also probably good in general to have tests showing we validate data when read

What changes are included in this PR?

Add tests that show when invalid arrow data is written to IPC files, the StreamReader, FileReader and FileDecoder catch and verify those errors

Are there any user-facing changes?

This is entirely tests, no changes in functionality

Add tests for invalid arrays

alamb · 2025-02-07T17:48:49Z

arrow-ipc/src/reader.rs

@@ -1744,27 +1745,73 @@ mod tests {
        });
    }

-    fn roundtrip_ipc(rb: &RecordBatch) -> RecordBatch {
+    /// Write the record batch to an in-memory buffer in IPC File format


I just refactored these helpers into smaller chunks

alamb · 2025-02-07T17:50:07Z

arrow-ipc/src/reader.rs

+
+    /// Return the first record batch read from the IPC File buffer
+    /// using the FileDecoder API
+    fn read_ipc_with_decoder(buf: Vec<u8>) -> Result<RecordBatch, ArrowError> {


Every time I find myself using FileDecoder I end up copy/pasting the example.

I am starting to think having something like

arrow-rs/arrow/examples/zero_copy_ipc.rs

Lines 84 to 96 in 468e992

/// Incrementally decodes [`RecordBatch`]es from an IPC file stored in a Arrow

/// [`Buffer`] using the [`FileDecoder`] API.

///

/// This is a wrapper around the example in the `FileDecoder` which handles the

/// low level interaction with the Arrow IPC format.

struct IPCBufferDecoder {

/// Memory (or memory mapped) Buffer with the data

buffer: Buffer,

/// Decoder that reads Arrays that refers to the underlying buffers

decoder: FileDecoder,

/// Location of the batches within the buffer

batches: Vec<Block>,

}

in the actual crate might be useful

(given I am about to go copy/paste it again into comet...)

Here is another example:

Benchmarks for Arrow IPC reader #7091 (comment)

#7091 (comment)

alamb · 2025-02-07T17:51:57Z

arrow-ipc/src/reader.rs

@@ -2492,4 +2539,109 @@ mod tests {
                assert_eq!(decoded_batch.expect("Failed to read RecordBatch"), batch);
            });
    }
+
+    #[test]
+    fn test_validation_of_invalid_list_array() {


Any thoughts on how I can make an invalid PrimitiveArray would be most apprecaited.

Anything i tried to mismatch the len and the actual data, resulted in panics in bounds checks (even with the try_new_unchecked).

I think it is probably a good thing that it is so hard to create invalid arrays but it would be nice to test this path

ArrayData will normally let you do various inadvisable things

One thing I tried to do was make null buffers that had different sizes than the underlying values -- that actually failed the ArrayData creation checks, but when it was re-read via IPC the resulting arrays were fine.

Maybe it was due to padding or something. I'll play around with it some more

I concluded it was impossible to create an invalid PrimitiveArray -- when I made one that had invalid nulls, then I got a panic in the BooleanBufferConstructor here:

arrow-rs/arrow-buffer/src/buffer/boolean.rs

Line 65 in d4432c0

assert!(

I tried a more invasive change to add tests for validation for PrimitiveArrays and concluded that the current code will panic rather than error

Specifically, this code which generates invalid IPC data results in a panic on decode rather than an Result

#[test] fn test_validation_of_invalid_primitive_array() { // Int32Array with not enough nulls let array = unsafe { let nulls = NullBuffer::from(&[true, false, true, false]); let buffer = ScalarBuffer::<i32>::from_iter(0..8000); let data = ArrayDataBuilder::new(DataType::Int32) .len(8000) .add_buffer(buffer.into()) .nulls(Some(nulls)) // too few nulls .build_unchecked(); Int32Array::from(data) }; expect_ipc_validation_error( Arc::new(array), "Invalid argument error: Nulls do not match", ); }

Since I am not really trying to change / improve validation in this PR or project, I do not plan to change how it works (though I will file a ticket with the report of panic vs error)

filed

panic rather than failure when reading certain invalid arrow ipc streams #7124

alamb · 2025-02-07T17:52:27Z

arrow-ipc/src/reader.rs

+        let err = read_ipc(&buf).unwrap_err();
+        assert_eq!(err.to_string(), expected_err);
+
+        // TODO verify there is no error when validation is disabled


I plan to add a few more lines here when we disable validation

…tests

alamb · 2025-02-10T21:16:06Z

arrow-ipc/src/reader.rs

@@ -2492,4 +2539,109 @@ mod tests {
                assert_eq!(decoded_batch.expect("Failed to read RecordBatch"), batch);
            });
    }
+
+    #[test]
+    fn test_validation_of_invalid_list_array() {


I concluded it was impossible to create an invalid PrimitiveArray -- when I made one that had invalid nulls, then I got a panic in the BooleanBufferConstructor here:

arrow-rs/arrow-buffer/src/buffer/boolean.rs

Line 65 in d4432c0

assert!(

arrow-buffer/src/buffer/boolean.rs

This reverts commit 0b88bbc.

This reverts commit 8d885a1.

alamb · 2025-02-10T21:58:48Z

I think this PR is ready for review

tustvold · 2025-02-11T22:48:58Z

arrow-ipc/src/reader.rs

+        };
+        expect_ipc_validation_error(
+            Arc::new(array),
+            "Invalid argument error: Invalid UTF8 sequence at string index 3 (3..45): invalid utf-8 sequence of 1 bytes from index 38"


This error message is a bit peculiar, but that's not really something for this PR

alamb · 2025-02-12T11:44:41Z

Thanks again @tustvold

alamb added 2 commits February 7, 2025 12:39

Add tests for validating IPC data read/written

05f6861

Add tests for invalid arrays

Test with file decoder too

b961041

github-actions bot added the arrow Changes to the arrow crate label Feb 7, 2025

alamb commented Feb 7, 2025

View reviewed changes

alamb marked this pull request as ready for review February 7, 2025 17:53

alamb changed the title ~~Tests for IPC validation~~ Add tests that arrow IPC data is validated Feb 7, 2025

alamb added 4 commits February 8, 2025 11:34

Merge remote-tracking branch 'apache/main' into alamb/ipc_validation_…

247d27d

…tests

consolidate test

80372ea

Add test for test_validation_of_invalid_primitive_array

8d885a1

Rework ArrayData validation to return error rather than panic

0b88bbc

alamb commented Feb 10, 2025

View reviewed changes

alamb added 2 commits February 10, 2025 16:55

Revert "Rework ArrayData validation to return error rather than panic"

6fc755d

This reverts commit 0b88bbc.

Revert "Add test for test_validation_of_invalid_primitive_array"

7c663ac

This reverts commit 8d885a1.

alamb mentioned this pull request Feb 11, 2025

Add with_skip_validation flag to IPC StreamReader, FileReader and FileDecoder #7120

Open

tustvold approved these changes Feb 11, 2025

View reviewed changes

tustvold reviewed Feb 11, 2025

View reviewed changes

alamb merged commit 78c9df9 into apache:main Feb 12, 2025
26 checks passed

alamb deleted the alamb/ipc_validation_tests branch February 12, 2025 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests that arrow IPC data is validated #7096

Add tests that arrow IPC data is validated #7096

alamb commented Feb 7, 2025 •

edited

Loading

alamb Feb 7, 2025

alamb Feb 7, 2025

alamb Feb 8, 2025

tustvold Feb 11, 2025

alamb Feb 7, 2025

tustvold Feb 7, 2025

alamb Feb 7, 2025

alamb Feb 10, 2025

alamb Feb 10, 2025

alamb Feb 12, 2025

alamb Feb 7, 2025

alamb Feb 10, 2025

alamb commented Feb 10, 2025

tustvold Feb 11, 2025

alamb commented Feb 12, 2025

	/// Incrementally decodes [`RecordBatch`]es from an IPC file stored in a Arrow
	/// [`Buffer`] using the [`FileDecoder`] API.
	///
	/// This is a wrapper around the example in the `FileDecoder` which handles the
	/// low level interaction with the Arrow IPC format.
	struct IPCBufferDecoder {
	/// Memory (or memory mapped) Buffer with the data
	buffer: Buffer,
	/// Decoder that reads Arrays that refers to the underlying buffers
	decoder: FileDecoder,
	/// Location of the batches within the buffer
	batches: Vec<Block>,
	}

Add tests that arrow IPC data is validated #7096

Add tests that arrow IPC data is validated #7096

Conversation

alamb commented Feb 7, 2025 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Feb 10, 2025

Choose a reason for hiding this comment

alamb commented Feb 12, 2025

alamb commented Feb 7, 2025 •

edited

Loading