wangyong9999 commented on code in PR #314:
URL: https://github.com/apache/paimon-cpp/pull/314#discussion_r3999067460
##########
src/paimon/format/parquet/page_filtered_row_group_reader.cpp:
##########
@@ -100,6 +100,38 @@ std::optional<DataPageLayout> GetDataPageLayout(
return DataPageLayout{column_chunk_offset, first_data_page_offset};
}
+// Call only after GetDataPageLayout has validated the ordered page row
offsets.
+template <typename Visitor>
+void VisitSelectedPages(const RowRanges& row_ranges,
Review Comment:
The scan this replaces is one of three linear passes over the same page
vector on this path: `GetDataPageLayout` validates every page location before
either caller gets here, and `ComputeCompressedRowRanges` walks every page
again afterwards — the description keeps both as linear. So the cursor plus
binary search removes one pass out of three, and the benchmark table shows
nothing outside noise.
What it costs is a helper that is only correct while the row ranges stay
ascending and disjoint and `first_row_index` stays strictly increasing, an
invariant carried in a comment that callers have to honour, plus the 121-line
equivalence test needed to hold it in place.
The OffsetIndex memoization is worth keeping —
`RowGroupPageIndexReaderImpl::GetOffsetIndex` calls `OffsetIndex::Make` on
every lookup, so that one removes real work. I would drop the page-cursor
rewrite until a profile shows the scan matters.
##########
src/paimon/format/parquet/file_reader_wrapper_test.cpp:
##########
@@ -311,6 +312,99 @@ TEST_F(FileReaderWrapperTest,
PredicateReadsOnlyItsPageIndexesAndKeepsPayloadRea
ASSERT_FALSE(end);
}
+TEST_F(FileReaderWrapperTest, ReusesParsedOffsetIndexesWithinFileReader) {
+ std::string file_path = PathUtil::JoinPath(dir_->Str(),
"parsed-index.parquet");
+ PrepareParquetFile(file_path, /*row_count=*/2000,
/*enable_page_index=*/true);
+ ASSERT_OK_AND_ASSIGN(auto reader, PrepareReaderWrapper(file_path));
+ std::weak_ptr<::parquet::OffsetIndex> released_index;
+ {
+ auto first = reader->GetRowGroupPageIndexReader(0);
+ auto second = reader->GetRowGroupPageIndexReader(1);
+ ASSERT_TRUE(first);
+ ASSERT_TRUE(second);
+ ASSERT_NE(first, second);
+ for (int32_t col = 0; col < 3; ++col) {
+ auto offset = first->GetOffsetIndex(col);
+ auto column = first->GetColumnIndex(col);
+ ASSERT_TRUE(offset);
+ ASSERT_TRUE(column);
+ ASSERT_EQ(offset, first->GetOffsetIndex(col));
+ ASSERT_NE(offset, second->GetOffsetIndex(col));
+ released_index = offset;
+ }
+ ASSERT_THROW(first->GetOffsetIndex(-1), ::parquet::ParquetException);
+ ASSERT_THROW(first->GetColumnIndex(100), ::parquet::ParquetException);
+ ASSERT_TRUE(first->GetOffsetIndex(0));
+ }
+ ASSERT_FALSE(released_index.expired());
+ reader.reset();
+ ASSERT_TRUE(released_index.expired());
+}
+
+TEST_F(FileReaderWrapperTest,
ParsedOffsetIndexesRespectRowGroupRetentionLimit) {
Review Comment:
This writes 1,025 single-row row groups to re-assert
`kMaxRowGroupPageIndexReaders`, a constant this PR does not change, and pins it
through `weak_ptr` expiry, which is the retention map's internals rather than
the memoization being added.
`MissingPageIndexesRemainReadable` below never runs its index assertions at
all: with `enable_page_index=false` the row group has no index ranges, so
`PageIndexReaderImpl::RowGroup()` returns nullptr and the whole `if (indexes)`
body is skipped. What is left is a 2,000-row read that existing tests already
cover.
`ReusesParsedOffsetIndexesWithinFileReader` covers what actually changed; I
would drop these two.
--
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]
