You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2026/09/04 22:13:11 UTC
[PR] Core: Make MetricsConfig.forTable aware of the v4 partition spec [iceberg]
amogh-jahagirdar opened a new pull request, #17960:
URL: https://github.com/apache/iceberg/pull/17960
For v4 tables, collect untruncated metrics on the source column of any order-preserving partition transform, so bounds are usable to derive a partition value from stats instead of storing it explicitly. This is one of the primitives needed to eventually drop the v4 partition tuple. Note that we are not yet collecting information on bucket outputs.
Currently, all file format writers explicitly call MetricsConfig.forTable to build MetricsConfig instances. This change just adds the logic to the implementation of that API to require collecting full/untruncated stats for source columns of partition fields for V4 tables. If a user specified property says to not collect those, that property is ignored as it is ultimately neccessary to have these stats for the goal of dropping the partition tuple write requirement in V4.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4134862425
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/source/TestWriteMetricsConfig.java:
##########
@@ -282,13 +283,15 @@ public void testCustomMetricCollectionForNestedParquet() {
Map<Integer, Long> nullValueCounts = file.nullValueCounts();
assertThat(nullValueCounts)
- .hasSize(3)
- .containsKeys(longCol.fieldId(), recordId.fieldId(), recordData.fieldId());
+ .hasSize(4)
Review Comment:
These tests had to change because now we're just collecting the full stats for partition source columns regardless of format version. In this case we're identity partitioned by strCol
--
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: Make MetricsConfig.forTable aware of the v4 partition spec [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r3938237100
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -315,6 +321,21 @@ public static MetricsConfig from(Map<String, String> props, Schema schema, SortO
}
}
+ // configure metrics collection for partition fields ignoring any overrides
+ if (spec != null && schema != null) {
+ for (PartitionField field : spec.fields()) {
+ if (!field.transform().preservesOrder()) {
+ continue;
+ }
+
+ String name = schema.findColumnName(field.sourceId());
+ if (name != null) {
+ columnModes.put(name, MetricsModes.Full.get());
Review Comment:
Parquet technically allows for a configurable truncation for an entire file (not per column) but it defaults to MAX https://github.com/apache/parquet-java/blob/master/parquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java#L60. Since there's no API for finer grained guarantees from Parquet , we practically rely on the default from the parquet java library.
What we can do separately is add a unit test which just writes a big string/binary and in case the Parquet library changes out from under us, tests would fail and we would know. I didn't want to pollute this PR with that.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4138114257
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -286,6 +291,9 @@ public static MetricsConfig from(Map<String, String> props, Schema schema, SortO
// Override automatic modes with configured modes
columnModes.putAll(configuredColumnModes(props));
+ // Force full metrics for partition source columns
+ columnModes.putAll(partitionColumnModes(spec, schema));
Review Comment:
I agree with this. If we want to recover partition information, then we cannot allow user config to override this. The content stats for these fields becomes mandatory.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r3938498682
##########
kafka-connect/kafka-connect/src/test/java/org/apache/iceberg/connect/data/WriterTestBase.java:
##########
@@ -64,14 +68,20 @@ public class WriterTestBase {
public void before() {
fileIO = new InMemoryFileIO();
- table = mock(Table.class);
+ table = mock(Table.class, withSettings().extraInterfaces(HasTableOperations.class));
Review Comment:
Flagging why I'm touching the kafka-connect module here:
1. MetricsConfig.forTable(table) now calls TableUtil.formatVersion(table), which only resolves for a Table that's a SerializableTable, HasTableOperations, or BaseMetadataTable. Anything else throws.
2. I think that's a reasonable requirement. MetricsConfig tells writers what to collect, and if you're writing, you're going to commit, which needs TableOperations anyway. I didn't add a Preconditions check for it since the formatVersion exception already covers it if this assumption ever breaks.
WriterTestBase mocked a Table with no operations backing it and never had to satisfy this before, so I updated it to implement HasTableOperations with a stubbed format version.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4139286096
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
+ }
+
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ for (PartitionField field : spec.fields()) {
+ if (field.transform().preservesOrder()) {
+ String name = schema.findColumnName(field.sourceId());
+ Preconditions.checkArgument(
+ name != null, "Cannot find source column for partition field: %s", field.name());
+ builder.put(name, MetricsModes.Full.get());
+ }
+ }
+
+ return builder.buildKeepingLast();
Review Comment:
Added the test case, while it's possible I don't think it's really realistic (but of course we still need to correctly handle it). The implementation prevents having multiple time based transform on the same source columns, so I stuck with identity and truncate on the same source column for the purpose of the 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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4134327424
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
+ }
+
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ for (PartitionField field : spec.fields()) {
+ if (field.transform().preservesOrder()) {
+ String name = schema.findColumnName(field.sourceId());
+ if (name != null) {
Review Comment:
In this context, skipping when it's null doesn't make sense to me.
This would mean we're trying to build a MetricsConfig with a schema and a spec, where the spec references a source ID that doesn't actually exist in the schema. If we're trying to require collecting stats, then we wouldn't be able to build a valid MetricsConfig.
So, I think we should fail here if findColumnName returns null.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4138130526
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
Review Comment:
If spec is non-null, then schema cannot be null right? Otherwise, this could skip adding columns for a spec because the schema wasn't passed in as well.
Rather than failing though, it may be easier to fall back to `spec.schema()` like the sort order code uses.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4139416399
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
+ }
+
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ for (PartitionField field : spec.fields()) {
+ if (field.transform().preservesOrder()) {
+ String name = schema.findColumnName(field.sourceId());
+ Preconditions.checkArgument(
+ name != null, "Cannot find source column for partition field: %s", field.name());
+ builder.put(name, MetricsModes.Full.get());
+ }
+ }
+
+ return builder.buildKeepingLast();
Review Comment:
There are actually cases where this happens. If you have a day-partitioned table and add an hour partition, we allow that because people using metadata tables may have filters on the day partition. In that case, you'd have a partition spec with multiple fields that share a source field.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4139229874
##########
core/src/test/java/org/apache/iceberg/TestMetricsConfig.java:
##########
@@ -22,14 +22,80 @@
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
+import java.io.File;
+import java.io.IOException;
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
public class TestMetricsConfig {
+ @TempDir private File temp;
+
+ private static final int ID = 1;
+ private static final int EVENT_TIME = 2;
+ private static final int CATEGORY = 3;
+ private static final int DATA = 4;
+
+ private static final Schema PARTITIONED_SCHEMA =
+ new Schema(
+ required(ID, "id", Types.IntegerType.get()),
+ optional(EVENT_TIME, "event_time", Types.TimestampType.withoutZone()),
+ optional(CATEGORY, "category", Types.StringType.get()),
+ optional(DATA, "data", Types.StringType.get()));
+
+ @Test
+ public void testConfigCannotDisablePartitionSourceMetrics() throws IOException {
+ PartitionSpec spec = PartitionSpec.builderFor(PARTITIONED_SCHEMA).identity("category").build();
+ Map<String, String> props =
+ ImmutableMap.of(TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX + "category", "none");
+ Table table = TestTables.create(temp, "identity-override", PARTITIONED_SCHEMA, spec, 4, props);
+
+ assertThat(MetricsConfig.forTable(table).columnMode(CATEGORY))
+ .as("column config should not be able to disable metrics for a partition source column")
+ .isEqualTo(MetricsModes.Full.get());
+ }
+
+ @Test
+ public void testBucketPartitionColumnIgnored() throws IOException {
+ PartitionSpec spec = PartitionSpec.builderFor(PARTITIONED_SCHEMA).bucket("category", 4).build();
+ Table table = TestTables.create(temp, "bucket", PARTITIONED_SCHEMA, spec, 4, ImmutableMap.of());
+
+ assertThat(MetricsConfig.forTable(table).columnMode(CATEGORY))
+ .as("non-order-preserving partition transform should not promote its source column")
+ .isEqualTo(MetricsModes.Truncate.withLength(16));
+ }
+
+ @Test
+ public void testColumnModeAndFieldIdsFromPartitionSpec() throws IOException {
Review Comment:
Done. Let me know if the 2 partition fields for the same source case is as realistic as possible. I do think fundamentally that use case isn't realistic though it is allowed (and we can't have multiple time transforms on the same source columns at least from this implementation). So the test that was added was an identity + truncate on the same source column.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4139218536
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
+ }
+
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ for (PartitionField field : spec.fields()) {
+ if (field.transform().preservesOrder()) {
+ String name = schema.findColumnName(field.sourceId());
+ Preconditions.checkArgument(
+ name != null, "Cannot find source column for partition field: %s", field.name());
+ builder.put(name, MetricsModes.Full.get());
Review Comment:
Yeah, I thought about this and just kept it as full for simplicity. But yes as an improvement we can allow for stats truncation to at least W as long as partition values are taken from lower bounds. but that's just more complexity at this point. I added a comment explaining why we're using full and suggesting the improvement we could do in the future.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4138241559
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
+ }
+
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ for (PartitionField field : spec.fields()) {
+ if (field.transform().preservesOrder()) {
+ String name = schema.findColumnName(field.sourceId());
+ Preconditions.checkArgument(
+ name != null, "Cannot find source column for partition field: %s", field.name());
+ builder.put(name, MetricsModes.Full.get());
Review Comment:
I think there are only two cases we need to worry about with variable-length data:
- `identity` always requires full because values cannot be truncated
- `truncate[W]` could use `truncate(>W)` because the lower bound is always a simple truncation
Truncate is a little tricky because truncation for upper bounds can be a shorter string, or one that is the right length but doesn't match the lower bound's prefix. We should make sure we only use the lower bound to produce partition values.
Always using `Full` is the safest call, but I think this could also allow stats truncation. Probably good to add a comment explaining the use of full for now.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4139407446
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -286,6 +291,9 @@ public static MetricsConfig from(Map<String, String> props, Schema schema, SortO
// Override automatic modes with configured modes
columnModes.putAll(configuredColumnModes(props));
+ // Force full metrics for partition source columns
+ columnModes.putAll(partitionColumnModes(spec, schema));
Review Comment:
I'm unresolving this so it's more obvious if anyone looks at the PR.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r3938498682
##########
kafka-connect/kafka-connect/src/test/java/org/apache/iceberg/connect/data/WriterTestBase.java:
##########
@@ -64,14 +68,20 @@ public class WriterTestBase {
public void before() {
fileIO = new InMemoryFileIO();
- table = mock(Table.class);
+ table = mock(Table.class, withSettings().extraInterfaces(HasTableOperations.class));
Review Comment:
Flagging why I'm touching the kafka-connect module tests here:
1. MetricsConfig.forTable(table) now calls TableUtil.formatVersion(table), which only resolves for a Table that's a SerializableTable, HasTableOperations, or BaseMetadataTable. Anything else throws.
2. I think that's a reasonable requirement. MetricsConfig tells writers what to collect, and if you're writing, you're going to commit, which needs TableOperations anyway. I didn't add a Preconditions check for it since the formatVersion exception already covers it if this assumption ever breaks but we can add one. I can't imagine a MetricsConfig being more generalizable than this use case?
WriterTestBase mocked a Table with no operations backing it and never had to satisfy this before, so I updated it to implement HasTableOperations with a stubbed format version.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4108647535
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -118,7 +118,12 @@ public static void validate(Map<String, String> props, Schema schema) {
* @return a metrics config for the given table
*/
public static MetricsConfig forTable(Table table) {
Review Comment:
Yes, this is the correct method. The only calls to `from` are from this class and test classes where we don't have a `Table` instance. We should probably switch those over to mocks and deprecate the `from` method at some point.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r3938237100
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -315,6 +321,21 @@ public static MetricsConfig from(Map<String, String> props, Schema schema, SortO
}
}
+ // configure metrics collection for partition fields ignoring any overrides
+ if (spec != null && schema != null) {
+ for (PartitionField field : spec.fields()) {
+ if (!field.transform().preservesOrder()) {
+ continue;
+ }
+
+ String name = schema.findColumnName(field.sourceId());
+ if (name != null) {
+ columnModes.put(name, MetricsModes.Full.get());
Review Comment:
Parquet technically allows for a configurable truncation for an entire file (not per column) but it defaults to Int max so we're OK for now. https://github.com/apache/parquet-java/blob/master/parquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java#L60. Since there's no API for finer grained guarantees from Parquet , we practically rely on the default from the parquet java library.
What we can do separately is add a unit test which just writes a big string/binary and in case the Parquet library changes out from under us, tests would fail and we would know. I didn't want to pollute this PR with that.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r3938409086
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -118,7 +118,12 @@ public static void validate(Map<String, String> props, Schema schema) {
* @return a metrics config for the given table
*/
public static MetricsConfig forTable(Table table) {
Review Comment:
This is the only called method for passing through to writers as far as I can tell. Technically writers can always choose to write with any partition spec, but I don't see it in the code base so I didn't bother adding a `from(props, schema, partitionSpec, SortOrder)` 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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #17960:
URL: https://github.com/apache/iceberg/pull/17960
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4138274551
##########
core/src/test/java/org/apache/iceberg/TestMetricsConfig.java:
##########
@@ -22,14 +22,80 @@
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
+import java.io.File;
+import java.io.IOException;
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
public class TestMetricsConfig {
+ @TempDir private File temp;
+
+ private static final int ID = 1;
+ private static final int EVENT_TIME = 2;
+ private static final int CATEGORY = 3;
+ private static final int DATA = 4;
+
+ private static final Schema PARTITIONED_SCHEMA =
+ new Schema(
+ required(ID, "id", Types.IntegerType.get()),
+ optional(EVENT_TIME, "event_time", Types.TimestampType.withoutZone()),
+ optional(CATEGORY, "category", Types.StringType.get()),
+ optional(DATA, "data", Types.StringType.get()));
+
+ @Test
+ public void testConfigCannotDisablePartitionSourceMetrics() throws IOException {
+ PartitionSpec spec = PartitionSpec.builderFor(PARTITIONED_SCHEMA).identity("category").build();
+ Map<String, String> props =
+ ImmutableMap.of(TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX + "category", "none");
+ Table table = TestTables.create(temp, "identity-override", PARTITIONED_SCHEMA, spec, 4, props);
+
+ assertThat(MetricsConfig.forTable(table).columnMode(CATEGORY))
+ .as("column config should not be able to disable metrics for a partition source column")
+ .isEqualTo(MetricsModes.Full.get());
+ }
+
+ @Test
+ public void testBucketPartitionColumnIgnored() throws IOException {
+ PartitionSpec spec = PartitionSpec.builderFor(PARTITIONED_SCHEMA).bucket("category", 4).build();
+ Table table = TestTables.create(temp, "bucket", PARTITIONED_SCHEMA, spec, 4, ImmutableMap.of());
+
+ assertThat(MetricsConfig.forTable(table).columnMode(CATEGORY))
+ .as("non-order-preserving partition transform should not promote its source column")
+ .isEqualTo(MetricsModes.Truncate.withLength(16));
+ }
+
+ @Test
+ public void testColumnModeAndFieldIdsFromPartitionSpec() throws IOException {
Review Comment:
Could you add a test for `truncate` and one where there are two partition fields for a single source column?
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4134998375
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -255,9 +260,36 @@ public Set<Integer> map(
* @return metrics configuration
*/
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();
+ MetricsMode defaultMode = defaultModeFor(props, schema, order, columnModes, idToName);
+ return new MetricsConfig(columnModes, defaultMode, idToName);
+ }
+
+ private static MetricsConfig from(
+ Map<String, String> props,
+ Schema schema,
+ SortOrder order,
+ PartitionSpec spec,
+ int formatVersion) {
+ Map<Integer, String> idToName = Maps.newHashMap();
+ Map<String, MetricsMode> columnModes = Maps.newHashMap();
+ MetricsMode defaultMode = defaultModeFor(props, schema, order, columnModes, idToName);
+
+ if (formatVersion >= 4) {
+ configureFullMetricsForPartitionSourceColumns(spec, schema, columnModes, idToName);
Review Comment:
That's a great point, that approach makes the upgrde path be able to make better tightness assumptions. I went ahead and made that change.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4108663485
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -255,9 +260,36 @@ public Set<Integer> map(
* @return metrics configuration
*/
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();
+ MetricsMode defaultMode = defaultModeFor(props, schema, order, columnModes, idToName);
+ return new MetricsConfig(columnModes, defaultMode, idToName);
+ }
+
+ private static MetricsConfig from(
+ Map<String, String> props,
+ Schema schema,
+ SortOrder order,
+ PartitionSpec spec,
+ int formatVersion) {
+ Map<Integer, String> idToName = Maps.newHashMap();
+ Map<String, MetricsMode> columnModes = Maps.newHashMap();
+ MetricsMode defaultMode = defaultModeFor(props, schema, order, columnModes, idToName);
+
+ if (formatVersion >= 4) {
+ configureFullMetricsForPartitionSourceColumns(spec, schema, columnModes, idToName);
Review Comment:
I think that we want to do this for all format versions. When you upgrade to v4, it's better to have tighter metrics than what we can infer from the partition values. And the number of columns affected by this would be very small. I think we can just make this change without worrying about anyone having issues.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4134327424
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
+ }
+
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ for (PartitionField field : spec.fields()) {
+ if (field.transform().preservesOrder()) {
+ String name = schema.findColumnName(field.sourceId());
+ if (name != null) {
Review Comment:
In this context, skipping when it's null doesn't make sense to me.
This would mean we're trying to build a MetricsConfig with a schema and a spec, where the spec references a source ID that doesn't actually exist in the schema. If we're trying to require collecting stats, then we wouldn't be able to build a valid MetricsConfig.
I think it's only a possibility with the deprecated APIs where a caller can just pass in whatever spec and schema they want, but on a given Table instance it's not possible for the current spec/schema to be incompatible.
So, I think we should fail here if findColumnName returns null.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4134327424
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
+ }
+
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ for (PartitionField field : spec.fields()) {
+ if (field.transform().preservesOrder()) {
+ String name = schema.findColumnName(field.sourceId());
+ if (name != null) {
Review Comment:
In this context, skipping when it's null doesn't make sense to me.
This would mean we're trying to build a MetricsConfig with a schema and a spec, where the spec references a source ID that doesn't actually exist in the schema. If we're trying to require collecting stats, then we wouldn't be able to build a valid MetricsConfig.
This generally shouldn't happen, and with only the fromTable API (when the deprecated paths are removed), where it's the "current" schema and spec, it certainly shouldn't happen.
So, I think we should fail here if findColumnName returns null.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4134911536
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/source/TestWriteMetricsConfig.java:
##########
@@ -282,13 +283,15 @@ public void testCustomMetricCollectionForNestedParquet() {
Map<Integer, Long> nullValueCounts = file.nullValueCounts();
assertThat(nullValueCounts)
- .hasSize(3)
- .containsKeys(longCol.fieldId(), recordId.fieldId(), recordData.fieldId());
+ .hasSize(4)
Review Comment:
what's interesting/a bit suspect is that the lower/upper bounds assertions didn't need to change. I think this is a separate issue though, the metrics config we're building is correct, and clearly the column is materialized in parquet, but for whatever reason the parquet writer is only handing back the null/value counts and not the lower/upper bounds.
I'm looking at this separately though, maybe another plumbing issue in the writer or something i'm missing as to why parquet is handing back metrics which omit lower/upper bounds for this string column.
I also don't really know if I agree with the utility of this test class, but while it's here I'll just stick to updating it.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4139188614
##########
core/src/test/java/org/apache/iceberg/TestMetricsConfig.java:
##########
@@ -22,14 +22,80 @@
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
+import java.io.File;
+import java.io.IOException;
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
public class TestMetricsConfig {
+ @TempDir private File temp;
+
+ private static final int ID = 1;
+ private static final int EVENT_TIME = 2;
+ private static final int CATEGORY = 3;
+ private static final int DATA = 4;
+
+ private static final Schema PARTITIONED_SCHEMA =
+ new Schema(
+ required(ID, "id", Types.IntegerType.get()),
+ optional(EVENT_TIME, "event_time", Types.TimestampType.withoutZone()),
+ optional(CATEGORY, "category", Types.StringType.get()),
+ optional(DATA, "data", Types.StringType.get()));
+
+ @Test
+ public void testConfigCannotDisablePartitionSourceMetrics() throws IOException {
+ PartitionSpec spec = PartitionSpec.builderFor(PARTITIONED_SCHEMA).identity("category").build();
+ Map<String, String> props =
+ ImmutableMap.of(TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX + "category", "none");
+ Table table = TestTables.create(temp, "identity-override", PARTITIONED_SCHEMA, spec, 4, props);
Review Comment:
Ah yeah, done!
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r3938498682
##########
kafka-connect/kafka-connect/src/test/java/org/apache/iceberg/connect/data/WriterTestBase.java:
##########
@@ -64,14 +68,20 @@ public class WriterTestBase {
public void before() {
fileIO = new InMemoryFileIO();
- table = mock(Table.class);
+ table = mock(Table.class, withSettings().extraInterfaces(HasTableOperations.class));
Review Comment:
Flagging why I'm touching the kafka-connect module here:
1. MetricsConfig.forTable(table) now calls TableUtil.formatVersion(table), which only resolves for a Table that's a SerializableTable, HasTableOperations, or BaseMetadataTable. Anything else throws.
2. I think that's a reasonable requirement. MetricsConfig tells writers what to collect, and if you're writing, you're going to commit, which needs TableOperations anyway. I didn't add a Preconditions check for it since the formatVersion exception already covers it if this assumption ever breaks but we can add one. I can't imagine a MetricsConfig being more generalizable than this use case?
WriterTestBase mocked a Table with no operations backing it and never had to satisfy this before, so I updated it to implement HasTableOperations with a stubbed format version.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4134911536
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/source/TestWriteMetricsConfig.java:
##########
@@ -282,13 +283,15 @@ public void testCustomMetricCollectionForNestedParquet() {
Map<Integer, Long> nullValueCounts = file.nullValueCounts();
assertThat(nullValueCounts)
- .hasSize(3)
- .containsKeys(longCol.fieldId(), recordId.fieldId(), recordData.fieldId());
+ .hasSize(4)
Review Comment:
what's interesting/a bit suspect is that the lower/upper bounds assertions didn't need to change. I think this is a separate issue though, the metrics config we're building is correct, and clearly the column is materialized in parquet, but for whatever reason the parquet writer is only handing back the null/value counts and not the lower/upper bounds.
I'm looking at this separately though, maybe another plumbing issue in the writer.
I also don't really know if I agree with the utility of this test class, but while it's here I'll just stick to updating it.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4135092051
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/source/TestWriteMetricsConfig.java:
##########
@@ -282,13 +283,15 @@ public void testCustomMetricCollectionForNestedParquet() {
Map<Integer, Long> nullValueCounts = file.nullValueCounts();
assertThat(nullValueCounts)
- .hasSize(3)
- .containsKeys(longCol.fieldId(), recordId.fieldId(), recordData.fieldId());
+ .hasSize(4)
Review Comment:
Ah okay it's just the test itself. we're producing multiple files, one of which is a complete null partitioned file. For that file lower/upper is actually just going to be 3. but the test will still fail (due to task planning not being deterministic) for the other file.
Taking a step back, this test shouldn't care about partitioning (it's just testing nested field metrics). So we can just use an unpartitioned spec here.
I think it's another example for why this test class probably isn't super useful.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4138244897
##########
core/src/test/java/org/apache/iceberg/TestMetricsConfig.java:
##########
@@ -22,14 +22,80 @@
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
+import java.io.File;
+import java.io.IOException;
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
public class TestMetricsConfig {
+ @TempDir private File temp;
+
+ private static final int ID = 1;
+ private static final int EVENT_TIME = 2;
+ private static final int CATEGORY = 3;
+ private static final int DATA = 4;
+
+ private static final Schema PARTITIONED_SCHEMA =
Review Comment:
Is the schema partitioned?
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4138142652
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
+ }
+
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ for (PartitionField field : spec.fields()) {
+ if (field.transform().preservesOrder()) {
+ String name = schema.findColumnName(field.sourceId());
+ Preconditions.checkArgument(
+ name != null, "Cannot find source column for partition field: %s", field.name());
+ builder.put(name, MetricsModes.Full.get());
+ }
+ }
+
+ return builder.buildKeepingLast();
Review Comment:
Would be nice to comment that multiple fields can have the same source ID, so `buildKeepingLast` is needed.
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
+ }
+
+ ImmutableMap.Builder<String, MetricsMode> builder = ImmutableMap.builder();
+ for (PartitionField field : spec.fields()) {
+ if (field.transform().preservesOrder()) {
+ String name = schema.findColumnName(field.sourceId());
+ Preconditions.checkArgument(
+ name != null, "Cannot find source column for partition field: %s", field.name());
+ builder.put(name, MetricsModes.Full.get());
+ }
+ }
+
+ return builder.buildKeepingLast();
Review Comment:
Would be nice to comment that multiple fields can have the same source ID, so `buildKeepingLast` is needed. This is also a good test case!
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4139420925
##########
core/src/test/java/org/apache/iceberg/TestMetricsConfig.java:
##########
@@ -22,14 +22,80 @@
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
+import java.io.File;
+import java.io.IOException;
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
public class TestMetricsConfig {
+ @TempDir private File temp;
+
+ private static final int ID = 1;
+ private static final int EVENT_TIME = 2;
+ private static final int CATEGORY = 3;
+ private static final int DATA = 4;
+
+ private static final Schema PARTITIONED_SCHEMA =
+ new Schema(
+ required(ID, "id", Types.IntegerType.get()),
+ optional(EVENT_TIME, "event_time", Types.TimestampType.withoutZone()),
+ optional(CATEGORY, "category", Types.StringType.get()),
+ optional(DATA, "data", Types.StringType.get()));
+
+ @Test
+ public void testConfigCannotDisablePartitionSourceMetrics() throws IOException {
+ PartitionSpec spec = PartitionSpec.builderFor(PARTITIONED_SCHEMA).identity("category").build();
+ Map<String, String> props =
+ ImmutableMap.of(TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX + "category", "none");
+ Table table = TestTables.create(temp, "identity-override", PARTITIONED_SCHEMA, spec, 4, props);
+
+ assertThat(MetricsConfig.forTable(table).columnMode(CATEGORY))
+ .as("column config should not be able to disable metrics for a partition source column")
+ .isEqualTo(MetricsModes.Full.get());
+ }
+
+ @Test
+ public void testBucketPartitionColumnIgnored() throws IOException {
+ PartitionSpec spec = PartitionSpec.builderFor(PARTITIONED_SCHEMA).bucket("category", 4).build();
+ Table table = TestTables.create(temp, "bucket", PARTITIONED_SCHEMA, spec, 4, ImmutableMap.of());
+
+ assertThat(MetricsConfig.forTable(table).columnMode(CATEGORY))
+ .as("non-order-preserving partition transform should not promote its source column")
+ .isEqualTo(MetricsModes.Truncate.withLength(16));
+ }
+
+ @Test
+ public void testColumnModeAndFieldIdsFromPartitionSpec() throws IOException {
Review Comment:
That sounds fine.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r3938498682
##########
kafka-connect/kafka-connect/src/test/java/org/apache/iceberg/connect/data/WriterTestBase.java:
##########
@@ -64,14 +68,20 @@ public class WriterTestBase {
public void before() {
fileIO = new InMemoryFileIO();
- table = mock(Table.class);
+ table = mock(Table.class, withSettings().extraInterfaces(HasTableOperations.class));
Review Comment:
Flagging why I'm touching the kafka-connect module tests here:
1. MetricsConfig.forTable(table) now calls TableUtil.formatVersion(table), which only resolves for a Table that's a SerializableTable, HasTableOperations, or BaseMetadataTable. Anything else throws.
2. I think that's a reasonable requirement. MetricsConfig tells writers what to collect, and if you're writing, you're going to commit, which needs TableOperations anyway. I didn't add a Preconditions check for it since the formatVersion exception already covers it if this assumption ever breaks but we can add one. I can't imagine a MetricsConfig being more generalizable than this use case?
WriterTestBase mocked a Table with no operations backing it and never had to satisfy this before, so I updated it to implement HasTableOperations with a stubbed format version. I think it's also reasonable to update this test because this test is touching the writer path.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4135092051
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/source/TestWriteMetricsConfig.java:
##########
@@ -282,13 +283,15 @@ public void testCustomMetricCollectionForNestedParquet() {
Map<Integer, Long> nullValueCounts = file.nullValueCounts();
assertThat(nullValueCounts)
- .hasSize(3)
- .containsKeys(longCol.fieldId(), recordId.fieldId(), recordData.fieldId());
+ .hasSize(4)
Review Comment:
Ah okay it's just the test itself. we're producing multiple files, one of which is a complete null partitioned file. For that file lower/upper is never going to be produced.
but the test will still fail (due to task planning not being deterministic) for the other file.
Taking a step back, this test shouldn't care about partitioning (it's just testing nested field metrics). So we can just use an unpartitioned spec here.
I think it's another example for why this test class probably isn't super useful.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4139269287
##########
core/src/main/java/org/apache/iceberg/MetricsConfig.java:
##########
@@ -338,6 +346,24 @@ private static Map<String, MetricsMode> configuredColumnModes(Map<String, String
return builder.build();
}
+ private static Map<String, MetricsMode> partitionColumnModes(PartitionSpec spec, Schema schema) {
+ if (spec == null || schema == null) {
+ return ImmutableMap.of();
Review Comment:
Yeah that's true. The only path for passing the spec is the for table API , meaninig schema also cannot be null. I've tightened this up so that we just use spec.schema() and also this simplifies the field resolution by using the bound spec schema as a source of truth. And we know it has to be bound because the only path is through the api that passes in the table.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4139186292
##########
core/src/test/java/org/apache/iceberg/TestMetricsConfig.java:
##########
@@ -22,14 +22,80 @@
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
+import java.io.File;
+import java.io.IOException;
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
public class TestMetricsConfig {
+ @TempDir private File temp;
+
+ private static final int ID = 1;
+ private static final int EVENT_TIME = 2;
+ private static final int CATEGORY = 3;
+ private static final int DATA = 4;
+
+ private static final Schema PARTITIONED_SCHEMA =
Review Comment:
Ha no, was meant to be schema for the partitioned test cases, but we can just call it `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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields for V4 tables [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4134911536
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/source/TestWriteMetricsConfig.java:
##########
@@ -282,13 +283,15 @@ public void testCustomMetricCollectionForNestedParquet() {
Map<Integer, Long> nullValueCounts = file.nullValueCounts();
assertThat(nullValueCounts)
- .hasSize(3)
- .containsKeys(longCol.fieldId(), recordId.fieldId(), recordData.fieldId());
+ .hasSize(4)
Review Comment:
what's interesting/a bit suspect is that the lower/upper bounds assertions didn't need to change. I think this is a separate issue though, the metrics config we're building is correct, and clearly the column is materialized in parquet, but for whatever reason the parquet writer is only handing back the null/value counts and not the lower/upper bounds.
I'm looking at this separately though. I also don't really know if aI agree with the utility of this test class, but while it's here I'll just stick to updating it.
--
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: Make MetricsConfig.forTable collect full metrics for order preserving transform source fields [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17960:
URL: https://github.com/apache/iceberg/pull/17960#discussion_r4138254028
##########
core/src/test/java/org/apache/iceberg/TestMetricsConfig.java:
##########
@@ -22,14 +22,80 @@
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
+import java.io.File;
+import java.io.IOException;
import java.util.Map;
import java.util.Set;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
public class TestMetricsConfig {
+ @TempDir private File temp;
+
+ private static final int ID = 1;
+ private static final int EVENT_TIME = 2;
+ private static final int CATEGORY = 3;
+ private static final int DATA = 4;
+
+ private static final Schema PARTITIONED_SCHEMA =
+ new Schema(
+ required(ID, "id", Types.IntegerType.get()),
+ optional(EVENT_TIME, "event_time", Types.TimestampType.withoutZone()),
+ optional(CATEGORY, "category", Types.StringType.get()),
+ optional(DATA, "data", Types.StringType.get()));
+
+ @Test
+ public void testConfigCannotDisablePartitionSourceMetrics() throws IOException {
+ PartitionSpec spec = PartitionSpec.builderFor(PARTITIONED_SCHEMA).identity("category").build();
+ Map<String, String> props =
+ ImmutableMap.of(TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX + "category", "none");
+ Table table = TestTables.create(temp, "identity-override", PARTITIONED_SCHEMA, spec, 4, props);
Review Comment:
Could you use `MetricsTestUtil` rather than `TestTables`? That avoids needing to write anything to disk or have a temp file.
--
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