Disable metrics for very wide tables to reduce metadata size #3959

szehon-ho · 2022-01-24T19:59:28Z

Giving a try to the suggestion in the last community sync to address very-wide-table.

This changes the default MetricsMode from "truncate(16)" to "none" if there are over 100 columns (should it be counts?). This is overriden if the user explicitly sets a default, and of course if a user explicitly sets metrics on a particular column.

RussellSpitzer · 2022-01-25T02:11:57Z

core/src/main/java/org/apache/iceberg/TableProperties.java


  public static final String METRICS_MODE_COLUMN_CONF_PREFIX = "write.metadata.metrics.column.";
  public static final String DEFAULT_WRITE_METRICS_MODE = "write.metadata.metrics.default";
-  public static final String DEFAULT_WRITE_METRICS_MODE_DEFAULT = "truncate(16)";


This is changing a public API and to a new type. May need to reconsider this. Would probably be safer to just add the new variable below with a slightly different name?

Although if other folks don't mind, I think leaving this as is is also fine.

Sure, changed it back, it made the diff smaller

RussellSpitzer · 2022-01-25T02:22:47Z

core/src/main/java/org/apache/iceberg/MetricsConfig.java

+        finalDefaultMode = MetricsModes.fromString(defaultModeProp);
+      } catch (IllegalArgumentException err) {
+        // User override was invalid, log the error and use the default
+        LOG.warn("Ignoring invalid default metrics mode: {}", defaultModeProp, err);


I'm not sure why we had this as a log warn before, seems like this should just fail ... but I don't think we need to change the behavior yet

I might be mistaken, but I think it was the same behavior, warn log is still there. but in any case after changing the default from enum back to a string the diff now looks less. Now in this method only the original variable name "defaultMode" should be changed to avoid conflict.

Yeah I just don't know why we originally had this behavior ... seems odd to me 🤷

The idea here was to not fail writes just because a user messed up a tangentially related config property. I think it's better to allow pipelines to continue.

RussellSpitzer · 2022-01-25T18:35:41Z

@danielcweeks When we discussed this were you thinking about no metrics for tables with more than 100 columns? or no metrics for anything other than the first 100 columns.

Obviously in both cases we keep sort column metrics by default

szehon-ho · 2022-01-31T23:58:16Z

Hi @rdblue @danielcweeks , any thought on this? Thanks in advance

danielcweeks · 2022-02-04T22:25:56Z

@danielcweeks When we discussed this were you thinking about no metrics for tables with more than 100 columns? or no metrics for anything other than the first 100 columns.

Obviously in both cases we keep sort column metrics by default

@RussellSpitzer I would think it would be either all or nothing. I'm not sure we can speculate that the first 100 columns are somehow more meaningful than those that follow (though I guess you could make that argument).

For now it seems like just taking a simple approach would be better. There are some edge cases like what happens in the event that someone is incrementally adding columns and it crosses that threshold, but I wouldn't worry about it too much at this point.

danielcweeks · 2022-02-04T22:35:59Z

core/src/main/java/org/apache/iceberg/MetricsConfig.java

  public static MetricsConfig forTable(Table table) {
-    return from(table.properties(), table.sortOrder());
+    String defaultMode;
+    if (table.schema().columns().size() <= MAX_COLUMNS) {


Is there a way to override this behavior?

Good point, I can make a table config , does that sound reasonable?

"defaultMetricCollection : all" or something? Don't we already have that?

Also this feels like it should probably be a catalog level prop 🤷

Was initially thinking like 'write.metadata.metrics.max.columns'='100'? Yea good point maybe catalog level prop is cleaner

Took another look, and actually it's a bit messy to make this a catalog property in the current code (the Table is serialized and sent to writers, but Catalog is not..).

Was thinking it makes sense as a table property as well (ie, if i know this table is worth optimizing, set the max columns to be higher). May be a bit awkward to have a different catalog just for this. And if user really wants to have one global setting, this change is coming: #4011

I think this is okay for now. Tables will still respect whatever is set as write.metadata.metrics.default so this really just changes Iceberg's default in a reasonable way. It is also good to note that metrics for sort columns are automatically promoted to at least truncate[16] so it isn't as though we're losing all stats.

aokolnychyi · 2022-02-09T05:08:38Z

I am not sure how useful a table property is or whether we should even make it configurable. Specifically, I am worried about adding a rarely used table property. We are usually pretty conservative when it comes to adding new table properties. Is there a good use case for this to be configurable? It seems that if anyone needs to tune the metrics config, the existing framework is already good enough and allows us to set default and per-column modes. I think we should encourage users to set that.

Also, I think 100 is too big. I'd prefer it to be around 24-32. I've seen tables with 100 string columns and the metadata size already had an impact on the performance. Any thoughts on that? Keep in mind we always collect stats for sort columns and will collect stats for identity columns in the future.

This reverts commit 5c88e0e.

This reverts commit bcd10ed.

szehon-ho · 2022-02-10T22:01:37Z

Hi @aokolnychyi @RussellSpitzer @rdblue @danielcweeks if you guys want another look after the last comments, thanks

rdblue · 2022-02-13T23:59:01Z

core/src/main/java/org/apache/iceberg/MetricsConfig.java

+   * @param defaultMode default, if not set by user property
+   * @return metrics configuration
+   */
+  private static MetricsConfig from(Map<String, String> props, SortOrder order, String defaultMode) {


Instead of renaming the variable that is used everywhere, I'd just rename the incoming argument. There would be fewer changes if this used defaultDefaultMode or something.

rdblue · 2022-02-13T23:59:25Z

core/src/main/java/org/apache/iceberg/MetricsConfig.java

+    } else {
+      defaultMode = MetricsModes.None.get().toString();
+    }
+    return from(table.properties(), table.sortOrder(), defaultMode);


Nit: missing newline after control flow block and before return.

rdblue

I think this is about ready, but there is still some active discussion about overriding the number of columns. I'd probably opt to ignore this for now because users still have final control through write.metadata.metrics.default.

rdblue · 2022-02-14T16:50:55Z

Merging this. Thanks, @szehon-ho and everyone for the reviews!

szehon-ho · 2022-02-14T18:05:51Z

Thanks @rdblue for merge and all the reviewers! Sorry didn't see the last style comments before merge, i can prepare another pr for those

rdblue · 2022-02-15T17:12:10Z

@szehon-ho, don't worry about it. I didn't want to keep this outstanding for minor things like that.

…ize (apache#3959) Reference: (cherry picked from commit 67f4716)

…#3959)

github-actions bot added core data flink spark labels Jan 24, 2022

szehon-ho force-pushed the metrics_demotion_master branch from f815584 to c4145dd Compare January 24, 2022 22:02

Disable metrics for very wide tables to reduce metadata size

7631ee8

szehon-ho force-pushed the metrics_demotion_master branch from d1d8686 to 7631ee8 Compare January 25, 2022 00:30

szehon-ho requested review from RussellSpitzer, aokolnychyi and rdblue January 25, 2022 01:35

RussellSpitzer reviewed Jan 25, 2022

View reviewed changes

Address review comments

be2fff3

danielcweeks reviewed Feb 4, 2022

View reviewed changes

szehon-ho added 2 commits February 4, 2022 18:28

Make max columns configurable

bcd10ed

Add -1 as config value for unlimited

5c88e0e

szehon-ho added 2 commits February 10, 2022 09:53

Revert "Add -1 as config value for unlimited"

68dde57

This reverts commit 5c88e0e.

Revert "Make max columns configurable"

1c0bce5

This reverts commit bcd10ed.

szehon-ho force-pushed the metrics_demotion_master branch from f953922 to 4d40cd6 Compare February 10, 2022 18:10

Change max to 32

181cddc

szehon-ho force-pushed the metrics_demotion_master branch from 4d40cd6 to 181cddc Compare February 10, 2022 18:12

rdblue reviewed Feb 13, 2022

View reviewed changes

rdblue approved these changes Feb 14, 2022

View reviewed changes

danielcweeks approved these changes Feb 14, 2022

View reviewed changes

rdblue merged commit 67f4716 into apache:master Feb 14, 2022

hililiwei pushed a commit to hililiwei/iceberg that referenced this pull request Jun 29, 2022

[Backport] Core: Disable metrics for wide tables to reduce metadata s…

8196a22

…ize (apache#3959) Reference: (cherry picked from commit 67f4716)

rdblue mentioned this pull request Jul 6, 2022

Core: Update MetricsConfig to use a default for first 32 columns #5215

Merged

sunchao pushed a commit to sunchao/iceberg that referenced this pull request May 9, 2023

Core: Disable metrics for wide tables to reduce metadata size (apache…

06da294

…#3959)

hantangwangd mentioned this pull request Aug 26, 2024

[Iceberg]Support setting the max number of columns for which metrics are collected prestodb/presto#23468

Merged

6 tasks

Disable metrics for very wide tables to reduce metadata size #3959

Disable metrics for very wide tables to reduce metadata size #3959

Uh oh!

Conversation

szehon-ho commented Jan 24, 2022

Uh oh!

RussellSpitzer Jan 25, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Jan 25, 2022

Uh oh!

szehon-ho commented Jan 31, 2022

Uh oh!

danielcweeks commented Feb 4, 2022

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 Feb 4, 2022 • 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!

aokolnychyi commented Feb 9, 2022

Uh oh!

szehon-ho commented Feb 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

rdblue commented Feb 14, 2022

Uh oh!

szehon-ho commented Feb 14, 2022

Uh oh!

rdblue commented Feb 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RussellSpitzer Jan 25, 2022 •

edited

Loading

szehon-ho Feb 4, 2022 •

edited

Loading