dannycjones commented on code in PR #2887:
URL: https://github.com/apache/iceberg-rust/pull/2887#discussion_r3688997756
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -168,6 +190,20 @@ pub struct TableProperties {
pub cdc_max_chunk_size: usize,
/// Content-defined chunking normalization level (gearhash bit adjustment).
pub cdc_norm_level: i32,
+ /// Parquet compression codec name (e.g. `zstd`, `gzip`). Validated when
the
+ /// writer is built, not when properties are parsed.
+ pub parquet_compression_codec: String,
Review Comment:
>Validated when the writer is built, not when properties are parsed.
This is a good tenet for the whole module - i.e. we push validation to
access, since we may not care if we're writing ORC files (as an example).
If you end up pushing another commit, I suggest we add it to the module
comment.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -46,6 +47,16 @@ use crate::transform::create_transform_function;
use crate::writer::{CurrentFileStatus, DataFile};
use crate::{Error, ErrorKind, Result};
+/// Default compression levels, pinned to parquet-java's defaults so files are
+/// comparable across implementations rather than tracking the parquet-rs
+/// defaults, which may differ and could change without us noticing.
Review Comment:
should this be a standard comment or module comment? this is currently
attached to `DEFAULT_ZSTD_COMPRESSION_LEVEL`.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -46,6 +47,16 @@ use crate::transform::create_transform_function;
use crate::writer::{CurrentFileStatus, DataFile};
use crate::{Error, ErrorKind, Result};
+/// Default compression levels, pinned to parquet-java's defaults so files are
+/// comparable across implementations rather than tracking the parquet-rs
+/// defaults, which may differ and could change without us noticing.
+/// Default zstd level (parquet-rs uses 1).
+const DEFAULT_ZSTD_COMPRESSION_LEVEL: i32 = 3;
+/// Default gzip level.
+const DEFAULT_GZIP_COMPRESSION_LEVEL: u32 = 6;
+/// Default brotli level.
+const DEFAULT_BROTLI_COMPRESSION_LEVEL: u32 = 1;
Review Comment:
I think we should use the types in the `compression` module, unless we
specifically want to diverge. While they're used primarily for metadata and
puffin right now, I think they were written with data files in mind.
I'd extend that module to support brotli. It'll give us a common module
where the conversions and errors are tested.
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -317,6 +353,36 @@ impl TableProperties {
/// Default matches `parquet::file::properties::DEFAULT_CDC_NORM_LEVEL`.
pub const PROPERTY_PARQUET_CDC_NORM_LEVEL_DEFAULT: i32 = 0;
+ /// Compression codec for Parquet data files (e.g. `zstd`, `gzip`,
`snappy`,
+ /// `lz4`, `brotli`, `uncompressed`). The codec name is validated when the
+ /// 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";
Review Comment:
I'm aligned with Matt's "_Principle of Least Astonishment_" and with the
outcome from the Rust sync - let's adopt Java defaults unless we have a reason
to diverge.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -110,6 +125,62 @@ impl ParquetWriterBuilder {
}
}
+fn parquet_compression(codec: &str, level: Option<i32>) -> Result<Compression>
{
+ let compression = match codec.to_lowercase().as_str() {
+ "uncompressed" | "none" => Compression::UNCOMPRESSED,
+ "snappy" => Compression::SNAPPY,
+ "lzo" => Compression::LZO,
+ "lz4" => Compression::LZ4,
+ "lz4_raw" => Compression::LZ4_RAW,
Review Comment:
Where does this come from? I don't see it in the iceberg-java repo, and it's
not documented for iceberg-java:
https://iceberg.apache.org/docs/latest/configuration/#write-properties
I'd rather we don't go beyond Java without intent.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -82,22 +93,26 @@ impl ParquetWriterBuilder {
/// schema, translating `write.parquet.*` settings into `WriterProperties`
/// instead of using parquet-rs defaults.
///
- /// Currently translates the content-defined-chunking keys
- /// (`write.parquet.content-defined-chunking.*`); other keys fall back to
/// parquet-rs defaults.
Review Comment:
dead comment
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -110,6 +125,62 @@ impl ParquetWriterBuilder {
}
}
+fn parquet_compression(codec: &str, level: Option<i32>) -> Result<Compression>
{
+ let compression = match codec.to_lowercase().as_str() {
+ "uncompressed" | "none" => Compression::UNCOMPRESSED,
+ "snappy" => Compression::SNAPPY,
+ "lzo" => Compression::LZO,
+ "lz4" => Compression::LZ4,
+ "lz4_raw" => Compression::LZ4_RAW,
Review Comment:
Same for `lzo`.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -46,6 +47,16 @@ use crate::transform::create_transform_function;
use crate::writer::{CurrentFileStatus, DataFile};
use crate::{Error, ErrorKind, Result};
+/// Default compression levels, pinned to parquet-java's defaults so files are
Review Comment:
I don't think there's anything for this PR.
That being said, the proposed shared testing library might be interesting. I
wonder if that can capture "this is what a Iceberg data file in Parquet looks
like with table properties [X, Y, Z]". Then we can assert encodings, etc..
I think its just something to keep in mind for the future, at this point.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
