kamcheungting-db commented on PR #1083:
URL: https://github.com/apache/iceberg-cpp/pull/1083#issuecomment-6805037620
Thanks for the PR! The design looks right: validation runs once per batch
(not per row), `NANOARROW_VALIDATION_LEVEL_FULL` matches the projection path
from #1043, the nested wrappers in `ExtractValue` correctly skip re-validation,
and the default matches the property's default.
A few things while rebasing onto main (the PR is conflicting; #1065
restructured `file_scan_task_reader.cc`):
**1. Thread the property into residual evaluation too**
#1065 added `TaskStreamSource::KeepResidualMatches` in
`src/iceberg/data/file_scan_task_reader.cc`, which wraps each batch with:
```cpp
ArrowArrayStructLike::Make(input_arrow_schema, input_batch);
```
With this PR's default `validate_batch=true`,
`read.arrow.validate-batch=false` would be honored for delete filtering
(`ComputeAliveRows`) and equality-delete loading (`LoadEqualityDeletes`), but
residual evaluation would still run full validation — all three consume the
same buffers, so the property's contract would become path-dependent.
Suggested fix: pass `properties_.Get(ReaderProperties::kArrowValidateBatch)`
from `Open()` into `TaskStreamSource`, store it as a member, and pass it to
that `Make` call:
```cpp
ICEBERG_ASSIGN_OR_RAISE(
auto row, ArrowArrayStructLike::Make(input_arrow_schema, input_batch,
/*row_index=*/0, validate_batch_));
```
**2. Minor**
- Please document `validate_batch` on the three `Make` declarations (as
noted in the thread), e.g.:
```
/// \param validate_batch Whether to fully validate the array's buffers and
/// children (offsets, lengths, buffer sizes) when the view is set.
Disable
/// only for input that is already validated or trusted.
```
- `bool validate_batch_;` in the three `Impl` classes could be `const bool`,
matching `DeleteFilter` and `DeleteLoader` in this PR.
- Now that the property also governs delete filtering and equality-delete
loading, the `kArrowValidateBatch` comment in `file_reader.h` ("before
merge-on-read projection") could be broadened.
- The new tests construct `DeleteFilter` directly; one end-to-end test
through `FileScanTaskReader::Open` with `read.arrow.validate-batch=false` would
also cover the production wiring.
**Question (not blocking):** with defaults on, a merge-on-read batch is now
fully validated twice — once in the delete-filter wrapper and once in
`ProjectBatch` (#1043). That seems acceptable, but validating once at the
stream entry might be leaner long-term.
--
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]
