Skip to content

feat(ipc): Allow the stream reader's validation level to be configured - #964

Merged
paleolimbot merged 2 commits into
apache:mainfrom
rustyconover:ipc-reader-validation-level
Oct 17, 2026
Merged

paleolimbot merged 2 commits into
apache:mainfrom
rustyconover:ipc-reader-validation-level

Conversation

@rustyconover

Copy link
Copy Markdown
Contributor

ArrowIpcArrayStreamReader always decodes with NANOARROW_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_level field to ArrowIpcArrayStreamReaderOptions. It applies to both record batches and dictionary batches. Passing NULL options keeps the current behaviour (FULL). An out-of-range value makes ArrowIpcArrayStreamReaderInit() return EINVAL.

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:

validation level time
FULL (current behaviour) 0.66 ms
DEFAULT 0.38 ms

The float64-only fixtures from dev/benchmarks don't change, because full validation has nothing extra to check for them.

Compatibility: callers that fill in ArrowIpcArrayStreamReaderOptions field by field need to set the new field as well. That's the same situation as when use_shared_buffers was added in #165. The R and Python bindings pass NULL, so they're unaffected. The EINVAL check should catch most callers that miss the new field, rather than letting them silently get a random level.

Tests: StreamReaderValidationLevel points a dictionary index past the end of the dictionary in the existing dictionary fixture, then checks that FULL rejects the batch and DEFAULT accepts it. StreamReaderInvalidValidationLevel covers the EINVAL path. 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 on main under ASan.

🤖 Generated with Claude Code

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-commenter

codecov-commenter commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.25%. Comparing base (ea215b0) to head (4fea478).
⚠️ Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

rustyconover added a commit to Query-farm-haybarn/haybarn-wasm that referenced this pull request Oct 16, 2026
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 paleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment on lines +648 to +652
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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment thread src/nanoarrow/ipc/reader.c Outdated
Comment on lines +577 to +581
if (options != NULL &&
((int)options->validation_level < (int)NANOARROW_VALIDATION_LEVEL_NONE ||
(int)options->validation_level > (int)NANOARROW_VALIDATION_LEVEL_FULL)) {
return EINVAL;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a switch() is what we do in other places for enum validation and I think is just as compact here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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>

@paleolimbot paleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@paleolimbot
paleolimbot merged commit cc67c6b into apache:main Oct 17, 2026
43 checks passed
@rustyconover
rustyconover deleted the ipc-reader-validation-level branch October 17, 2026 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants