zjw1111 commented on code in PR #185:
URL: https://github.com/apache/paimon-cpp/pull/185#discussion_r3718898433
##########
test/inte/write_and_read_inte_test.cpp:
##########
@@ -1529,6 +1529,94 @@ TEST_P(WriteAndReadInteTest,
TestAppendWithParquetPageIndexFilter) {
ASSERT_TRUE(expected->Equals(read_result)) << read_result->ToString();
}
+/// Reproduces the prefetch + parquet page-index filter failure: the predicate
keeps only the last
+/// page of every row group, so the prefetch reader ends up seeking to a row
in the middle of a row
+/// group, which FileReaderWrapper::SeekToRow rejects.
+TEST_P(WriteAndReadInteTest, TestAppendWithParquetPageIndexFilterAndPrefetch) {
+ auto [file_format, file_system] = GetParam();
+ if (file_format != "parquet" || file_system != "local") {
+ return;
+ }
+
+ auto test_dir = UniqueTestDirectory::Create("local");
+ arrow::FieldVector fields = {arrow::field("f0", arrow::int32()),
+ arrow::field("f1", arrow::utf8())};
+ auto schema = arrow::schema(fields);
+ std::map<std::string, std::string> options = {
+ {Options::MANIFEST_FORMAT, "orc"},
+ {Options::FILE_FORMAT, "parquet"},
+ {Options::TARGET_FILE_SIZE, "1048576"},
+ {Options::BUCKET, "-1"},
+ {Options::FILE_SYSTEM, "local"},
+ // One row per page (see TestAppendWithParquetPageIndexFilter for why
these three
+ // options are needed together) and 4 rows per row group, so the 16
rows below end up
+ // in 4 row groups of 4 single-row pages.
+ {Options::WRITE_BATCH_SIZE, "1"},
+ {"parquet.page.size", "1"},
+ {"parquet.enable-dictionary", "false"},
+ {"parquet.write.enable-page-index", "true"},
+ {"parquet.write.max-row-group-length", "4"},
+ {"parquet.read.enable-page-index-filter", "true"},
+ };
+ ASSERT_OK_AND_ASSIGN(
+ auto helper, TestHelper::Create(test_dir->Str(), schema,
/*partition_keys=*/{},
+ /*primary_keys=*/{}, options,
/*is_streaming_mode=*/true));
+ std::string table_path = test_dir->Str() + "/foo.db/bar";
+
+ std::string data = R"([
+ [0, "v0"], [1, "v1"], [2, "v2"], [3, "v3"],
+ [4, "v4"], [5, "v5"], [6, "v6"], [7, "v7"],
+ [8, "v8"], [9, "v9"], [10, "v10"], [11, "v11"],
+ [12, "v12"], [13, "v13"], [14, "v14"], [15, "v15"]
+ ])";
+ ASSERT_OK_AND_ASSIGN(std::unique_ptr<RecordBatch> batch,
+ TestHelper::MakeRecordBatch(arrow::struct_(fields),
data,
+ /*partition_map=*/{},
/*bucket=*/0, {}));
+ ASSERT_OK(helper->WriteAndCommit(std::move(batch), /*commit_identifier=*/0,
+
/*expected_commit_messages=*/std::nullopt));
+
+ // Keep only the last row of every row group, so each row group is
partially matched and its
Review Comment:
Small wording nit on this comment: the predicate `In{7, 11, 15}` matches the
last row of RG1/RG2/RG3, but RG0 (rows 0–3) has no match and is fully pruned
(excluded), so it is not "partially matched". Would it be possible to phrase
this as "the last row of every *matched* row group" to avoid the impression
that RG0 is also partially matched? This doesn't affect the test or the
expected result `[7, 11, 15]`.
Separately (no change needed here): the new integration test only exercises
`SetPrefetchMaxParallelNum(1)`, i.e. a single sub reader crossing row group
boundaries. The re-keying + `min_range` retirement interaction under multiple
concurrent reader queues isn't directly covered — it looks correct from the
code, but a multi-reader regression case could be worth a follow-up if it's
cheap to add.
--
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]
