Skip to content

Core: Add id tracking to MetricsConfig #17022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rdblue merged 13 commits into from
Jul 7, 2026
Merged

Core: Add id tracking to MetricsConfig #17022

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4becc0c
Core: Add id tracking to MetricsConfig.
rdblue Jun 29, 2026
173594a
Apply spotless.
rdblue Jun 30, 2026
42b26cc
Update older Spark versions.
rdblue Jun 30, 2026
8e1e7b2
Fixup last commit's typo.
rdblue Jun 30, 2026
ab334da
Add default implementation of hasBounds().
rdblue Jun 30, 2026
bdcb363
Update revapi to remove deprecated method.
rdblue Jun 30, 2026
306effe
Use longer error message to avoid changing another test file.
rdblue Jun 30, 2026
1cea2e1
Fix Amogh's review comments.
rdblue Jul 1, 2026
ba1473c
Fix Flink tests after changes to TestWriterMetrics.
rdblue Jul 1, 2026
19f5313
Fix import.
rdblue Jul 1, 2026
abb2417
Fix more tests.
rdblue Jul 1, 2026
a706936
Make field final.
rdblue Jul 1, 2026
de8f061
Fix imports.
rdblue Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .palantir/revapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,9 @@ acceptedBreaks:
- code: "java.field.removedWithConstant"
old: "field org.apache.iceberg.PartitionStatsHandler.PARTITION_FIELD_NAME"
justification: "Removed deprecated functionality for partition stats"
- code: "java.method.removed"
old: "method org.apache.iceberg.MetricsConfig org.apache.iceberg.MetricsConfig::forPositionDelete(org.apache.iceberg.Table)"
justification: "Deprecated for removal in 1.12.0"
- code: "java.method.removed"
old: "method org.apache.iceberg.Schema org.apache.iceberg.PartitionStatsHandler::schema(org.apache.iceberg.types.Types.StructType,\
\ int)"
Expand Down
127 changes: 80 additions & 47 deletions core/src/main/java/org/apache/iceberg/MetricsConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
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.Joiner;
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;
Expand All @@ -49,27 +48,39 @@
public final class MetricsConfig implements Serializable {

private static final Logger LOG = LoggerFactory.getLogger(MetricsConfig.class);
private static final Joiner DOT = Joiner.on('.');

// 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);
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()),
DEFAULT_MODE);
DEFAULT_MODE,
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 Map<Integer, String> idToName;
Comment thread
rdblue marked this conversation as resolved.
Outdated

private MetricsConfig(Map<String, MetricsMode> columnModes, MetricsMode defaultMode) {
private MetricsConfig(
Map<String, MetricsMode> columnModes,
MetricsMode defaultMode,
Map<Integer, String> idToName) {
this.columnModes = SerializableMap.copyOf(columnModes).immutableMap();
this.defaultMode = defaultMode;
if (idToName != null) {

@amogh-jahagirdar amogh-jahagirdar Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok it's a bit confusing because there are accessor APIs below which assume that idToName is not null. Currently, we pass it in, but I'm not sure I understand the intent of if (idToName != null) check is. It's a private constructor, can't we force every where else to pass in the idToName and then work off that assumption that it's always defined?

@rdblue rdblue Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, I don't think that we can force every code path to provide a schema, which is how this is derived. That would be more changes and those code paths don't need to use IDs, so I left it out. For instance, fromProperties is still defined (until next release) and we can't fake a schema for that call. We could cause it to fail, but that would defeat the purpose of deprecating it.

The approach I took here was to use idToName if it is present. I thought this was a better option than causing external calls to fromProperties to fail. We can tighten this up after the next release when we can guarantee all code paths pass a schema.

@amogh-jahagirdar amogh-jahagirdar Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, okay I missed the original fromProperties, yeah in that case we can just keep it as is.

@rdblue rdblue Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added assertions to ensure we don't get NPEs. We can remove them once idToName is always present.

this.idToName = SerializableMap.copyOf(idToName).immutableMap();
}
}

public static MetricsConfig getDefault() {
Expand All @@ -84,13 +95,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 2.0.0
* @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.
*
Expand All @@ -100,32 +121,40 @@ public static MetricsConfig forTable(Table table) {
return from(table.properties(), table.schema(), table.sortOrder());
}

/**
* Creates a metrics config for a position delete file.
*
* @param table an Iceberg table
* @deprecated This method is deprecated as of version 1.11.0 and will be removed in 1.12.0.
* Position deletes that include row data are no longer supported. Use {@link
* #forPositionDelete()} instead.
*/
@Deprecated
public static MetricsConfig forPositionDelete(Table table) {
ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
public Iterable<Integer> metricsFieldIds() {
return idToName.keySet();

@amogh-jahagirdar amogh-jahagirdar Jul 1, 2026

Copy link
Copy Markdown
Contributor

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?

@rdblue rdblue Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

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 idToName is null because there could be a configuration, but the config wasn't initialized the right way. If idToName is null, then it was initialized without a schema, which we want to stop doing.

@rdblue rdblue Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

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 that idToName is present and throw an exception if not. That way, when we get rid of fromProperties, we can get rid of the check.

}

columnModes.put(MetadataColumns.DELETE_FILE_PATH.name(), MetricsModes.Full.get());
columnModes.put(MetadataColumns.DELETE_FILE_POS.name(), MetricsModes.Full.get());
public MetricsMode columnMode(int id) {
String name = idToName.get(id);
if (name != null) {
return columnMode(name);
}

MetricsConfig tableConfig = forTable(table);
return defaultMode;
}

MetricsMode defaultMode = tableConfig.defaultMode;
tableConfig.columnModes.forEach(
(columnAlias, mode) -> {
String positionDeleteColumnAlias =
DOT.join(MetadataColumns.DELETE_FILE_ROW_FIELD_NAME, columnAlias);
columnModes.put(positionDeleteColumnAlias, mode);
});
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);

return new MetricsConfig(columnModes.build(), defaultMode);
ValidationException.check(
Comment thread
rdblue marked this conversation as resolved.
null == idToName || idToName.get(field.fieldId()).equals(column),
"Incorrect field name for id %s: %s (expected %s)",
field.fieldId(),
column,
idToName.get(field.fieldId()));
}
}

