JingsongLi commented on code in PR #1072:
URL: https://github.com/apache/paimon-rust/pull/1072#discussion_r4215471937
##########
crates/paimon/src/arrow/format/parquet.rs:
##########
@@ -445,7 +445,66 @@ fn parquet_writer_properties(
);
}
}
- Ok(props.build())
+ Ok(parquet_sizing_properties(props, options)?.build())
+}
+
+/// Java's parquet writer sizing keys; `file.block-size` overrides
`parquet.block.size` as in
+/// Java's `ParquetFileFormat`.
+fn parquet_sizing_properties(
+ mut props: parquet::file::properties::WriterPropertiesBuilder,
+ options: &HashMap<String, String>,
+) -> crate::Result<parquet::file::properties::WriterPropertiesBuilder> {
+ const BLOCK_SIZE: &str = "parquet.block.size";
+ const PAGE_SIZE: &str = "parquet.page.size";
+ const DICTIONARY_PAGE_SIZE: &str = "parquet.dictionary.page.size";
+ const PAGE_ROW_COUNT_LIMIT: &str = "parquet.page.row.count.limit";
+ const PAGE_SIZE_ROW_CHECK_MIN: &str = "parquet.page.size.row.check.min";
+ let positive = |key: &str| -> crate::Result<Option<usize>> {
+ options
+ .get(key)
+ .map(|value| {
+ value
+ .trim()
+ .parse::<usize>()
+ .ok()
+ .filter(|parsed| *parsed > 0)
+ .ok_or_else(|| crate::Error::ConfigInvalid {
+ message: format!("Option '{key}' must be a positive
integer, got: {value}"),
+ })
+ })
+ .transpose()
+ };
+ let file_block_size = CoreOptions::new(options)
+ .file_block_size()?
+ .map(|bytes| {
+ usize::try_from(bytes)
+ .ok()
+ .filter(|bytes| *bytes > 0)
+ .ok_or_else(|| crate::Error::ConfigInvalid {
+ message: format!("Option 'file.block-size' must be
positive, got: {bytes}"),
+ })
+ })
+ .transpose()?;
+ if let Some(bytes) = file_block_size.or(positive(BLOCK_SIZE)?) {
+ props = props.set_max_row_group_bytes(Some(bytes));
Review Comment:
[P2] Bound the initial Arrow batch before relying on the byte limit
parquet-rs 58.4 only estimates/splits by bytes when the current row group
already contains rows; an empty group accepts the entire first RecordBatch and
flushes afterwards. The production table writer forwards that batch unchanged.
I reproduced with an actual filesystem append table: parquet.block.size=16384,
dictionary disabled, compression=none, and one 65,536-row two-INT batch produce
a single 559,708-byte row group. The PK buffering path also produces one group
for 4,000 rows with file.block-size=16 kb. Thus the intended point-read
granularity still depends on caller/buffer batch size, especially with wide
rows. Please feed bounded slices and check/flush between them (or fix the
dependency behavior), and test a large initial batch through TableWrite rather
than only 100-row format-writer chunks.
##########
crates/paimon/src/arrow/format/parquet.rs:
##########
@@ -445,7 +445,66 @@ fn parquet_writer_properties(
);
}
}
- Ok(props.build())
+ Ok(parquet_sizing_properties(props, options)?.build())
+}
+
+/// Java's parquet writer sizing keys; `file.block-size` overrides
`parquet.block.size` as in
+/// Java's `ParquetFileFormat`.
+fn parquet_sizing_properties(
+ mut props: parquet::file::properties::WriterPropertiesBuilder,
+ options: &HashMap<String, String>,
+) -> crate::Result<parquet::file::properties::WriterPropertiesBuilder> {
+ const BLOCK_SIZE: &str = "parquet.block.size";
+ const PAGE_SIZE: &str = "parquet.page.size";
+ const DICTIONARY_PAGE_SIZE: &str = "parquet.dictionary.page.size";
+ const PAGE_ROW_COUNT_LIMIT: &str = "parquet.page.row.count.limit";
+ const PAGE_SIZE_ROW_CHECK_MIN: &str = "parquet.page.size.row.check.min";
+ let positive = |key: &str| -> crate::Result<Option<usize>> {
+ options
+ .get(key)
+ .map(|value| {
+ value
+ .trim()
+ .parse::<usize>()
+ .ok()
+ .filter(|parsed| *parsed > 0)
+ .ok_or_else(|| crate::Error::ConfigInvalid {
+ message: format!("Option '{key}' must be a positive
integer, got: {value}"),
+ })
+ })
+ .transpose()
+ };
+ let file_block_size = CoreOptions::new(options)
+ .file_block_size()?
+ .map(|bytes| {
+ usize::try_from(bytes)
+ .ok()
+ .filter(|bytes| *bytes > 0)
+ .ok_or_else(|| crate::Error::ConfigInvalid {
+ message: format!("Option 'file.block-size' must be
positive, got: {bytes}"),
+ })
+ })
+ .transpose()?;
+ if let Some(bytes) = file_block_size.or(positive(BLOCK_SIZE)?) {
Review Comment:
[P2] Parse parquet.block.size only when file.block-size is absent
Option::or eagerly evaluates positive(BLOCK_SIZE)?, even when
file_block_size is Some. An existing table with file.block-size=16 kb and a
shadowed parquet.block.size=128mb therefore fails an actual native write with
ConfigInvalid, although the effective sizing value is valid. Java's
getParquetConfiguration overwrites BLOCK_SIZE with file.block-size before
parsing it, so the dormant value does not prevent writes there. Such options
pass schema validation and previously worked in the native writer. Please make
this fallback lazy and add a write/commit/read regression with a malformed
shadowed parquet.block.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]
