Repository navigation
Conversation
ProjectBatchNanoarrow validated incoming batches at
NANOARROW_VALIDATION_LEVEL_DEFAULT before using
ArrowArrayViewGet{String,Bytes}Unsafe and raw list/map offset
arithmetic per row. nanoarrow documents these Unsafe accessors as
memory-safe only after FULL validation: DEFAULT only checks the
first/last offsets, not that every intermediate offset is
non-decreasing and in-bounds. A non-monotonic offset can produce a
negative size that overflows to a huge size_t in the append path, or
an out-of-range offset that gets copied into the output batch.
Raise the validation level to FULL, matching what
manifest_reader.cc already does for the same untrusted-batch
boundary.
b2b000b to
195d475
Compare
| // equivalent untrusted-batch boundary in manifest_reader.cc. | ||
| ICEBERG_NANOARROW_RETURN_UNEXPECTED_WITH_ERROR( | ||
| ArrowArrayViewValidate(&input_view, NANOARROW_VALIDATION_LEVEL_DEFAULT, &error), | ||
| ArrowArrayViewValidate(&input_view, NANOARROW_VALIDATION_LEVEL_FULL, &error), |
There was a problem hiding this comment.
ProjectBatch runs per batch on the scan path via file_scan_task_reader.cc:118, so FULL validation now walks every offsets buffer on every batch. Unlike the manifest_reader case these come from our own readers - is that cost intended?
There was a problem hiding this comment.
Good catch, the cost is real but small in absolute terms. I benchmarked ProjectBatch (nanoarrow path) on a 50k-row batch (int32 + string column) with DEFAULT vs FULL validation, averaged over 200 runs:
| selectivity | DEFAULT | FULL | overhead |
|---|---|---|---|
| 100% rows alive | ~4074 us/batch | ~4219 us/batch | +3.6% |
| 1% rows alive (heavy delete filtering) | ~44.5 us/batch | ~82.2 us/batch | +85% (~1.85x) |
So in the worst case (most rows filtered out by deletes, which is exactly where the row-append cost no longer dominates), FULL validation roughly doubles this step's time - but the absolute delta is ~38us for a 50k-row batch (~0.75ns/row), which should be negligible next to I/O/decode for a real scan.
Given nanoarrow has no bounds-checked "safe" counterpart to ArrowArrayViewGetStringUnsafe/GetBytesUnsafe in this pinned version, FULL batch-level validation is the only way to satisfy contract today. @wgtmac curious for your take: keep this as-is given the cost is small and bounded, or is there a reason to treat the in-tree reader path differently (e.g. skip FULL validation when the batch is known to come from our own Avro/Parquet readers)?
This branch has not been deployed
2 participants
Summary
ProjectBatchNanoarrowvalidated incoming batches atNANOARROW_VALIDATION_LEVEL_DEFAULTbefore usingArrowArrayViewGet{String,Bytes}Unsafeand raw list/map offset arithmetic per row. nanoarrow documents theseUnsafeaccessors as memory-safe only afterFULLvalidation:DEFAULTonly checks the first/last offsets, not that every intermediate offset is non-decreasing and in-bounds.size_tin the append path, or an out-of-range offset whose bytes get copied into the returned output batch.NANOARROW_VALIDATION_LEVEL_FULL, matching whatmanifest_reader.ccalready does for the same untrusted-batch boundary, closing the inconsistency between the two call sites.Test plan
ProjectBatchTest.RejectsNonMonotonicStringOffsets, which hand-assembles (via the nanoarrow C ABI, since Arrow's builders always emit monotonic offsets) a string array with offsets[0, 5, 2]: the first/last offsets are consistent with the 2-byte data buffer, soDEFAULTvalidation accepts it, butFULLvalidation correctly rejects it as non-monotonic.ProjectBatchreturned a value instead of an error) and passes with it../src/iceberg/test/data_test— 103/103 passed.pre-commit runon the changed files — passed.