static Set<Integer> limitFieldIds(Schema schema, int limit) {
Expand Down Expand Up @@ -225,6 +254,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
Expand All @@ -237,13 +267,20 @@ public static MetricsConfig from(Map<String, String> props, Schema schema, SortO
} else if (schema == null) {
defaultMode = DEFAULT_MODE;
} else {
if (TypeUtil.getProjectedIds(schema).size() <= maxInferredDefaultColumns) {
Set<Integer> ids = TypeUtil.getProjectedIds(schema);
Comment thread
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)) {
columnModes.put(schema.findColumnName(id), DEFAULT_MODE);
String name = schema.findColumnName(id);
idToName.put(id, name);
columnModes.put(name, DEFAULT_MODE);
}

// all other columns don't use metrics
Expand All @@ -254,18 +291,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(sc -> columnModes.put(sc, sortedColDefaultMode));
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);
return new MetricsConfig(columnModes, defaultMode, idToName);
}

/**
Expand Down Expand Up @@ -309,19 +357,4 @@ private static MetricsMode parseMode(String modeString, MetricsMode fallback, St
return fallback;
}
}

public void validateReferencedColumns(Schema schema) {

@rdblue rdblue Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were moved above the private and static methods. It was hard to find them before.

for (String column : columnModes.keySet()) {
ValidationException.check(
schema.findField(column) != null,
"Invalid metrics config, could not find column %s from table prop %s in schema %s",
column,
METRICS_MODE_COLUMN_CONF_PREFIX + column,
schema);
}
}

public MetricsMode columnMode(String columnAlias) {
return columnModes.getOrDefault(columnAlias, defaultMode);
}
}
27 changes: 26 additions & 1 deletion core/src/main/java/org/apache/iceberg/MetricsModes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,12 @@ public static MetricsMode fromString(String mode) {
*
* <p>Implementations must be immutable.
*/
public interface MetricsMode extends Serializable {}
public interface MetricsMode extends Serializable {
default boolean hasBounds() {

@nastra nastra Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this used anywhere? it's not clear from the PR description why this is being added

throw new UnsupportedOperationException(
"Unexpected implementation of MetricsMode without hasBounds");
}
}

