zjw1111 commented on code in PR #226:
URL: https://github.com/apache/paimon-cpp/pull/226#discussion_r3820615157
##########
src/paimon/format/parquet/parquet_format_defs.h:
##########
@@ -40,6 +40,10 @@ namespace paimon::parquet {
// write
static inline const char PARQUET_BLOCK_SIZE[] = "parquet.block.size";
static inline const char PARQUET_PAGE_SIZE[] = "parquet.page.size";
+// Max number of rows in a single data page, aligned with parquet-mr's
+// "parquet.page.row.count.limit" (PARQUET-1414). A page is finished when
either
+// this row count or the page byte size (parquet.page.size) is reached first.
Review Comment:
This comment overstates the guarantee. In parquet-mr the row count is a hard
bound: `ColumnWriteStoreBase` pulls `rowCountForNextRowCountCheck` down to
`rowsWrittenSoFar + pageRowCountLimit` (`// Do the check earlier if required to
keep the row count limit`), so a page never exceeds the limit. Here the
condition is only evaluated at write-batch granularity in
`CommitWriteAndCheckPageLimit`, so the actual bound is `ceil(limit /
write.batch-size) * write.batch-size` — 20480 rows with the default
`write.batch-size=1024`, not 20000.
Please reword this to a best-effort limit and state that a page may exceed
it by up to one write batch (arrow-rs describes its equivalent option that
way). Same for the PR description, which currently claims page layout
consistent with parquet-mr — per-page comparison across engines will not match
exactly.
##########
src/paimon/format/parquet/parquet_writer_builder.cpp:
##########
@@ -86,6 +86,16 @@ Result<std::shared_ptr<::parquet::WriterProperties>>
ParquetWriterBuilder::Prepa
OptionsUtils::GetValueFromMap<int64_t>(options_,
PARQUET_PAGE_SIZE,
::parquet::kDefaultDataPageSize));
builder.data_pagesize(page_size);
+ PAIMON_ASSIGN_OR_RAISE(
+ int64_t page_row_count_limit,
+ OptionsUtils::GetValueFromMap<int64_t>(options_,
PARQUET_PAGE_ROW_COUNT_LIMIT,
+
::parquet::DEFAULT_DATA_PAGE_ROW_COUNT_LIMIT));
+ // Aligned with parquet-mr's checkArgument(rowCount > 0) for
parquet.page.row.count.limit.
+ if (page_row_count_limit <= 0) {
+ return Status::Invalid(fmt::format("Option '{}' should be greater than
0, but got {}",
+ PARQUET_PAGE_ROW_COUNT_LIMIT,
page_row_count_limit));
+ }
+ builder.data_page_row_count_limit(page_row_count_limit);
Review Comment:
Related: the effective granularity of this limit comes from
`builder.write_batch_size(batch_size_)` at line 61, i.e. Paimon's
`write.batch-size` (default 1024). The two options are semantically unrelated,
so setting `write.batch-size` to e.g. 65536 silently turns
`parquet.page.row.count.limit=20000` into a no-op — the first check already
lands well past the limit, giving 65536 rows per page.
At minimum this coupling needs to be documented in the option description. A
stronger fix is to bound the batch size used for the page-size check so the row
limit stays meaningful regardless of `write.batch-size`.
--
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]
