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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
