trino

Support setting compression_codec table properties for Iceberg Connector

#25755

Open

Nit: no need for a lambda here, just newCompressionCodec.orElse(oldCompressionCodec)

orElse requires the parameter to be T and return T, but we are processing on optional and want to return optional, so want to use the or instaed of orElse

Ah, I missed that- still, you want to avoid spinning a lambda for a cheap operation like this. Maybe a cleaner way to write it is: newCompressionCodec.isPresent() ? newCompressionCodec : oldCompressionCodec; like we do later on for compression level.

Nit: no need for a lambda here, just orElse(OptionalInt.empty())

done

Error message here mentions LZ4 explicitly, but it seems like this should be a parameter based on compressionCodec.get()

changed to pass the compressionCodec.get() instead of LZ4

I wonder whether this branch would actually be easier to read and understand as an enum switch statement. That way we would fail to compile if a new enum were added and we would know we needed to add new handling at that point.

done

Let's rewrite this as a switch over the file formats and compression codecs so that it's easier to read and understand the valid combinations.

done

duplicate call to getFileFormat, you can pass the instance from the previous line here instead

done

I would just inline compressionLevelValue != null && !compressionLevelValue.isEmpty() instead of bringing in Strings to do that

done

Let's move these cleanup / refactoring changes to a separate commit

Is this related to the iceberg parquet compression handling bug? If so, we'll want an inline comment explaining what's going on here

done

Could you clarify what is the bug being referring to here ? I responded at #24851 (comment) to the concern raised there.

If we let Iceberg write the table property write.parquet.compression-codec to zstd by default for us, then this PR will introduce some non-backward-compatible behavior. Suppose we have a user that didn't set write.parquet.compression-codec in their table property, if they are setting write.parquet.compression-codec to snappy in their iceberg config/iceberg session property, when they write a new file, on a previous version of Trino, the compression-codec being used would be snappy. But because this change will try to also read compression-codec from table properties when writing a file, this time the compression-codec being used would be zstd instead of snappy, which is not backward compatible. So we want to disable Iceberg from setting this table property if that's not being set by user.

We'll definitely want a test around this quirk

it should be covered by the test implicitly, but I still added a test on it

Unintentional new line insertion?

removed

Looks like checkstyle is complaining about this line, but giving the wrong line number in the error.

Property name should be compression_codec to maintain consistency with equivalent iceberg table property and the connector session property.

The order of precedence should be: session property > table property > catalog config property

I can change the property name. But I don't understand why session property should have a higher precedence over table property, if a user set table property for a particular table, it should have higher precedence compared to the default one from session property right?

getCompressionCodec

will update the name

If user doesn't set anything, our default for iceberg.compression-codec catalog config property should apply.

Already replied in a previous comment.

It feels very odd to add a table property which is supported only for AVRO file format. When we're adding something as a Trino property, the expectation is that Trino is able to actually implement it for the typical use case (orc/parquet in this case). I suggest dropping the "compression_level" changes unless we actually implement it in Trino orc and parquet writers. Trino is already adhering to iceberg table property write.avro.compression-level without these changes, and you can still use ALTER TABLE tableName SET PROPERTIES extra_properties ... to change this property through Trino. So we don't strictly need this to be a Trino property.

Sure, I can remove support for setting compression_level.

The issue you've linked to is closed, we need to either re-open that (with explanation why) or file a new issue and refer to that.

Sure, we may re-open it after we reach agreement on #25755 (comment)

Why ? If user has not set table property, the catalog config property for compression codec still applies

explained in a previous comment