SteNicholas commented on code in PR #481:
URL: https://github.com/apache/paimon-cpp/pull/481#discussion_r4420894877
##########
src/paimon/common/reader/blob_fallback_batch_reader.cpp:
##########
@@ -101,24 +105,35 @@ BlobFallbackBatchReader::BlobFallbackBatchReader(
arrow_pool_(arrow_pool),
finished_reader_metrics_(std::make_shared<MetricsImpl>()) {}
+Result<BatchReader::ReadBatchWithBitmap>
BlobFallbackBatchReader::ReadSegmentBatch(
+ const Segment& segment, int32_t max_rows, bool read_payload) {
+ const std::optional<BlobBatchRequest> previous_request =
segment.blob_context->request;
+ segment.blob_context->request = BlobBatchRequest{max_rows, read_payload};
+ ScopeGuard reset_request([context = segment.blob_context,
previous_request]() {
+ context->request = previous_request;
+ });
+ return segment.reader->NextBatchWithBitmap();
Review Comment:
Two related concerns about how read/skip requests reach
`BlobFileBatchReader`. I think one fix covers both:
1. **Call count when coverage alternates.** `FillWindow` issues one bounded
`ReadSegmentBatch` per contiguous read/skip run (L127-137). If the newest layer
alternates value/placeholder within a window, every run in an older group is 1
row long, so a 1024-row window becomes ~1024 `NextBatchWithBitmap` calls per
older group. Each call goes through FieldMappingReader →
CompleteRowTrackingFieldsBatchReader → DV reader → `BlobFileBatchReader`, with
an export/import, `AddArrowArrayLifetime`, a `ScopeGuard` and a 1-row
`BuildTargetArray` or `MakeArrayOfNull`. For scalar BLOBs in descriptor mode,
`BuildTargetArray` doesn't read payloads anyway, so skipping saves nothing
there and this path ends up slower than before.
2. **Implicit contract through shared mutable state.** The request is set on
the shared `BlobReadContext` before `NextBatchWithBitmap()` and restored
afterwards, so correctness relies on no layer in between ever buffering,
reading ahead, or calling the inner reader outside this call. That holds today,
but if a wrapper ever served rows produced under an earlier
`read_payload=false` request, they would arrive as all-null arrays.
`ComputePlaceholderFlags` would treat them as explicit NULLs that take
precedence over older layers, so the rows would silently become NULL instead of
falling back. The `length() > remaining` check doesn't catch this.
Would it make sense to make this an explicit reader API instead, e.g. a
single call with `max_rows` plus a per-row payload mask, or a `Skip(n)` that
only advances the position? That would bound the number of calls per window,
make the contract enforceable, and also avoid allocating all-null arrays (and
filling `_ROW_ID`/`_SEQUENCE_NUMBER` that are then discarded) for older layers
whose rows are already fully covered. If that's too large for this PR, merging
short skip runs into the surrounding read runs would at least address the call
count.
##########
src/paimon/common/reader/blob_fallback_batch_reader.cpp:
##########
@@ -74,6 +75,9 @@ Result<std::unique_ptr<BlobFallbackBatchReader>>
BlobFallbackBatchReader::Create
return Status::Invalid("Blob fallback sequence group should not be
empty.");
}
for (const auto& segment : segments) {
+ if (segment.reader != nullptr && segment.blob_context == nullptr) {
+ return Status::Invalid("Blob fallback file segment needs a
blob read context.");
+ }
Review Comment:
With the pending buffer removed, `BlobReadContext` changes from an
optimization into a hard requirement. Before this change, a file segment
without a context still worked (it just read every payload); now `Create`
rejects it and the whole data-evolution blob read fails.
The context is only attached when
`dynamic_cast<BlobReadContextAwareBuilder*>` succeeds in
`AbstractSplitRead::CreateRawFileReader`. If `PrepareReaderBuilder` ever
returns a wrapped builder, or a blob-compatible format's builder doesn't
implement the interface, every blob fallback read starts failing with this
error.
If the hard requirement is intended, could you add a case for it to
`TestCreateValidation`? Nothing covers this check right now, and without it a
segment with no context would dereference a null `blob_context` in
`ReadSegmentBatch`.
##########
src/paimon/format/blob/blob_read_context.h:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <memory>
+#include <optional>
+
+#include "paimon/visibility.h"
+
+namespace paimon {
+
+struct BlobBatchRequest {
+ /// Upper bound on selected rows before outer deletion-vector filtering.
+ int32_t max_rows;
+ bool read_payload;
+};
Review Comment:
A few nits:
- `BlobBatchRequest` has no default member initializers, so
`BlobBatchRequest req;` leaves `max_rows` and `read_payload` indeterminate.
Only aggregate init is used today, but `int32_t max_rows = 0;` and `bool
read_payload = true;` would make it safe.
- `BlobBatchRequest` and `SetBlobReadContext` (L49) are missing `///`
comments; `docs/code-style.md` asks for Doxygen comments on classes and public
methods.
- In `BlobFallbackBatchReader::FillWindow`, `gap_row_ids` is filled for gap
chunks of every group, but `AssembleRowIdRun` only reads `group_chunks[0]`.
Filling it only when `group_idx == 0` avoids per-row allocations for older
layers.
##########
src/paimon/core/operation/abstract_split_read.cpp:
##########
@@ -287,9 +286,19 @@ Result<std::unique_ptr<FileBatchReader>>
AbstractSplitRead::CreateRawFileReader(
PAIMON_ASSIGN_OR_RAISE(std::string data_file_identifier,
file->FileFormat());
PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<ReaderBuilder> reader_builder,
PrepareReaderBuilder(data_file_identifier,
extra_format_options));
- return CreateFieldMappingReader(data_file_path, file, partition,
std::move(reader_builder),
- field_mapping_builder, dv_factory,
row_ranges,
- data_file_path_factory);
+ // Capture the per-file blob context before late materialization or other
readers wrap the
+ // format. The builder passes it directly to the bottom blob reader.
+ std::shared_ptr<BlobReadContext> blob_context;
+ if (auto* blob_builder =
dynamic_cast<BlobReadContextAwareBuilder*>(reader_builder.get())) {
+ blob_context = std::make_shared<BlobReadContext>();
+ blob_builder->SetBlobReadContext(blob_context);
+ }
Review Comment:
Until now, non-test code under `src/paimon/core` and `src/paimon/common`
didn't include anything from `src/paimon/format/blob/`. With this change,
`abstract_split_read.cpp` and `blob_fallback_batch_reader.h` include the blob
plugin's internal `paimon/format/blob/blob_read_context.h`, which reverses the
dependency direction. This generic path also now does a `dynamic_cast` and
allocates a `BlobReadContext` for every blob file on every read path (normal,
merge, lazy-concat), although only the data-evolution fallback reader uses it.
Suggestion: move `BlobReadContext`/`BlobReadContextAwareBuilder` under
`src/paimon/common/reader` (the blob plugin already includes
`paimon/common/...` headers), and attach the context only for readers created
by `DataEvolutionSplitRead::CreateBlobFallbackReader`.
--
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]
