Core: Add id tracking to MetricsConfig by rdblue · Pull Request #17022 · apache/iceberg

rdblue · 2026-06-30T18:00:27Z

Metrics modes are configured in table properties based on column names, but content stats needs to look up MetricsMode based on column ID. This adds support to look up metrics mode by ID, columnMode(int), and adds an accessor for the IDs for which there is a metrics mode. This is needed to build a table's current content stats struct for writing.

This also cleans up a couple of issues with MetricsConfig:

Removes the deprecated forPositionDelete(Table) instead of modifying its behavior. Also removes uses of this deprecated method, using forPositionDelete() instead.
Updates the deprecation of fromProperties to 1.13.0 instead of 2.0.0 because this is in core. Also removes uses of this deprecated method, which should be done when it is deprecated.
Adds validate(Map<String, String>,Schema) to replace uses of fromProperties

rdblue · 2026-06-30T18:03:58Z

  }
+
+  @Test
+  public void testColumnModeAndFieldIdsFromColumnConfig() {


These are the tests for new behavior.

rdblue · 2026-06-30T18:04:45Z

    }
  }
-
-  public void validateReferencedColumns(Schema schema) {


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

amogh-jahagirdar · 2026-07-01T00:21:27Z

-  public static MetricsConfig forPositionDelete(Table table) {
-    ImmutableMap.Builder<String, MetricsMode> columnModes = ImmutableMap.builder();
+  public Iterable<Integer> metricsFieldIds() {
+    return idToName.keySet();


Should this return an empty list if idToName is null?

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.

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.

amogh-jahagirdar · 2026-07-01T00:32:16Z

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


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?

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.

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

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

amogh-jahagirdar · 2026-07-01T00:50:52Z

-          table == null
-              ? MetricsConfig.forPositionDelete()
-              : MetricsConfig.forPositionDelete(table);
+      MetricsConfig metricsConfig = MetricsConfig.forPositionDelete();


Since we're just calling MetricsConfig.forPositionDelete(), now we're falling through to the default truncate(16) and changing what we collect bounds on (and it looks like Spark CI is failing). It is a behavior change for the position deletes where rows are stored but iirc nobody really even writes those and that was on a deprecation path anyways.

Agreed, I have a separate PR aimed to remove deprecated method for 1.12 which include the MetricsConfig.forPositionDelete(table) in #16449

I updated this to use None by default and fixed the tests so they are passing.

amogh-jahagirdar

Looks correct to me, we'll just need to fixup the wong preconditions import before we merge!

dramaticlly · 2026-07-01T20:18:02Z

+          schema);
+
+      ValidationException.check(
+          null == idToName || column.equals(idToName.get(field.fieldId())),


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

nastra · 2026-07-03T07:40:58Z

-    assertThat(bounds).hasSize(4);
-    assertThat((int) Conversions.fromByteBuffer(Types.IntegerType.get(), bounds.get(1)))
-        .isEqualTo(3);
+    assertThat(bounds).hasSize(2);


nit: we should be able to make a one-liner out of this entire check: assertThat(bounds).hasSize(2).doesNotContainKeys(1, 2, 3, 4, 5);

also maybe we can just inline this check into the test that uses it and remove checkRowStatistics(), since it's really only used in a single test

nastra · 2026-07-03T07:51:49Z

   */
-  public interface MetricsMode extends Serializable {}
+  public interface MetricsMode extends Serializable {
+    default boolean hasBounds() {


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

rdblue · 2026-07-07T19:36:06Z

@nastra, sorry I missed your comments, I should have reloaded. I just saw tests passing and merged.

The hasBounds method is for stats struct generation and it was part of my branch when I separated the metrics changes out.

Core: Add id tracking to MetricsConfig.

4becc0c

rdblue requested a review from amogh-jahagirdar June 30, 2026 18:00

github-actions Bot added spark core data flink labels Jun 30, 2026

Apply spotless.

173594a

rdblue force-pushed the update-metrics-config branch from 3a5cd12 to 173594a Compare June 30, 2026 18:03

rdblue commented Jun 30, 2026

View reviewed changes

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

rdblue added 5 commits June 30, 2026 11:28

Update older Spark versions.

42b26cc

Fixup last commit's typo.

8e1e7b2

Add default implementation of hasBounds().

ab334da

Update revapi to remove deprecated method.

bdcb363

Use longer error message to avoid changing another test file.

306effe

amogh-jahagirdar reviewed Jul 1, 2026

View reviewed changes

rdblue added 2 commits July 1, 2026 12:34

Fix Amogh's review comments.

1cea2e1

Fix Flink tests after changes to TestWriterMetrics.

ba1473c

amogh-jahagirdar approved these changes Jul 1, 2026

View reviewed changes

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

Fix import.

19f5313

dramaticlly approved these changes Jul 1, 2026

View reviewed changes

rdblue added 3 commits July 1, 2026 14:22

Fix more tests.

abb2417

Make field final.

a706936

Fix imports.

de8f061

nastra reviewed Jul 3, 2026

View reviewed changes

rdblue merged commit 773907d into apache:main Jul 7, 2026
55 checks passed

Kurtiscwright pushed a commit to Kurtiscwright/iceberg that referenced this pull request Jul 7, 2026

Core: Add id tracking to MetricsConfig (apache#17022)

9b197ec

Uh oh!

Conversation

rdblue commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

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!

amogh-jahagirdar left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

rdblue commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rdblue commented Jun 30, 2026 •

edited

Loading