Core: use ZSTD compression parquet by default for new tables by szehon-ho · Pull Request #8299 · apache/iceberg

szehon-ho · 2023-08-12T01:16:00Z

This is another attempt at #8158, based on #8158 (comment).

szehon-ho · 2023-08-12T01:16:41Z

Will need to fix tests, but the idea is here, cc @dbtsai , @RussellSpitzer

dbtsai · 2023-08-12T03:28:39Z

      this.identifier = identifier;
      this.schema = schema;
      this.tableProperties.putAll(tableDefaultProperties());
+      // Explicitly set ZSTD for new tables


Maybe // Explicitly set Parquet compression codec for new tables

dbtsai · 2023-08-12T03:31:43Z

+      // Explicitly set ZSTD for new tables
+      this.tableProperties.put(
+          TableProperties.PARQUET_COMPRESSION,
+          TableProperties.PARQUET_COMPRESSION_NEW_TABLE_DEFAULT);


this.tableProperties.put( TableProperties.PARQUET_COMPRESSION, TableProperties.PARQUET_COMPRESSION_DEFAULT);

so we don't need to introduce a new conf, PARQUET_COMPRESSION_NEW_TABLE_DEFAULT

dbtsai · 2023-08-12T03:33:45Z

+                  Map<String, String> newProperties = Maps.newHashMap(base.properties());
+                  newProperties.put(
+                      TableProperties.PARQUET_COMPRESSION,
+                      TableProperties.PARQUET_COMPRESSION_DEFAULT);


if (base.properties().get(TableProperties.PARQUET_COMPRESSION) == null) { Map<String, String> newProperties = Maps.newHashMap(base.properties()); newProperties.put(TableProperties.PARQUET_COMPRESSION, "gzip");

dbtsai · 2023-08-12T03:34:12Z

  public static final String PARQUET_COMPRESSION = "write.parquet.compression-codec";
  public static final String DELETE_PARQUET_COMPRESSION = "write.delete.parquet.compression-codec";
  public static final String PARQUET_COMPRESSION_DEFAULT = "gzip";
+  public static final String PARQUET_COMPRESSION_NEW_TABLE_DEFAULT = "zstd";


Then, we don't need PARQUET_COMPRESSION_NEW_TABLE_DEFAULT

dbtsai · 2023-08-12T06:05:24Z

Minor comments above. Thank you for working on this followup. @szehon-ho

szehon-ho · 2023-08-14T23:23:23Z

Thanks for looking. Updated and fixed a round of tests.

aokolnychyi · 2023-08-15T16:17:48Z

      this.schema = schema;
      this.tableProperties.putAll(tableDefaultProperties());
+
+      // Explicitly set default Parquet compression codecs for new tables


This seems to only cover catalogs that extend this class.
Is it something we can address in TableMetadata to avoid changes in multiple places?

Theroetically we could but it sounds strange to me, making the TableMetadata object construct by default with a few properties set , as its a public class?

Well, the problem here is that we update multiple places with the same logic and it only covers some implementations. I am not sure how I feel about changing TableMetadata either. Let me think.

Took a look, REST catalog table builder doesnt seem to build TableMetadata themselves (gets from the response object). So this wont help that case. I tried to organize the code a little bit into TableMetadata class in any case

aokolnychyi · 2023-08-15T16:20:44Z

      old: "method org.apache.iceberg.view.ViewBuilder org.apache.iceberg.view.ViewBuilder::withQueryColumnNames(java.util.List<java.lang.String>)"
      justification: "Acceptable break due to updating View APIs and the View Spec"
+    org.apache.iceberg:iceberg-core:
+    - code: "java.field.constantValueChanged"


Another idea could be to add a new constant for new tables and persist the codec only during table creation, while still relying on the old PARQUET_COMPRESSION_DEFAULT for existing tables.

That way we won't have to change SnapshotProducer and encode old values.

That being said, I am not against the current implementation.

I had that earlier (not change the constant), I think @dbtsai wanted to do it the other way.

szehon-ho · 2023-08-15T20:58:40Z

Looks like there's some open question. First, I assume we want to do this, to reduce the impact for user (keep gzip for old table and use zstd for new tables). If that's the case, the questions:

Should we change TableMetadata constructor to set these two compression table properties for new TableMetadata? Core: use ZSTD compression parquet by default for new tables #8299 (comment)

Currently changing each catalog risks missing this change in custom catalog.
But TableMetadata is public API and it may be strange to have new instances default with 2 properties. But then again, maybe this is a good direction and we should start returning with other default table property as well (which is a way for us to start persisting the defaults).

Should we just have two compression constants (DEFAULT and NEW_TABLE_DEFAULT) and not have to persist GZIP explicitly in SnapshotProducer? Then, the only change would be making createTable persist ZSTD default.
Core: use ZSTD compression parquet by default for new tables #8299 (comment)

Its less intrusive code in SnapshotProducer, and doesnt change a public constant. But it just looks a bit weird to have two defaults.

szehon-ho · 2023-08-15T21:01:21Z

@rdblue @nastra @jackye1995 @stevenzwu @danielcweeks , could you guys take a look? Seems like it can go either way here.

nastra · 2023-08-16T13:20:15Z

I kind of like option 2) where we would essentially have PARQUET_COMPRESSION_DEFAULT and PARQUET_COMPRESSION_DEFAULT_SINCE_1_4_0, indicating that the newer default should be used on new tables since Iceberg 1.4.0 (or any other name that basically indicates that this is the new default for new tables).

szehon-ho · 2023-08-17T13:48:52Z

