zjw1111 commented on code in PR #209:
URL: https://github.com/apache/paimon-cpp/pull/209#discussion_r3803377993
##########
src/paimon/format/parquet/parquet_file_batch_reader.cpp:
##########
@@ -635,14 +635,18 @@ Result<std::vector<std::pair<uint64_t, uint64_t>>>
ParquetFileBatchReader::GenRe
PAIMON_PARQUET_CATCH_AND_RETURN_STATUS("ParquetFileBatchReader::GenReadRanges")
}
+Result<std::vector<std::pair<uint64_t, uint64_t>>>
ParquetFileBatchReader::PreBufferRange() {
+ return reader_->GetPreBufferRanges();
+}
+
Result<::parquet::ReaderProperties>
ParquetFileBatchReader::CreateReaderProperties(
const std::shared_ptr<arrow::MemoryPool>& pool,
const std::map<std::string, std::string>& options) {
::parquet::ReaderProperties reader_properties;
// TODO(jinli.zjw): set more ReaderProperties (compare with java)
PAIMON_ASSIGN_OR_RAISE(
bool enable_pre_buffer,
- OptionsUtils::GetValueFromMap<bool>(options,
PARQUET_READ_ENABLE_PRE_BUFFER, true));
+ OptionsUtils::GetValueFromMap<bool>(options,
PARQUET_READ_ENABLE_PRE_BUFFER, false));
Review Comment:
The PR description says the format-level prebuffer is disabled through
`ReaderBuilder::WithPreBufferEnabled(false)`, scoped to the case where the
read-ahead cache is active. `WithPreBufferEnabled` does not exist anywhere in
the codebase, and `include/paimon/format/reader_builder.h` is not part of this
PR. What this change actually does is flip the default of
`parquet.read.enable-pre-buffer` from `true` to `false` for every parquet read,
both here and in `CreateArrowReaderProperties` (line 670).
Paths where neither prefetch layer is active after this change:
- `context_->EnablePrefetch() == false`: `abstract_split_read.cpp:148` never
constructs `PrefetchFileBatchReaderImpl`, so there is no read-ahead cache at
all.
- `PrefetchCacheMode::NEVER`: `prefetch_file_batch_reader_impl.cpp:85` does
not create the cache.
- `EXCLUDE_PREDICATE` / `EXCLUDE_BITMAP` / `EXCLUDE_BITMAP_OR_PREDICATE`
with a predicate or selection bitmap present: `NeedInitCache()`
(`prefetch_file_batch_reader_impl.cpp:307-323`) returns false, so the cache is
constructed but `Init()` is never called. Every `CacheInputStream::Read` still
routes through `cache_->Read`, misses, and falls back to the underlying stream
— a 100% miss rate, plus an extra `InputStream` opened at
`prefetch_file_batch_reader_impl.cpp:84-88` and the miss counters folded into
the metrics reported by `GetReaderMetrics()`.
In all three cases parquet reads degrade to per-chunk synchronous IO with no
prefetch of any kind.
Related: the `enable_buffered_stream()` -> `disable_buffered_stream()`
switch on lines 651-653 changes per-read IO granularity and memory behavior for
all parquet reads, not just the prebuffer path.
`parquet.read.enable-pre-buffer` is a user-visible option and `docs/` does not
mention it, so this default change is undocumented.
--
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]
