You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rdblue (via GitHub)" <gi...@apache.org> on 2026/06/30 18:00:29 UTC
[PR] Core: Add id tracking to MetricsConfig [iceberg]
rdblue opened a new pull request, #17022:
URL: https://github.com/apache/iceberg/pull/17022
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3507182720
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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();
+ }
- 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,
+ "Cannot 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(
Review Comment:
You're right. I'll change this to `column.equals`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3507179416
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3500901319
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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();
+ }
- 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,
+ "Cannot 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(
Review Comment:
This check was added to ensure that the id to name map is correct when it is supplied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#issuecomment-4907938712
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3518264453
##########
data/src/test/java/org/apache/iceberg/io/TestWriterMetrics.java:
##########
@@ -353,21 +353,11 @@ public void testMaxColumnsWithDefaultOverride() throws IOException {
}
protected void checkRowStatistics(Map<Integer, ByteBuffer> bounds) {
- assertThat(bounds).hasSize(4);
- assertThat((int) Conversions.fromByteBuffer(Types.IntegerType.get(), bounds.get(1)))
- .isEqualTo(3);
+ assertThat(bounds).hasSize(2);
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3508710680
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -49,27 +49,37 @@
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);
+ 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 Map<Integer, String> idToName;
Review Comment:
wondering if it worth to make idToName final as well?
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -100,32 +120,42 @@ 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() {
+ Preconditions.checkState(idToName != null, "Cannot resolve column mode by ID: missing schema");
+ return idToName.keySet();
+ }
- 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) {
+ Preconditions.checkState(idToName != null, "Cannot resolve column mode by ID: missing schema");
+ 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);
+ }
- return new MetricsConfig(columnModes.build(), 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(
+ null == idToName || column.equals(idToName.get(field.fieldId())),
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3518254410
##########
data/src/test/java/org/apache/iceberg/io/TestWriterMetrics.java:
##########
@@ -353,21 +353,11 @@ public void testMaxColumnsWithDefaultOverride() throws IOException {
}
protected void checkRowStatistics(Map<Integer, ByteBuffer> bounds) {
- assertThat(bounds).hasSize(4);
- assertThat((int) Conversions.fromByteBuffer(Types.IntegerType.get(), bounds.get(1)))
- .isEqualTo(3);
+ assertThat(bounds).hasSize(2);
Review Comment:
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);`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3508596015
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkFileWriterFactory.java:
##########
@@ -166,10 +166,7 @@ public PositionDeleteWriter<InternalRow> newPositionDeleteWriter(
} else {
LOG.warn("Position deletes with deleted rows are deprecated and will be removed in 1.12.0.");
Map<String, String> properties = table == null ? ImmutableMap.of() : table.properties();
- MetricsConfig metricsConfig =
- table == null
- ? MetricsConfig.forPositionDelete()
- : MetricsConfig.forPositionDelete(table);
+ MetricsConfig metricsConfig = MetricsConfig.forPositionDelete();
Review Comment:
I updated this to use `None` by default and fixed the tests so they are passing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #17022:
URL: https://github.com/apache/iceberg/pull/17022
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3518313212
##########
core/src/main/java/org/apache/iceberg/MetricsModes.java:
##########
@@ -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() {
Review Comment:
is this used anywhere? it's not clear from the PR description why this is being added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3508815548
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -30,9 +30,9 @@
import java.util.Set;
import java.util.function.Supplier;
import javax.annotation.concurrent.Immutable;
+import org.apache.curator.shaded.com.google.common.base.Preconditions;
Review Comment:
wrong preconditions import
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3500888077
##########
core/src/test/java/org/apache/iceberg/TestMetricsConfig.java:
##########
@@ -112,4 +114,95 @@ public void testNestedListOfMaps() {
assertThat(MetricsConfig.limitFieldIds(schema, 3)).isEqualTo(Set.of(5, 3, 4));
assertThat(MetricsConfig.limitFieldIds(schema, 4)).isEqualTo(Set.of(5, 3, 4));
}
+
+ @Test
+ public void testColumnModeAndFieldIdsFromColumnConfig() {
Review Comment:
These are the tests for new behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3507765749
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkFileWriterFactory.java:
##########
@@ -166,10 +166,7 @@ public PositionDeleteWriter<InternalRow> newPositionDeleteWriter(
} else {
LOG.warn("Position deletes with deleted rows are deprecated and will be removed in 1.12.0.");
Map<String, String> properties = table == null ? ImmutableMap.of() : table.properties();
- MetricsConfig metricsConfig =
- table == null
- ? MetricsConfig.forPositionDelete()
- : MetricsConfig.forPositionDelete(table);
+ MetricsConfig metricsConfig = MetricsConfig.forPositionDelete();
Review Comment:
Agreed, I have a separate PR aimed to remove deprecated method for 1.12 which include the `MetricsConfig.forPositionDelete(table)` in https://github.com/apache/iceberg/pull/16449
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3508474922
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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;
- 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) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3508488534
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3508525175
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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;
- 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) {
Review Comment:
Got it, okay I missed the original fromProperties, yeah in that case we can just keep it as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3508598229
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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;
- 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) {
Review Comment:
I added assertions to ensure we don't get NPEs. We can remove them once `idToName` is always present.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3500893045
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -309,19 +357,4 @@ private static MetricsMode parseMode(String modeString, MetricsMode fallback, St
return fallback;
}
}
-
- public void validateReferencedColumns(Schema schema) {
Review Comment:
These were moved above the `private` and `static` methods. It was hard to find them before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Add id tracking to MetricsConfig [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17022:
URL: https://github.com/apache/iceberg/pull/17022#discussion_r3502571104
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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;
- 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) {
Review Comment:
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?
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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();
Review Comment:
Should this return an empty list if idToName is null?
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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);
Review Comment:
Ah okay, this is the case where it's <= maxInferredDefaultColumns, it doesn't matter, we're taking all those ids.
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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);
Review Comment:
are we changing the behavior of which IDs are selected? previously looks like limitFieldIDs would go in struct field order, and now we're going in the set order which is undefined past some point I think.
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -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();
+ }
- 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,
+ "Cannot 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(
Review Comment:
It looks like this check throw an NPE if the field doesn't exist in idToName?
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkFileWriterFactory.java:
##########
@@ -166,10 +166,7 @@ public PositionDeleteWriter<InternalRow> newPositionDeleteWriter(
} else {
LOG.warn("Position deletes with deleted rows are deprecated and will be removed in 1.12.0.");
Map<String, String> properties = table == null ? ImmutableMap.of() : table.properties();
- MetricsConfig metricsConfig =
- table == null
- ? MetricsConfig.forPositionDelete()
- : MetricsConfig.forPositionDelete(table);
+ MetricsConfig metricsConfig = MetricsConfig.forPositionDelete();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org