/**
* Under this mode, value_counts, null_value_counts, nan_value_counts, lower_bounds, upper_bounds
Expand All @@ -72,6 +77,11 @@ public static None get() {
return INSTANCE;
}

@Override
public boolean hasBounds() {
return false;
}

@Override
public String toString() {
return "none";
Expand All @@ -86,6 +96,11 @@ public static Counts get() {
return INSTANCE;
}

@Override
public boolean hasBounds() {
return false;
}

@Override
public String toString() {
return "counts";
Expand All @@ -108,6 +123,11 @@ public static Truncate withLength(int length) {
return new Truncate(length);
}

@Override
public boolean hasBounds() {
return true;
}

public int length() {
return length;
}
Expand Down Expand Up @@ -145,6 +165,11 @@ public static Full get() {
return INSTANCE;
}

@Override
public boolean hasBounds() {
return true;
}

@Override
public String toString() {
return "full";
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/iceberg/PropertiesUpdate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public Map<String, String> apply() {

// Validate the metrics
if (base != null && base.schema() != null) {
MetricsConfig.fromProperties(newProperties).validateReferencedColumns(base.schema());
MetricsConfig.validate(newProperties, base.schema());
}

return newProperties;
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/iceberg/TableMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ static TableMetadata newTableMetadata(

// Validate the metrics configuration. Note: we only do this on new tables to we don't
// break existing tables.
MetricsConfig.fromProperties(properties).validateReferencedColumns(schema);
MetricsConfig.validate(properties, schema);

PropertyUtil.validateCommitProperties(properties);

Expand Down
4 changes: 2 additions & 2 deletions core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ public void testBoundsWithSpecificValues() {
@SuppressWarnings("deprecation")
void testBoundsForAllMetricsModes(String metricsMode) {
MetricsConfig metricsConfig =
MetricsConfig.fromProperties(
ImmutableMap.of(TableProperties.DEFAULT_WRITE_METRICS_MODE, metricsMode));
MetricsConfig.from(
ImmutableMap.of(TableProperties.DEFAULT_WRITE_METRICS_MODE, metricsMode), SCHEMA, null);
Metrics metrics =
FileGenerationUtil.generateRandomMetrics(
SCHEMA,
Expand Down
24 changes: 15 additions & 9 deletions core/src/test/java/org/apache/iceberg/TestMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ public void testMetricsModeForNestedStructFields() throws IOException {
MetricsModes.None.get().toString(),
TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX + "nestedStructCol.longCol",
MetricsModes.Full.get().toString());
MetricsConfig config = MetricsConfig.fromProperties(properties);
MetricsConfig config = MetricsConfig.from(properties, NESTED_SCHEMA, null);

Metrics metrics = getMetrics(NESTED_SCHEMA, config, buildNestedTestRecord());
MetricsWithStats metricsWithStats =
Expand Down Expand Up @@ -590,7 +590,8 @@ public void testNoneMetricsMode() throws IOException {
Metrics metrics =
getMetrics(
NESTED_SCHEMA,
MetricsConfig.fromProperties(ImmutableMap.of("write.metadata.metrics.default", "none")),
MetricsConfig.from(
ImmutableMap.of("write.metadata.metrics.default", "none"), NESTED_SCHEMA, null),
buildNestedTestRecord());
MetricsWithStats metricsWithStats =
new MetricsWithStats(metrics, MetricsUtil.fromMetrics(NESTED_SCHEMA, metrics));
Expand All @@ -613,8 +614,8 @@ public void testCountsMetricsMode() throws IOException {
Metrics metrics =
getMetrics(
NESTED_SCHEMA,
MetricsConfig.fromProperties(
ImmutableMap.of("write.metadata.metrics.default", "counts")),
MetricsConfig.from(
ImmutableMap.of("write.metadata.metrics.default", "counts"), NESTED_SCHEMA, null),
buildNestedTestRecord());
MetricsWithStats metricsWithStats =
new MetricsWithStats(metrics, MetricsUtil.fromMetrics(NESTED_SCHEMA, metrics));
Expand All @@ -638,7 +639,8 @@ public void testFullMetricsMode() throws IOException {
Metrics metrics =
getMetrics(
NESTED_SCHEMA,
MetricsConfig.fromProperties(ImmutableMap.of("write.metadata.metrics.default", "full")),
MetricsConfig.from(
ImmutableMap.of("write.metadata.metrics.default", "full"), NESTED_SCHEMA, null),
buildNestedTestRecord());
MetricsWithStats metricsWithStats =
new MetricsWithStats(metrics, MetricsUtil.fromMetrics(NESTED_SCHEMA, metrics));
Expand Down Expand Up @@ -675,8 +677,10 @@ public void testTruncateStringMetricsMode() throws IOException {
Metrics metrics =
getMetrics(
singleStringColSchema,
MetricsConfig.fromProperties(
ImmutableMap.of("write.metadata.metrics.default", "truncate(10)")),
MetricsConfig.from(
ImmutableMap.of("write.metadata.metrics.default", "truncate(10)"),
singleStringColSchema,
null),
record);
MetricsWithStats metricsWithStats =
new MetricsWithStats(metrics, MetricsUtil.fromMetrics(singleStringColSchema, metrics));
Expand All @@ -702,8 +706,10 @@ public void testTruncateBinaryMetricsMode() throws IOException {
Metrics metrics =
getMetrics(
singleBinaryColSchema,
MetricsConfig.fromProperties(
ImmutableMap.of("write.metadata.metrics.default", "truncate(5)")),
MetricsConfig.from(
ImmutableMap.of("write.metadata.metrics.default", "truncate(5)"),
singleBinaryColSchema,
null),
record);
MetricsWithStats metricsWithStats =
new MetricsWithStats(metrics, MetricsUtil.fromMetrics(singleBinaryColSchema, metrics));
Expand Down
Loading
Loading