feat(arrow-ipc) Add: Support FileReader and StreamReader skip array data validation

[totoroyyb]

Which issue does this PR close?

• Closes Improve Arrow-IPC performance by avoiding Unsafe Unchecked IPC Read RecordBatch #3287
• Closes Optionally disable data validation for arrow-ipc #6933

Rationale for this change

Beforehand, array data validation is performed for each array creation when reading an IPC file (or stream), which comes with a significant overhead. In some cases, this overhead is unwanted or the file content is trusted.

There are existing functions defined in the codebase to avoid data validation but this is not exposed to the upper level APIs. This PR brings options for both FileReader and StreamReader to disable it.

What changes are included in this PR?

• Add skip_validations, as an argument, to multiple functions signatures.
• Provide a new constructor try_new_unvalidated to FileReader
• Provide a new function with_skip_validations to StreamReader
• Various related minor changes.

Are there any user-facing changes?

No, I don't think so. There are no API-breaking changes, and essentially, two new APIs are introduced. Other changes are on the internal codes I believe.

[totoroyyb]

FYI, clippy does complain about too many arguments on one or two function signatures atm. I just want to give this PR a go for now.

[tustvold]

Unfortunately as implemented this is unsound, in that it permits UB via safe functions, this probably needs a bit more thought

[tustvold]

Technically all of these functions are now unsound and must be marked unsafe. This is not ideal, and probably needs a bit more thought into handling this better

[alamb]

One possibility might be to create a new function like:

fn unsafe create_array_unchecked(
    reader: &mut ArrayReader,
    field: &Field,
    variadic_counts: &mut VecDeque<i64>,
    require_alignment: bool,
    skip_validations: bool,
) -> Result<ArrayRef, ArrowError> {

And change the existing function to call it:

fn create_array(
    reader: &mut ArrayReader,
    field: &Field,
    variadic_counts: &mut VecDeque<i64>,
    require_alignment: bool,
) -> Result<ArrayRef, ArrowError> {
  
    let skip_validations = false;
    // safety: enable validatiions when checking
    unsafe { create_array_unchecked(reader, field, variadic_counts, require_aligment, skip_validations) };
}

This is kind of messy though -- finding some way to encapsulate the settings into a struct would be better

[totoroyyb]

Pls check the latest commit, which separating unsafe codes.

Each function like create_array, now has a unsafe version that skips validation. Codebase is not idea, but currently I don't have much better idea to encapusulate it.

[totoroyyb]

Refactor all unsafe codes into separate functions. It is a bit messy as mentioned @alamb mentioned. From bottom-to-up, there are many code movings and renames.

Now, FileReader has a new wrapper struct:

/// An iterator over the record batches (without validation) in an Arrow file
pub struct UnvalidatedFileReader<R: Read + Seek> {
    reader: FileReader<R>,
}

impl<R: Read + Seek> Iterator for UnvalidatedFileReader<R> {
    type Item = Result<RecordBatch, ArrowError>;

    fn next(&mut self) -> Option<Self::Item> {
        if self.reader.current_block < self.reader.total_blocks {
            // Use the unsafe `maybe_next_unvalidated` function
            unsafe {
                match self.reader.maybe_next_unvalidated() {
                    Ok(Some(batch)) => Some(Ok(batch)),
                    Ok(None) => None, // End of the file
                    Err(e) => Some(Err(e)),
                }
            }
        } else {
            None
        }
    }
}

This still unsound iteractor implementation, but I am not sure how to express it so that we can use iterator with validation disabled.

Any thoughts or suggestions?

[alamb]

Thanks @totoroyyb - I will try and review this PR in the next day or two.

[alamb]

I am starting to review this PR now -- thank you for your patience

[alamb]

First of all, thank you @totoroyyb -- I think this is a very nice feature and it would be great to get in. The current code is quite nicely docmented

At a minimum I think the PR needs

1. Tests: There should be tests that demonstrate that reading / writing data while skipping validation still works functionally. This includes at a minimum testing any new pub apis
2. Benchmarks: given the goal of this PR is to improve performance I think we need a benchmark to show it is actually improving the speed. Unfortunately, I wasn't able to find any existing arrow-ipc benchmarks, but can help draft one shortly

I would like to suggest we break it down a little differently to minimize API changes

1. Avoid adding ArrayDataBuilder::build_aligned_unchecked and instead simply add a flag on the builder. I will try and add a PR on this shortly
2. Consider adding flags to StreamReader and FilerReader rather than new structs like UnvalidatedStreamReader

[alamb]

This seems like it doesn't set the unvalidated flag. It seems like maybe we can direct the users to the `FileReaderBuilder API if they want to avoid validation

[totoroyyb]

This one was left here due to the changes I made in the last commit.

[alamb]

I don't understand the need for a whole different struct -- wouldn't a flag on FileReader (to mirror the flag on StreamReader) be more consistent?

[alamb]

TLDR is I think this is an important PR / feature @totoroyyb and I would like to help make it happen. I will make some initial PRs that hopefully move us forwards

Would you be willing to help write tests and benchmarks?

[totoroyyb]

@alamb Thanks for reviewing this. Yes, I'd be happy to help with benchmarks and test cases.

[totoroyyb]

@alamb Regarding adding a flag in the builder, which I did that way in the first commit. tustvold has suggested that we may need to mark unsafe for all functions that skip validation (here).

Seems like with a flag, there is no good way to mark it as unsafe for related functions. Let me know if I misunderstand something. Thanks!