wangyong9999 commented on code in PR #314:
URL: https://github.com/apache/paimon-cpp/pull/314#discussion_r3996191275
##########
src/paimon/format/parquet/parquet_input_stream.h:
##########
@@ -78,21 +80,22 @@ class ParquetInputStream : public ArrowInputStreamAdapter {
using ArrowInputStreamAdapter::ReadAt;
arrow::Result<int64_t> ReadAt(int64_t position, int64_t nbytes, void* out)
override {
- if (!cache_ || file_uri_.empty() || nbytes <= 0 ||
- nbytes > std::numeric_limits<int32_t>::max()) {
+ if (!cache_ || file_uri_.empty() || position < 0 || nbytes <= 0 ||
position > file_size_ ||
+ nbytes > file_size_ - position || nbytes >
std::numeric_limits<int32_t>::max()) {
return ArrowInputStreamAdapter::ReadAt(position, nbytes, out);
}
+ bool is_index = false;
auto range = index_ranges_.upper_bound(position);
- if (range == index_ranges_.begin()) {
- return ArrowInputStreamAdapter::ReadAt(position, nbytes, out);
+ if (range != index_ranges_.begin()) {
+ --range;
+ const int64_t offset = position - range->first;
+ is_index = offset <= range->second && nbytes <= range->second -
offset;
}
- --range;
- if (position - range->first > range->second ||
- nbytes > range->second - (position - range->first)) {
+ if (!is_index && !cache_data_) {
return ArrowInputStreamAdapter::ReadAt(position, nbytes, out);
}
auto key = CacheKey::ForKind(file_uri_, position,
static_cast<int32_t>(nbytes),
Review Comment:
Agreed on the request-shaped key and shared-budget concerns. I synchronized
this branch with main (including #272) through a merge commit, preserving the
review history, and withdrew the exact-range data-cache layer rather than just
documenting those limitations. The option, added ReadAsync path, supporting
tests/docs and accompanying local-filesystem change are removed. The retained
diff only covers reader-local parsed-index reuse, sparse page selection and
reuse of direct-plan decisions.
One qualification after tracing #272: its FileBlockCache is a single-block
fallback for reads not covered by registered prefetch ranges. Cross-block/large
reads decline, registered prefetch fetches bypass it, Read() waits on the
future, and single-flight is instance-local. Shared backing is a sensible
direction, but adding Cache alone would not cover all current Parquet/native
async paths. File-end block alignment also means that a shared key needs
unambiguous immutable content/range identity, and bounded blocks alone do not
isolate aggregate data occupancy from metadata.
The revised description separates these future design requirements from the
current implementation and removes the old cache/async combination as evidence
for the retained diff. The PR remains draft.
##########
src/paimon/format/parquet/parquet_input_stream.h:
##########
@@ -103,24 +106,89 @@ class ParquetInputStream : public ArrowInputStreamAdapter
{
int64_t size,
ArrowInputStreamAdapter::ReadAt(position, nbytes,
segment.MutableData()));
if (size != nbytes) {
- return Status::IOError("Short read of Parquet page index");
+ return Status::IOError("Short read of Parquet cached
range");
}
return std::make_shared<CacheValue>(segment, CacheCallback());
});
if (!value.ok()) {
return ToArrowStatus(value.status());
}
if (!value.value() || value.value()->GetSegment().Size() != nbytes) {
- return arrow::Status::IOError("Invalid Parquet page-index cache
value");
+ return arrow::Status::IOError("Invalid Parquet cached range
value");
}
std::memcpy(out, value.value()->GetSegment().Data(), nbytes);
return nbytes;
}
+ arrow::Future<std::shared_ptr<arrow::Buffer>> ReadAsync(const
arrow::io::IOContext& io_context,
+ int64_t position,
+ int64_t nbytes)
override {
+ if (!cache_data_ || !cache_ || file_uri_.empty() || position < 0 ||
nbytes <= 0 ||
Review Comment:
The duplication was real. After the design review I removed the entire added
ReadAsync data-cache path, so ParquetInputStream now matches synchronized main
and these duplicate guards/index lookups no longer exist in the diff. I have
not added a helper for the withdrawn layer. Any future shared-block
implementation should centralize its eligibility/key construction at that layer.
--
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]
