gty404 commented on code in PR #1043:
URL: https://github.com/apache/iceberg-cpp/pull/1043#discussion_r4332445581
##########
src/iceberg/arrow_c_data_util.cc:
##########
@@ -341,8 +341,14 @@ Result<ArrowArray> ProjectBatchNanoarrow(
internal::ArrowArrayViewGuard input_view_guard(&input_view);
ICEBERG_NANOARROW_RETURN_UNEXPECTED_WITH_ERROR(
ArrowArrayViewSetArray(&input_view, &input_batch, &error), error);
+ // FULL validation is required here: the append path below uses
+ // ArrowArrayViewGet{String,Bytes}Unsafe and raw list/map offset arithmetic,
+ // which nanoarrow only documents as memory-safe after FULL validation
+ // (DEFAULT does not check that every offsets-buffer entry is non-decreasing
+ // and in-bounds). This matches the FULL validation already performed on the
+ // 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),
Review 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 their documented safety
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