Thanks, makes sense to me, added new constant PARQUET_COMPRESSION_DEFAULT_SINCE_1_4_0

aokolnychyi · 2023-08-24T21:46:09Z

  public static final String PARQUET_COMPRESSION = "write.parquet.compression-codec";
  public static final String DELETE_PARQUET_COMPRESSION = "write.delete.parquet.compression-codec";
  public static final String PARQUET_COMPRESSION_DEFAULT = "gzip";
+  public static final String PARQUET_COMPRESSION_DEFAULT_SINCE_1_4_0 = "zstd";


Let me think. I do like the name but I also wonder if it sends a message the new default applies to all existing tables that did not provide a default value. It is probably not what happens as we only use this value in new tables.

cc @nastra @RussellSpitzer @szehon-ho @rdblue

I can go either way here, up to you @szehon-ho.

aokolnychyi · 2023-08-24T21:47:55Z

        schema, spec, sortOrder, location, unreservedProperties(properties), formatVersion);
  }

+  public static TableMetadata newTableMetadataWithDefaultProperties(


The name kind of indicates that all defaults are persisted. Are there any better alternatives?

After thinking about this a bit more, I'd probably just update the existing method given that we modify all places we know and that's the behavior we want to achieve.

Alright then , just change the existing method, hope there's no breakages

aokolnychyi · 2023-08-29T20:00:13Z

+        TableProperties.DELETE_PARQUET_COMPRESSION,
+        TableProperties.PARQUET_COMPRESSION_DEFAULT_SINCE_1_4_0);
+    defaults.putAll(unreservedProperties(properties));
+    int formatVersion =


I wonder whether we should add a method for fetching the format version, it is used in 3 places now.

private static int formatVersion(Map<String, String> properties) { return PropertyUtil.propertyAsInt( properties, TableProperties.FORMAT_VERSION, DEFAULT_TABLE_FORMAT_VERSION); }

I think its a good idea, I no longer make a new method though now so it may be unrelated as a change.

szehon-ho · 2023-08-31T22:41:07Z

@jerqi FYI i commented out the failing test for now, I think as @aokolnychyi wanted to get this one for 1.4, and we can also look at the fixes in the meantime.

roryqi · 2023-08-31T22:56:53Z

@jerqi FYI i commented out the failing test for now, I think as @aokolnychyi wanted to get this one for 1.4, and we can also look at the fixes in the meantime.

OK, I got it. I will fix the issue as soon as I can.

szehon-ho · 2023-09-07T10:28:39Z

Rebased on #8438 , TestCompression tests now pass, fyi @aokolnychyi when you are back

aokolnychyi · 2023-09-20T17:16:13Z

Szehon is on parental leave. I took his work and added Spark 3.5 changes in #8593. Closing this.

github-actions Bot added core hive labels Aug 12, 2023

dbtsai reviewed Aug 12, 2023

View reviewed changes

github-actions Bot added spark flink MR labels Aug 14, 2023

szehon-ho force-pushed the new_zstd_default branch from a71a5a4 to 4d0e4d7 Compare August 14, 2023 23:20

aokolnychyi reviewed Aug 15, 2023

View reviewed changes

szehon-ho force-pushed the new_zstd_default branch from f33366f to ac6e6fc Compare August 15, 2023 20:44

szehon-ho force-pushed the new_zstd_default branch 3 times, most recently from d09c720 to 6ec04c5 Compare August 16, 2023 23:03

nastra reviewed Aug 17, 2023

View reviewed changes

Comment thread hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java Outdated

aokolnychyi reviewed Aug 24, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java Outdated

aokolnychyi reviewed Aug 24, 2023

View reviewed changes

aokolnychyi added this to the Iceberg 1.4.0 milestone Aug 24, 2023

aokolnychyi reviewed Aug 29, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated

szehon-ho force-pushed the new_zstd_default branch from c08e78b to 43e3cab Compare August 31, 2023 00:23

szehon-ho mentioned this pull request Aug 31, 2023

Spark 3.4: Add write options to override the compression properties of the table #8313

Merged

szehon-ho added 4 commits September 7, 2023 10:15

Core: use ZSTD compression parquet by default for new tables

49dd484

Properly override delete file compression

ff146aa

Comment out delete file test under fix

0e970dd

Re-enable commented out test after fix of apache#8438

3e8b227

szehon-ho force-pushed the new_zstd_default branch from 47b9ce5 to 3e8b227 Compare September 7, 2023 02:35

aokolnychyi mentioned this pull request Sep 20, 2023

Core: Default to zstd compression for Parquet in new tables #8593

Merged

aokolnychyi closed this Sep 20, 2023

Conversation

szehon-ho commented Aug 12, 2023

Uh oh!

szehon-ho commented Aug 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbtsai commented Aug 12, 2023

Uh oh!

szehon-ho commented Aug 14, 2023

Uh oh!

aokolnychyi Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szehon-ho commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nastra commented Aug 16, 2023

Uh oh!

Uh oh!

szehon-ho commented Aug 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szehon-ho commented Aug 31, 2023

Uh oh!

roryqi commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szehon-ho commented Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aokolnychyi commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

aokolnychyi Aug 15, 2023 •

edited

Loading

aokolnychyi Aug 15, 2023 •

edited

Loading

szehon-ho commented Aug 15, 2023 •

edited

Loading

szehon-ho commented Aug 15, 2023 •

edited

Loading

aokolnychyi Aug 29, 2023 •

edited

Loading

roryqi commented Aug 31, 2023 •

edited

Loading

szehon-ho commented Sep 7, 2023 •

edited

Loading