zjw1111 commented on code in PR #242:
URL: https://github.com/apache/paimon-cpp/pull/242#discussion_r3849802698
##########
src/paimon/format/avro/avro_file_batch_reader.cpp:
##########
@@ -123,7 +128,7 @@ Result<BatchReader::ReadBatch>
AvroFileBatchReader::NextBatch() {
}
PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr<arrow::Array> array,
array_builder_->Finish());
- PAIMON_RETURN_NOT_OK_FROM_ARROW(array->Validate());
+ assert(array->Validate().ok());
Review Comment:
One-Sentence Summary: Please revert to the #ifndef pattern. I've already
discussed this with @lxy-9602 .
-----
Moving this into `assert` means that under `NDEBUG` the whole expression is
elided and `Validate()` is never called at all. `CMakeLists.txt:37-42` defaults
`CMAKE_BUILD_TYPE` to `Release`, and
`cmake_modules/SetupCxxFlags.cmake:383-384` compiles Release with `-DNDEBUG`,
so the check disappears from the default build. In debug builds the failure
mode also changes from a recoverable `Status::Invalid` to `abort()`.
Two things make that a bit riskier than it looks:
- Arrow's `ExportArray`/`ImportArray` do not run `Array::Validate()`
themselves, so this was the only structural check on the avro -> Arrow -> C
Data Interface path. Downstream, `ColumnarRow::GetXxx()` indexes by `row_id`
without bounds checks.
- The sibling readers still keep it: `parquet_file_batch_reader.cpp:619` and
`orc_adapter.cpp:943` both use
`PAIMON_RETURN_NOT_OK_FROM_ARROW(array->Validate())`, so avro becomes the only
exception.
Also, the cost here scales with the number of schema nodes rather than the
row count (element-wise scanning only happens in `ValidateFull`), and it runs
once per batch (`read.batch-size` defaults to 1024) — so the saving may be
smaller than expected.
Would it be possible to either keep the `Status`-returning check, or, if
profiling shows it really is a hotspot, turn it into an explicit debug-only
block with a short comment and apply the same treatment to the parquet/orc
readers so the three stay consistent? If you already have measurements for
this, adding them to the PR description would help.
--
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]
