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:
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.
##########
src/paimon/core/manifest/manifest_file.cpp:
##########
@@ -93,8 +93,9 @@ Status ManifestFile::ReadBucketEntries(const std::string&
file_name, int32_t buc
file_name,
[this, bucket, entries](const std::shared_ptr<arrow::StructArray>&
batch) -> Status {
const arrow::ArrayVector& fields = batch->fields();
+ ColumnarRow row(fields, pool_, /*row_id=*/0);
Review Comment:
This reuse looks correct to me — `GetRow`/`GetArray`/`GetMap` snapshot
`row_id_` at call time, nothing on this path calls `SetRowKind`, and it matches
Java's `ColumnarRow.setRowId`.
One gap though: the exact same pattern lives in the base class at
`src/paimon/core/utils/objects_file.h:139`, where `ObjectsFile<T>::Read()`
still constructs a `ColumnarRow` per row inside its callback. `ManifestFile` is
an `ObjectsFile<ManifestEntry>` and `ReadBucketEntries` is its narrower
bucket-filtered variant, while `ObjectsFile<T>::Read` is the shared path for
full manifest reads, manifest lists and index manifests
(`file_store_scan.cpp:266` and `:491`, `manifest_file_merger.cpp:178`,
`commit_scanner.cpp:122`, `expire_snapshots.cpp:301`). Those two are the only
per-row `ColumnarRow` constructions in non-test code.
Could you apply the same one-line change there as well? Otherwise the
broader read path misses the optimization and the two sibling loops diverge.
--
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]
