lxy-9602 commented on code in PR #342:
URL: https://github.com/apache/paimon-cpp/pull/342#discussion_r4026248994
##########
include/paimon/utils/prefetch_cache_config.h:
##########
@@ -34,8 +34,9 @@ namespace paimon {
/// ReadAheadCache to balance memory usage, I/O efficiency, and latency hiding.
class PAIMON_EXPORT CacheConfig {
public:
- /// Returns the maximum allowed size (in bytes) for a single cached range.
- /// Defaults to 32 MiB.
+ /// Returns the maximum allowed size (in bytes) for a single cached range,
both for the ranges
+ /// registered up front and, as the cap, for the ones registered mid-read.
+ /// Defaults to 8 MiB.
Review Comment:
Is the default 8 MB or 4 MB?
##########
include/paimon/reader/prefetch_file_batch_reader.h:
##########
@@ -135,6 +136,20 @@ class PAIMON_EXPORT PrefetchFileBatchReader : public
FileBatchReader {
virtual Result<std::vector<std::pair<uint64_t, uint64_t>>>
PreBufferRange() {
return std::vector<std::pair<uint64_t, uint64_t>>{};
}
+
+ /// Callback a reader reports byte ranges through when they only become
known after reading has
+ /// started, so the prefetch layer can register them with its shared
read-ahead cache.
+ ///
+ /// PreBufferRange() covers what is known up front; a reader whose ranges
depend on data it
+ /// has already read - the late-materialization payload pass only knows
which pages hold the
+ /// matched rows once the probe pass has run - reports them through this
callback instead.
+ using PreBufferRangeCallback =
+ std::function<void(std::vector<std::pair<uint64_t, uint64_t>>&&)>;
+
+ /// Installs the callback above, or clears it when `callback` is empty. By
default a reader has
+ /// no late byte ranges to report and ignores the callback.
+ /// @param callback The callback to report late byte ranges through.
+ virtual void SetPreBufferRangeCallback(PreBufferRangeCallback callback) {}
Review Comment:
I think the current complexity mainly comes from placing
`LateMaterializingReader` below `PrefetchReader`:
```text
PrefetchReader → LateMaterializingReader → FormatReader
```
Each sub-reader discovers payload ranges after probing and must report them
back to the outer layer. This introduces callbacks, lifetime management,
incremental `AddRanges()`, deduplication, and `Warmup(offset)`.
Could we invert the structure?
```text
LateMaterializingReader → PrefetchReader → FormatReader
```
Then late materialization can run two normal prefetch phases:
1. Configure and finish the probe phase.
2. Build the matched bitmap.
3. Reconfigure the prefetch reader for the payload phase.
This removes the late-range callback path and keeps responsibilities
clearer. The main trade-off is a global probe barrier, so we should benchmark
first-batch latency and total scan time. Unless the regression is significant,
this design seems easier to understand and maintain.
##########
src/paimon/format/parquet/page_filtered_row_group_reader.cpp:
##########
@@ -44,11 +46,15 @@ namespace paimon::parquet {
namespace {
-/// Ceiling on the value bytes a leaf may reserve up front from column chunk
metadata alone.
-/// The estimate it caps is a heuristic over footer fields, which are
attacker-controlled and
-/// need not describe the pages this read touches, so it must not turn into an
unbounded eager
-/// allocation. Past this size the builder's doubling is already amortized
against a large read.
-constexpr int64_t kMaxMetadataValueBytesReservation = int64_t{16} * 1024 *
1024;
+/// Multiple of a column chunk's compressed size that a leaf may reserve up
front in value bytes.
+/// The estimate is a heuristic over footer fields, and
`total_uncompressed_size` is a bare claim a
+/// forged footer can inflate to make a leaf eagerly allocate gigabytes for a
near-empty file.
+/// `total_compressed_size` is instead checked against the file length before
any read (Arrow's
+/// ComputeColumnChunkRange, and GetDataPageLayout below, bound a chunk and
its pages by it), so a
+/// factor of it is bounded by bytes that physically exist. A leaf
decompressing within the factor
+/// is reserved in full; a more compressible one is capped and left to the
builder's amortized
+/// doubling, which is cheap against the I/O and decode of a read that large.
+constexpr int64_t kMaxReservationDecompressionFactor = 20;
Review Comment:
This change removes the previous absolute cap of 16 MiB and replaces it with
a limit of up to `20 * total_compressed_size`. `ResetLeaf()` now calls
`ReserveValueBytes()` eagerly; for normal RowGroups with hundreds of MiB
compressed size and several GiB uncompressed size, this may allocate several
GiB up front before decoding even begins. Since the average is computed over
the entire chunk, it can also significantly overestimate the actual output when
skipped pages contain large values while matched pages contain only small
values, causing selective queries to OOM directly. I’d suggest keeping an
absolute cap, or integrating this with a controlled memory budget.
##########
src/paimon/format/parquet/page_filtered_row_group_reader.cpp:
##########
@@ -383,13 +391,22 @@ Result<std::shared_ptr<arrow::ChunkedArray>>
PageFilteredRowGroupReader::ReadFil
// including the pages this selection skips, so it misleads when
wide values sit in
// skipped pages. Either direction only costs performance — the
reservation is a
// hint the builder grows past when short — but they are why the
result is capped
- // instead of trusted. Fixed-width leaves ignore the byte count
entirely.
+ // instead of trusted, by a factor of the file-bounded compressed
size rather than the
+ // forgeable uncompressed one (see
kMaxReservationDecompressionFactor). Fixed-width
+ // leaves ignore the byte count entirely.
const double avg = static_cast<double>(chunk_bytes) /
static_cast<double>(chunk_values);
reserve_value_bytes = std::min(
{SaturatingDoubleToInteger<int64_t>(avg *
static_cast<double>(reserve_values)),
- chunk_bytes, kMaxMetadataValueBytesReservation});
+ chunk_bytes,
+ SaturatingDoubleToInteger<int64_t>(
+ static_cast<double>(chunk_compressed_bytes) *
+
static_cast<double>(kMaxReservationDecompressionFactor))});
}
+ std::cerr << "[reserve_value_bytes] col_idx=" << col_idx
+ << " reserve_values=" << reserve_values
+ << " reserve_value_bytes=" << reserve_value_bytes <<
std::endl;
+
Review Comment:
Could you please confirm whether this is intended to be output here?
--
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]
