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)?
Keep full validation as the default across projection backends. Allow callers to select basic validation for trusted arrays through scan reader options.
gty404
changed the title
Summary
Validate Arrow batches before projection to reject malformed offsets. Add
ArrowValidationModewith Full validation enabled by default across projection backends; callers with trusted arrays can select Basic throughFileScanTaskReader::Options::arrow_validation_mode.Validation
data_test: 193/193 passed; pre-commit passed.