feat: write value stats for append data files#477
Conversation
JingsongLi
left a comment
There was a problem hiding this comment.
In Java Side, the performance of the accumulator is poor, and the only truly useful link is the one that extracts stats from Parquet. Perhaps we should optimize its implementation instead of the accumulator.
@JingsongLi Thanks, addressed. I changed the implementation to avoid per-row stats accumulation. Append value stats are now extracted from Parquet writer metadata on close, aligned with the Java ParquetSimpleStatsExtractor path. Added/updated tests for append value stats, stats pruning, and timestamp stats. |
| parquet_metadata: Option<&parquet::file::metadata::ParquetMetaData>, | ||
| ) -> DataFileMeta { | ||
| let (value_stats, value_stats_cols) = | ||
| crate::arrow::format::parquet::extract_value_stats(parquet_metadata, write_fields); |
There was a problem hiding this comment.
Just return stats from parquet? It is not good to expose ParquetMetaData in core.
| .collect() | ||
| } | ||
|
|
||
| pub(crate) fn extract_value_stats( |
There was a problem hiding this comment.
We need to support default stats mode: truncate(16), otherwise the stats are too big.
|
@JingsongLi Addressed, thanks. Conflicts are resolved, ParquetMetaData is no longer exposed to core, and string value stats now use default truncate(16) with test coverage. |
|
Thanks for the update. I think there are still a couple of Java compatibility gaps here.
|
leaves12138
left a comment
There was a problem hiding this comment.
The existing stats-mode, null-count, timestamp, pruning, and MERGE-focused tests pass locally. I also added a minimal compatibility regression with metadata.stats-dense-store=false; it fails because the produced file has value_stats_cols = Some(["id"]) instead of None.
Java threads metadata.stats-dense-store into SimpleStatsConverter. In non-dense mode it stores full-width stats and leaves valueStatsCols null. In dense mode it filters disabled columns, but still leaves valueStatsCols null when every write column is represented. This implementation never reads the option and always emits Some(column_names), so Rust-written manifests are not compatible with the configured Java storage semantics. Please thread the dense-store flag into extraction/serialization and add coverage for false plus the all-columns-present dense case.
| Ok(Self { inner }) | ||
| let stats_modes = write_fields | ||
| .map(|fields| { | ||
| CoreOptions::new(format_options) |
There was a problem hiding this comment.
Parsing the per-column stats modes is not sufficient here because Java also passes metadata.stats-dense-store (default true) into SimpleStatsConverter. Rust currently ignores that option and extract_value_stats always returns dense, filtered stats with Some(column_names). With metadata.stats-dense-store=false, Java requires full-width stats and valueStatsCols = null; with dense storage enabled, Java also uses null when all write columns are present. Please thread this flag through and add compatibility tests. A local one-column counts regression with dense store disabled currently returns Some(["id"]) instead of None.
Extract append value stats from Parquet writer metadata and honor Java-compatible stats modes, dense storage, null counts, truncation, and timestamp handling. Signed-off-by: QuakeWang <wangfuzheng0814@foxmail.com>
Purpose
Rust append writes currently emit empty
_VALUE_STATS, so manifest-level stats pruning cannot skip data files written by Rust even when min/max/null-count information is available.Brief change log
DataFileWriter.DataFileMeta.value_statsand densevalue_stats_cols.Tests
cargo fmt --all -- --checkcargo clippy --all-targets --workspace --features fulltext,vortex,mosaic -- -D warningscargo test -p paimon --all-targets --features fulltext,vortex,mosaicAPI and Format
Documentation