Core: Add id tracking to MetricsConfig #17022
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ | |
| import javax.annotation.concurrent.Immutable; | ||
| import org.apache.iceberg.MetricsModes.MetricsMode; | ||
| import org.apache.iceberg.exceptions.ValidationException; | ||
| import org.apache.iceberg.relocated.com.google.common.base.Preconditions; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Maps; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Sets; | ||
|
|
@@ -49,27 +49,37 @@ | |
| public final class MetricsConfig implements Serializable { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(MetricsConfig.class); | ||
|
|
||
| // Disable metrics by default for wide tables to prevent excessive metadata | ||
| private static final MetricsMode DEFAULT_MODE = | ||
| MetricsModes.fromString(DEFAULT_WRITE_METRICS_MODE_DEFAULT); | ||
| private static final MetricsConfig DEFAULT = | ||
| new MetricsConfig(ImmutableMap.of(), DEFAULT_MODE, ImmutableMap.of()); | ||
| private static final MetricsConfig POSITION_DELETE_MODE = | ||
| new MetricsConfig( | ||
| ImmutableMap.of( | ||
| MetadataColumns.DELETE_FILE_PATH.name(), | ||
| MetricsModes.Full.get(), | ||
| MetadataColumns.DELETE_FILE_POS.name(), | ||
| MetricsModes.Full.get()), | ||
| MetricsModes.None.get(), | ||
| ImmutableMap.of( | ||
| MetadataColumns.DELETE_FILE_PATH.fieldId(), | ||
| MetadataColumns.DELETE_FILE_PATH.name(), | ||
| MetadataColumns.DELETE_FILE_POS.fieldId(), | ||
| MetadataColumns.DELETE_FILE_POS.name())); | ||
|
|
||
| private final Map<String, MetricsMode> columnModes; | ||
| private final MetricsMode defaultMode; | ||
| private final Map<Integer, String> idToName; | ||
|
|
||
| private MetricsConfig( | ||
| Map<String, MetricsMode> columnModes, | ||
| MetricsMode defaultMode, | ||
| Map<Integer, String> idToName) { | ||
| this.columnModes = SerializableMap.copyOf(columnModes).immutableMap(); | ||
| this.defaultMode = defaultMode; | ||
| this.idToName = idToName != null ? SerializableMap.copyOf(idToName).immutableMap() : null; | ||
| } | ||
|
|
||
| public static MetricsConfig getDefault() { | ||
|
|
@@ -84,13 +94,23 @@ public static MetricsConfig forPositionDelete() { | |
| * Creates a metrics config from table configuration. | ||
| * | ||
| * @param props table configuration | ||
| * @deprecated use {@link MetricsConfig#forTable(Table)}. Will be removed in 1.13.0 | ||
| */ | ||
| @Deprecated | ||
| public static MetricsConfig fromProperties(Map<String, String> props) { | ||
| return from(props, null, null); | ||
| } | ||
|
|
||
| /** | ||
| * Validates metrics config properties with the given schema. | ||
| * | ||
| * @param props table properties | ||
| * @param schema table schema | ||
| */ | ||
| public static void validate(Map<String, String> props, Schema schema) { | ||
| from(props, schema, null).validateReferencedColumns(schema); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a metrics config from a table. | ||
| * | ||
|
|
@@ -100,32 +120,42 @@ public static MetricsConfig forTable(Table table) { | |
| return from(table.properties(), table.schema(), table.sortOrder()); | ||
| } | ||
|
|
||
| public Iterable<Integer> metricsFieldIds() { | ||
| Preconditions.checkState(idToName != null, "Cannot resolve column mode by ID: missing schema"); | ||
| return idToName.keySet(); | ||
| } | ||
|
|
||
| public MetricsMode columnMode(int id) { | ||
| Preconditions.checkState(idToName != null, "Cannot resolve column mode by ID: missing schema"); | ||
| String name = idToName.get(id); | ||
| if (name != null) { | ||
| return columnMode(name); | ||
| } | ||
|
|
||
| return defaultMode; | ||
| } | ||
|
|
||
| public MetricsMode columnMode(String columnAlias) { | ||
| return columnModes.getOrDefault(columnAlias, defaultMode); | ||
| } | ||
|
|
||
| public void validateReferencedColumns(Schema schema) { | ||
| for (String column : columnModes.keySet()) { | ||
| Types.NestedField field = schema.findField(column); | ||
| ValidationException.check( | ||
| field != null, | ||
| "Invalid metrics config, could not find column %s from table prop %s in schema %s", | ||
| column, | ||
| METRICS_MODE_COLUMN_CONF_PREFIX + column, | ||
| schema); | ||
|
|
||
| ValidationException.check( | ||
|
rdblue marked this conversation as resolved.
|
||
| null == idToName || column.equals(idToName.get(field.fieldId())), | ||
|
Contributor
There was a problem hiding this comment. was thinking about the field rename and preserve the field id but I believe it's already covered in schema update https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/SchemaUpdate.java#L517-L524 |
||
| "Incorrect field name for id %s: %s (expected %s)", | ||
| field.fieldId(), | ||
| column, | ||
| idToName.get(field.fieldId())); | ||
| } | ||
| } | ||
|
|
||
| static Set<Integer> limitFieldIds(Schema schema, int limit) { | ||
|
|
@@ -225,6 +255,7 @@ public Set<Integer> map( | |
| */ | ||
| public static MetricsConfig from(Map<String, String> props, Schema schema, SortOrder order) { | ||
| int maxInferredDefaultColumns = maxInferredColumnDefaults(props); | ||
| Map<Integer, String> idToName = Maps.newHashMap(); | ||
| Map<String, MetricsMode> columnModes = Maps.newHashMap(); | ||
|
|
||
| // Handle user override of default mode | ||
|
|
@@ -237,13 +268,20 @@ public static MetricsConfig from(Map<String, String> props, Schema schema, SortO | |
| } else if (schema == null) { | ||
| defaultMode = DEFAULT_MODE; | ||
| } else { | ||
| Set<Integer> ids = TypeUtil.getProjectedIds(schema); | ||
|
rdblue marked this conversation as resolved.
|
||
| if (ids.size() <= maxInferredDefaultColumns) { | ||
| for (int id : ids) { | ||
| idToName.put(id, schema.findColumnName(id)); | ||
| } | ||
|
|
||
| // there are less than the inferred limit (including structs), so the default is used | ||
| // everywhere | ||
| defaultMode = DEFAULT_MODE; | ||
| } else { | ||
| for (Integer id : limitFieldIds(schema, maxInferredDefaultColumns)) { | ||
| String name = schema.findColumnName(id); | ||
| idToName.put(id, name); | ||
| columnModes.put(name, DEFAULT_MODE); | ||
| } | ||
|
|
||
| // all other columns don't use metrics | ||
|
|
@@ -254,18 +292,29 @@ public static MetricsConfig from(Map<String, String> props, Schema schema, SortO | |
| // First set sorted column with sorted column default (can be overridden by user) | ||
| MetricsMode sortedColDefaultMode = sortedColumnDefaultMode(defaultMode); | ||
| Set<String> sortedCols = SortOrderUtil.orderPreservingSortedColumns(order); | ||
| sortedCols.forEach( | ||
| name -> { | ||
| columnModes.put(name, sortedColDefaultMode); | ||
| Types.NestedField field = schema != null ? schema.findField(name) : null; | ||
| if (field != null) { | ||
| idToName.put(field.fieldId(), name); | ||
| } | ||
| }); | ||
|
|
||
| // Handle user overrides of defaults | ||
| for (String key : props.keySet()) { | ||
| if (key.startsWith(METRICS_MODE_COLUMN_CONF_PREFIX)) { | ||
| String columnAlias = key.replaceFirst(METRICS_MODE_COLUMN_CONF_PREFIX, ""); | ||
| MetricsMode mode = parseMode(props.get(key), defaultMode, "column " + columnAlias); | ||
| columnModes.put(columnAlias, mode); | ||
| Types.NestedField field = schema != null ? schema.findField(columnAlias) : null; | ||
| if (field != null) { | ||
| idToName.put(field.fieldId(), columnAlias); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return new MetricsConfig(columnModes, defaultMode, idToName); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -309,19 +358,4 @@ private static MetricsMode parseMode(String modeString, MetricsMode fallback, St | |
| return fallback; | ||
| } | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. These were moved above the |
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return an empty list if idToName is null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think it should fail if
idToNameis null because there could be a configuration, but the config wasn't initialized the right way. IfidToNameis null, then it was initialized without a schema, which we want to stop doing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this (and
columnMode(int)) to check thatidToNameis present and throw an exception if not. That way, when we get rid offromProperties, we can get rid of the check.