wangyong9999 commented on code in PR #323:
URL: https://github.com/apache/paimon-cpp/pull/323#discussion_r3987958837
##########
src/paimon/common/reader/late_materializing_file_batch_reader.cpp:
##########
@@ -45,28 +46,42 @@
namespace paimon {
Result<std::unique_ptr<LateMaterializingFileBatchReader>>
LateMaterializingFileBatchReader::Create(
- std::unique_ptr<FileBatchReader> inner, const
std::shared_ptr<arrow::MemoryPool>& arrow_pool) {
+ std::unique_ptr<FileBatchReader> inner, const
std::shared_ptr<arrow::MemoryPool>& arrow_pool,
+ ProbeValidation validation) {
if (arrow_pool == nullptr) {
return Status::Invalid("arrow pool could not be nullptr.");
}
if (inner == nullptr) {
return Status::Invalid("inner could not be nullptr.");
}
auto* prefetch_inner = dynamic_cast<PrefetchFileBatchReader*>(inner.get());
- auto reader = std::unique_ptr<LateMaterializingFileBatchReader>(
- new LateMaterializingFileBatchReader(std::move(inner), prefetch_inner,
arrow_pool));
+ auto reader =
+ std::unique_ptr<LateMaterializingFileBatchReader>(new
LateMaterializingFileBatchReader(
+ std::move(inner), prefetch_inner, arrow_pool,
std::move(validation)));
return reader;
}
Result<FileBatchReader::ReadBatch>
LateMaterializingFileBatchReader::NextBatch() {
if (state_ == kInit) {
// SetReadSchema has not been called: read with the file schema,
matching the
// FileBatchReader contract for schema-less reads.
- state_ = kNoLatMat;
+ if (validation_.validate) {
+ PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<ArrowSchema> schema,
inner_->GetFileSchema());
+ PAIMON_RETURN_NOT_OK(SetReadSchema(schema.get(), nullptr,
std::nullopt));
+ } else {
+ state_ = kNoLatMat;
+ }
}
if (state_ == kProbing) {
PAIMON_RETURN_NOT_OK(ReadAndFilterProbeData());
- if (matched_bitmap_.IsEmpty()) {
+ if (!row_ids_fit_) {
Review Comment:
Two of the branches in this function have no caller. `PrepareBucketRead`
always calls `SetReadSchema` before the first `NextBatch`, so the `kInit`
validation path above only serves `ValidatesWithoutReadSchema`. And the
`row_ids_fit_` fallback protects against a file with more than 2^32 rows, which
the rest of the read path (deletion vectors, range bitmaps, `RoaringBitmap32`
selections everywhere) cannot handle either; before this PR the same spot just
truncated. Suggest dropping both, with their tests, and returning an error on
overflow if you want it explicit. `kFiltering` then only remains for the
no-payload case.
##########
src/paimon/common/reader/late_materializing_file_batch_reader.h:
##########
@@ -38,12 +40,22 @@ class PredicateFilter;
// For convenience, we abbreviate `Later Materializing` as `LatMat`.
// This reader is installed below the prefetch layer (see
// AbstractSplitRead::CreateFileBatchReader) and performs probe/payload
two-phase reads when a
-// predicate is pushed down through SetReadSchema; without a predicate it is a
plain passthrough.
+// predicate is pushed down through SetReadSchema. Without a predicate or
validation it is a
+// plain passthrough.
class LateMaterializingFileBatchReader : public PrefetchFileBatchReader {
public:
+ struct ProbeValidation {
Review Comment:
Every row of a manifest file carries the same `_VERSION`: the serializer
writes the constant for the whole file and files are immutable. So validating
the rows you materialize already rejects an unsupported file whenever it
contributes anything to the scan, and a file that contributes nothing cannot
hurt. If lxy-9602 is fine with that, this hook, the `kFiltering` state and
their five tests can go, and `PrepareBucketRead` shrinks to building the
selector predicate.
--
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]
