feat(parquet): wire in additional parquet writer settings - #2887
feat(parquet): wire in additional parquet writer settings#2887xanderbailey wants to merge 1 commit into
Conversation
abcfba9 to
ca7d5ba
Compare
| /// writer is built, not when properties are parsed. | ||
| pub const PROPERTY_PARQUET_COMPRESSION_CODEC: &str = "write.parquet.compression-codec"; | ||
| /// Default Parquet compression codec (matches Iceberg's default since 1.4.0). | ||
| pub const PROPERTY_PARQUET_COMPRESSION_CODEC_DEFAULT: &str = "zstd"; |
There was a problem hiding this comment.
Have called this out in the PR description but current default is actually uncompressed from what I can see.
This opens up a question generally as to if we should copy the Java defaults for these settings which might be more intuitive for user or keep the defaults as the arrow-rs defaults... I don't have a strong opinion here so interested to hear people's thoughts.
There was a problem hiding this comment.
On the matter of inconsistency, there are other places where arrow-rs has different defaults to parquet-java, like the compression level for zstd too:
| assert_eq!(upper_bounds, HashMap::from([(0, Datum::int(i32::MAX))])); | ||
| } | ||
|
|
||
| // ----------------------------------------------------------------- |
There was a problem hiding this comment.
This looked very claude so I removed
xanderbailey
changed the title
|
Think I'll bring this one up as a topic for the rust community sync. It's not actually clear to me what our defaults should be here and if we should change them. I can see arguments both ways. |
|
My suggestion is to follow the Principle of Least Astonishment. Users interacting with their data lake across different Iceberg implementations might be surprised to get wildly different file size results from the de facto reference implementation (Java). I think in this case adhering to Java defaults, since it's likely something I don't expect to be codified in the spec, is reasonable. |
|
I'm inclined to agree with this take @mbutrovich |
3 participants
Which issue does this PR close?
What changes are included in this PR?
ParquetWriterBuilder::from_table_propertiespreviously only translated the content-defined-chunking settings; every other Parquet knob silently fell through to parquet-rs (arrow-rs) defaults, which diverge from the Java implementation. This PR maps the row-group / page / dictionary sizing and compression table properties into the parquet-rsWriterProperties:write.parquet.compression-codec(defaultzstd) andwrite.parquet.compression-levelwrite.parquet.row-group-size-bytes(default 128 MB)write.parquet.page-size-bytes(default 1 MB)write.parquet.page-row-limit(default 20000)write.parquet.dict-size-bytes(default 2 MB)How the new Iceberg defaults differ from the parquet-rs defaults that were previously in effect:
Are these changes tested?
Yes