You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2024/01/18 05:58:15 UTC
[PR] Core: Fix setting updated parquet compression property [iceberg]
amogh-jahagirdar opened a new pull request, #9503:
URL: https://github.com/apache/iceberg/pull/9503
Fixes #9490 .
As part of 1.4.0 we made the default compression for Parquet to zstd. However, this property is being set for every new table regardless of the underlying file formats (e.g. we wouldn't want to set these properties for Avro or ORC).
This change conditionally sets the property based on the underlying file formats when setting the properties in the TableMetadata for new table creation.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1458104584
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
The format can be set to ORC at write time. The parquet compression codec is already persisted then.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1457704960
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
Not sure if I follow, isn't that the right behavior? If the default format is parquet then the parquet compression codec should be set.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1459520414
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
Here is a smoke test with ORC format in Trino before and after the change. Before the change the test passes because Parquet compression was defined with ZSTD, after there's no such table property.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1457640453
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
If file format is not set in `rawProperties` but at write time, parquet compression codec will still be persisted since the default format is `parquet`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1458161912
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
Then I'm wondering if this can fix #9490. An alternative is to filter out parquet compression if the write format is not parquet when users are checking properties.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1457640453
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
If file format is not set in `rawProperties` but at write time, parquet compression codec will still be persisted since the default format is `parquet`. An example is `org.apache.iceberg.flink.sink.TestCompressionSettings`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#issuecomment-1906654805
@amogh-jahagirdar, I'm not sure I agree with this change. It seems to me that this is the Parquet default regardless of whether the default write format is Avro or ORC. It's a format-specific property so it doesn't apply in those cases. And if the table switches to Parquet at write time or is reconfigured, we want to have the new default configuration set.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1456949302
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
+ TableProperties.DEFAULT_FILE_FORMAT, TableProperties.DEFAULT_FILE_FORMAT_DEFAULT);
+ String deleteFileFormat =
+ rawProperties.getOrDefault(TableProperties.DELETE_DEFAULT_FILE_FORMAT, format);
+
// explicitly set defaults that apply only to new tables
- persistedProperties.put(
- TableProperties.PARQUET_COMPRESSION,
- TableProperties.PARQUET_COMPRESSION_DEFAULT_SINCE_1_4_0);
+ if (format.equalsIgnoreCase(FileFormat.PARQUET.name())
+ || deleteFileFormat.equalsIgnoreCase(FileFormat.PARQUET.name())) {
+ persistedProperties.put(
+ TableProperties.PARQUET_COMPRESSION,
+ TableProperties.PARQUET_COMPRESSION_DEFAULT_SINCE_1_4_0);
Review Comment:
I'm thinking there maybe a better pattern for these sorts of changes down the line. Some notion of version specific properties which will only be set on new tables based on the Iceberg version. But that idea is rather abstract at the moment and would require wider discussion.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#issuecomment-2408755023
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1458123535
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
Well if I think about it, what we are persisting at the table property level is a default for the table. Whether or not the Flink/Spark write options override at runtime doesn't really matter. For example, if they override with ORC in the runtime option and we end up persisting parquet compression doesn't really matter because if they later perform a write *without* the runtime operation we would be writing ZSTD parquet because we would be falling back to the table property.
I could see it being confusing though if everyone relied on the engine dynamic config for all their writes (always specifying in the write option) rather than the table property.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#issuecomment-1919889634
It looks like we don't have consensus that this is the right direction, so I'm going to at least remove it from the 1.5.0 milestone. We can discuss it more at the next community sync maybe?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#issuecomment-2424319115
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1456949302
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
+ TableProperties.DEFAULT_FILE_FORMAT, TableProperties.DEFAULT_FILE_FORMAT_DEFAULT);
+ String deleteFileFormat =
+ rawProperties.getOrDefault(TableProperties.DELETE_DEFAULT_FILE_FORMAT, format);
+
// explicitly set defaults that apply only to new tables
- persistedProperties.put(
- TableProperties.PARQUET_COMPRESSION,
- TableProperties.PARQUET_COMPRESSION_DEFAULT_SINCE_1_4_0);
+ if (format.equalsIgnoreCase(FileFormat.PARQUET.name())
+ || deleteFileFormat.equalsIgnoreCase(FileFormat.PARQUET.name())) {
+ persistedProperties.put(
+ TableProperties.PARQUET_COMPRESSION,
+ TableProperties.PARQUET_COMPRESSION_DEFAULT_SINCE_1_4_0);
Review Comment:
I'm thinking there maybe a better pattern for these sorts of changes down the line. Some notion of version specific properties which will only be set on new tables based on the Iceberg version and other conditions. But that idea is rather abstract at the moment and would require wider discussion.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1457616434
##########
core/src/test/java/org/apache/iceberg/TestTableMetadata.java:
##########
@@ -1729,6 +1729,46 @@ public void testNoTrailingLocationSlash() {
meta.location());
}
+ @Test
+ public void testCompressionDefaultPropertyForParquet() {
+ Map<String, String> properties =
+ ImmutableMap.of(TableProperties.DEFAULT_FILE_FORMAT, FileFormat.PARQUET.name());
+ TableMetadata metadata =
+ TableMetadata.newTableMetadata(
+ TEST_SCHEMA, SPEC_5, SORT_ORDER_3, "/some-location/", properties);
+
+ Assertions.assertThat(metadata.property(TableProperties.PARQUET_COMPRESSION, null))
+ .withFailMessage(
Review Comment:
Read the docs, I see the difference between the two now :)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1458112966
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
Ah I see, you're talking about the dynamic write configuration options that exist at the engine level ("write-format") for Spark and Flink. Yeah that's true, this doesn't really solve that case which would still be confusing for users. I'll think a bit more about a good solution for that then.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1458112966
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
Ah I see, you're talking about the dynamic write configuration options that exist at the engine level ("write-format") for Spark and Flink. Yeah that's true, this doesn't really address that case which maybe could be confusing for users. I'll think a bit more about a good solution for that then.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#issuecomment-1912799517
Is there a problem in persisting these properties even if the underlying file format is not Parquet?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1457704960
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
Not sure if I follow, isn't that the right behavior? If the default format when writing is parquet then the parquet compression codec should be set.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1457112913
##########
core/src/test/java/org/apache/iceberg/TestTableMetadata.java:
##########
@@ -1729,6 +1729,46 @@ public void testNoTrailingLocationSlash() {
meta.location());
}
+ @Test
+ public void testCompressionDefaultPropertyForParquet() {
+ Map<String, String> properties =
+ ImmutableMap.of(TableProperties.DEFAULT_FILE_FORMAT, FileFormat.PARQUET.name());
+ TableMetadata metadata =
+ TableMetadata.newTableMetadata(
+ TEST_SCHEMA, SPEC_5, SORT_ORDER_3, "/some-location/", properties);
+
+ Assertions.assertThat(metadata.property(TableProperties.PARQUET_COMPRESSION, null))
+ .withFailMessage(
Review Comment:
I don't think we should be using `withFailMessage()` because it will hide all contextual information and will only show this message. We rather want to use `.as()` in all of these cases here
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1458123535
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
Well if I think about it, what we are persisting at the table property level is a default for the table. Whether or not the Flink/Spark write options override at runtime doesn't really matter. For example, if they override with ORC in the runtime option and we end up persisting parquet compression doesn't really matter because if they later perform a write *without* the runtime operation we would be writing ZSTD parquet because we would be falling back to the table property.
I could see it being confusing though if everyone overrides their engine config for all their writes (always specifying in the write option) rather than the table property.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1458258796
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
I believe it should since in Trino the only way to specify the format is just at the table property.
Trino does not have any session properties in the Iceberg connector for overriding the file format at runtime as far as I can tell. https://github.com/trinodb/trino/blob/master/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergSessionProperties.java#L57
I'll run an end to end test with Trino and this PR to make sure this PR does solve the problem in #9490.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9503:
URL: https://github.com/apache/iceberg/pull/9503#discussion_r1459520414
##########
core/src/main/java/org/apache/iceberg/TableMetadata.java:
##########
@@ -90,10 +90,19 @@ private static Map<String, String> unreservedProperties(Map<String, String> rawP
private static Map<String, String> persistedProperties(Map<String, String> rawProperties) {
Map<String, String> persistedProperties = Maps.newHashMap();
+ String format =
+ rawProperties.getOrDefault(
Review Comment:
Here is a smoke test with ORC format in Trino before and after the change. Before the change the test passes because Parquet compression was defined with ZSTD, after the upgrade there's no such table property.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix setting updated parquet compression property [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #9503: Core: Fix setting updated parquet compression property
URL: https://github.com/apache/iceberg/pull/9503
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org