You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "lucasfang (via GitHub)" <gi...@apache.org> on 2026/09/09 06:39:58 UTC
[PR] perf(parquet): size Arrow builders by selected rows in page-filtered reads [paimon-cpp]
lucasfang opened a new pull request, #309:
URL: https://github.com/apache/paimon-cpp/pull/309
### Purpose
Linked issue: close #308
A page-filtered read resets each leaf with `ColumnReader::ResetLeaf(col_idx, total)`, where `total` is the leaf's whole-row-group size in compressed space, and that one number drives `RecordReader::Reserve(total)`. On a selective predicate that appends a few rows out of a large row group this mis-sizes the Arrow builders both ways: a variable-width (BYTE_ARRAY) leaf reserves its `::arrow::BinaryBuilder` offsets buffer for the entire row group (tens of MiB never written), while the value-data buffer is not reserved at all because `Reserve()` leaves it alone for BYTE_ARRAY readers (`uses_values_` is false), so it doubles once per decoded batch and each doubling copies everything accumulated so far.
This PR splits the single `reserve` into `reserve_records` / `reserve_values` / `reserve_value_bytes` and threads them from the format layer into the patched Arrow, so each leaf sizes its builders once for the selection. Output is unchanged — the reservation is only a capacity hint.
Changes:
- `cmake_modules/arrow.diff`: widen `parquet::arrow::ColumnReader::ResetLeaf` from `(col_idx, reserve)` to `(col_idx, reserve_records, reserve_values, reserve_value_bytes)`, threaded through `LeafReader`, `ExtensionReader`, `ListReader`, and `StructReader`; `LeafReader::ResetLeaf` now calls `Reserve(reserve_records)` plus `ReserveValueBytes(reserve_values, reserve_value_bytes)`. Add the `parquet::RecordReader::ReserveValueBytes(num_values, num_bytes)` virtual (default no-op for fixed-width readers) and a `ByteArrayChunkedRecordReader` override that reserves the builder offsets and data buffer, clamped to `kBinaryMemoryLimit - value_data_length()` to mirror `ArrowBinaryHelper::Prepare`.
- `page_filtered_row_group_reader.h` / `.cpp`: `ExecuteSkipReadPattern` gains `reserve_values` / `reserve_value_bytes`; `ReadFilteredField` computes them per leaf — `reserve_values = effective_ranges.RowCount()` and `reserve_value_bytes = min(avg_width * reserve_values, total_uncompressed_size)` from the column chunk, an over-estimate capped by the chunk and guarded by `num_values() > 0 && reserve_values > 0` — and fetches `column_chunk` whenever the selection is non-empty, not only on the page-index path.
### Tests
No new test: the change preserves output and the existing `page_filtered_row_group_reader_test.cpp` already covers every path it touches — `StringColumnPageFilter` and `StringColumnMultiBatchStreaming` (multi-batch BYTE_ARRAY selective reads with per-value content assertions, the doubling case `ReserveValueBytes` removes), the nested struct/list/map cases that thread `ResetLeaf` through `StructReader` / `ListReader`, and the dictionary cases. A reservation-size assertion would be brittle and couple to implementation details.
Verified against a bundled Arrow re-patched with the updated `arrow.diff` (Release, `-j 96`):
- `paimon-parquet-format-test`: 220 tests from 16 suites passed, including all 49 `PageFilteredRowGroupReaderTest` cases.
- `paimon-common-test`: 1551 tests from 179 suites passed.
### API and Format
No. No header under `include/paimon/` is touched and neither the storage format nor the protocol changes. The only API affected is paimon's own vendored Arrow patch (`cmake_modules/arrow.diff`): the internal `parquet::arrow::ColumnReader::ResetLeaf` signature and the new `parquet::RecordReader::ReserveValueBytes` virtual, neither of which is upstream Arrow's public API.
### Documentation
No. This is an internal read-path performance optimization with no user-visible API, configuration, or behavior change.
### Generative AI tooling
Generated-by: Qoder
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(parquet): size Arrow builders by selected rows in page-filtered reads [paimon-cpp]
Posted by "lxy-9602 (via GitHub)" <gi...@apache.org>.
lxy-9602 merged PR #309:
URL: https://github.com/apache/paimon-cpp/pull/309
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(parquet): size Arrow builders by selected rows in page-filtered reads [paimon-cpp]
Posted by "lxy-9602 (via GitHub)" <gi...@apache.org>.
lxy-9602 commented on code in PR #309:
URL: https://github.com/apache/paimon-cpp/pull/309#discussion_r3965915376
##########
cmake_modules/arrow.diff:
##########
@@ -12,17 +12,18 @@ index e7523add27..e079a1ad41 100644
)
endif()
diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
-index 8cb3ec83f5..0765df8fa8 100644
+index 8cb3ec83f5..9aad83ac95 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
-@@ -814,5 +814,6 @@ if(DEFINED ENV{ARROW_THRIFT_URL})
+@@ -814,6 +814,7 @@ if(DEFINED ENV{ARROW_THRIFT_URL})
set(THRIFT_SOURCE_URL "$ENV{ARROW_THRIFT_URL}")
else()
set_urls(THRIFT_SOURCE_URL
+ "https://archive.apache.org/dist/thrift/${ARROW_THRIFT_BUILD_VERSION}/thrift-${ARROW_THRIFT_BUILD_VERSION}.tar.gz"
"https://www.apache.org/dyn/closer.cgi?action=download&filename=/thrift/${ARROW_THRIFT_BUILD_VERSION}/thrift-${ARROW_THRIFT_BUILD_VERSION}.tar.gz"
"https://downloads.apache.org/thrift/${ARROW_THRIFT_BUILD_VERSION}/thrift-${ARROW_THRIFT_BUILD_VERSION}.tar.gz"
-@@ -983,6 +983,11 @@ if(CMAKE_TOOLCHAIN_FILE)
+ "https://apache.claz.org/thrift/${ARROW_THRIFT_BUILD_VERSION}/thrift-${ARROW_THRIFT_BUILD_VERSION}.tar.gz"
+@@ -983,6 +984,11 @@ if(CMAKE_TOOLCHAIN_FILE)
list(APPEND EP_COMMON_CMAKE_ARGS -DCMAKE_TOOLCHAIN_FILE=${CMAKE_TOOLCHAIN_FILE})
Review Comment:
May remove?
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org