Repository navigation
feat(ipc): Allow the stream reader's validation level to be configured - #964
Conversation
ArrowIpcArrayStreamReaderOptions gains a validation_level field, used for every record batch and dictionary batch the reader decodes. Passing NULL options keeps the current behaviour (NANOARROW_VALIDATION_LEVEL_FULL). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #964 +/- ##
=======================================
Coverage 78.24% 78.25%
=======================================
Files 106 106
Lines 16867 16909 +42
Branches 1993 2014 +21
=======================================
+ Hits 13198 13232 +34
+ Misses 2437 2435 -2
- Partials 1232 1242 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Vendor nanoarrow from rustyconover/arrow-nanoarrow haybarn/vendor, which adds the IPC stream reader changes proposed in apache/arrow-nanoarrow#964 and #965. The reader now references message bodies in the collected stream bytes instead of copying them, and checks buffer sizes without scanning every offset, as Arrow C++ did. Arrow inserts are back to the speed of the Arrow C++ build. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you!
We do need to fix the clang-tidy objection before merging but this is a great feature to have and I think speeds things up considerably in a lot of cases
| struct ArrowIpcArrayStreamReaderOptions options; | ||
| options.field_index = -1; | ||
| options.use_shared_buffers = 0; | ||
| options.validation_level = static_cast<ArrowValidationLevel>(4); | ||
| ASSERT_EQ(ArrowIpcArrayStreamReaderInit(&stream, &input, &options), EINVAL); |
There was a problem hiding this comment.
clang-tidy objects to 4 here since it's an invalid value. I think you can add a clang-tidy ignore line, but if not, you can also remove this test.
There was a problem hiding this comment.
Added a NOLINTNEXTLINE(clang-analyzer-optin.core.EnumCastOutOfRange) so the test can keep exercising the EINVAL path. I checked with clang-tidy 19: the warning shows up without the comment and is gone with it.
| if (options != NULL && | ||
| ((int)options->validation_level < (int)NANOARROW_VALIDATION_LEVEL_NONE || | ||
| (int)options->validation_level > (int)NANOARROW_VALIDATION_LEVEL_FULL)) { | ||
| return EINVAL; | ||
| } |
There was a problem hiding this comment.
a switch() is what we do in other places for enum validation and I think is just as compact here
There was a problem hiding this comment.
Done, switched to a switch over the four levels with default: return EINVAL.
…the invalid-value test Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ArrowIpcArrayStreamReaderalways decodes withNANOARROW_VALIDATION_LEVEL_FULL. That's the right default for bytes of unknown origin. But full validation walks every offset of every string, binary and list column and every dictionary index, and a caller that already trusts its producer can't opt out of that pass. The lower-level decoder functions already take a validation level; the stream reader just doesn't expose one.This adds a
validation_levelfield toArrowIpcArrayStreamReaderOptions. It applies to both record batches and dictionary batches. PassingNULLoptions keeps the current behaviour (FULL). An out-of-range value makesArrowIpcArrayStreamReaderInit()returnEINVAL.Motivation: I'm moving a WebAssembly build of DuckDB from Arrow C++ to nanoarrow. There, Arrow IPC streams written by Arrow JS in the same page are read into the database. Arrow C++'s reader doesn't do full validation by default, and the extra pass was a large part of an insert slowdown I measured after switching.
Reading a 23 MB stream from an in-memory buffer (1M rows of int32, double and string, in 16 batches), Release build, Apple M3, median of 50 runs:
FULL(current behaviour)DEFAULTThe float64-only fixtures from
dev/benchmarksdon't change, because full validation has nothing extra to check for them.Compatibility: callers that fill in
ArrowIpcArrayStreamReaderOptionsfield by field need to set the new field as well. That's the same situation as whenuse_shared_bufferswas added in #165. The R and Python bindings passNULL, so they're unaffected. TheEINVALcheck should catch most callers that miss the new field, rather than letting them silently get a random level.Tests:
StreamReaderValidationLevelpoints a dictionary index past the end of the dictionary in the existing dictionary fixture, then checks thatFULLrejects the batch andDEFAULTaccepts it.StreamReaderInvalidValidationLevelcovers theEINVALpath. The two existing tests that build options by hand now set the field. ctest passes locally (macOS arm64, Debug, extra warnings). Under ASan/UBSan the IPC tests pass, and the only failures are the same six allocation tests that also fail onmainunder ASan.🤖 Generated with Claude Code