You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dramaticlly (via GitHub)" <gi...@apache.org> on 2026/07/28 00:50:09 UTC
[PR] Core: Add nested field traversal in AppendBenchmark and FileGenerationUtil [iceberg]
dramaticlly opened a new pull request, #17391:
URL: https://github.com/apache/iceberg/pull/17391
Adding schema support for nested fields in AppendBenchmark and FileGenerationUtil. Previous it will only handle the primitive type but real production workload often involves complex type.
--
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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#discussion_r3676836550
##########
core/src/test/java/org/apache/iceberg/FileGenerationUtil.java:
##########
@@ -207,6 +211,44 @@ public static Metrics generateRandomMetrics(
originalTypes);
}
+ // collects leaf primitive fields, recursing through struct/list/map
+ static List<Types.NestedField> leafFields(Schema schema) {
Review Comment:
Thanks @nastra, updated per your suggestion.Still kept the package private leafFields method instead of inline so we can use in UT as well
--
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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#issuecomment-5260134451
Thanks @dramaticlly for the PR! Thanks everyone for the review!
--
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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#discussion_r3667456931
##########
core/src/test/java/org/apache/iceberg/FileGenerationUtil.java:
##########
@@ -207,6 +211,44 @@ public static Metrics generateRandomMetrics(
originalTypes);
}
+ // collects leaf primitive fields, recursing through struct/list/map
+ static List<Types.NestedField> leafFields(Schema schema) {
+ List<Types.NestedField> leaves = Lists.newArrayList();
+ TypeUtil.visit(
+ schema,
+ new TypeUtil.SchemaVisitor<Void>() {
Review Comment:
I think current behavior for leafFields will throw UnsupportedOperationException if running into a V3 table with variant type. To me, it seems reasonable as exception serve as a reminder to add variant support later. There's also change like #15629 to add dedicated benchmark for variant.
Prefer to keep the existing scope and revisit v3+ type later in follow up PR
##########
core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java:
##########
@@ -101,12 +115,32 @@ void testBoundsForAllMetricsModes(String metricsMode) {
ImmutableMap.of() /* no lower bounds */,
ImmutableMap.of() /* no upper bounds */);
- checkBounds(metrics, metricsConfig);
+ checkBounds(SCHEMA, metrics, metricsConfig);
+ }
+
+ @Test
+ public void testBoundsForNestedTypes() {
+ MetricsConfig metricsConfig = MetricsConfig.getDefault();
Review Comment:
Good call, parametrized on all metrics mode for better coverage
--
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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#issuecomment-5260117237
LGTM. Small nit: the Prompt Summary mentions a BinaryType truncate-mode crash fix, but the diff is purely nested-field support. Mind updating the description to match what shipped?
--
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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao merged PR #17391:
URL: https://github.com/apache/iceberg/pull/17391
--
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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#issuecomment-5170743800
Thanks @nastra , ready for another look
--
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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#discussion_r3664064168
##########
core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java:
##########
@@ -101,12 +115,32 @@ void testBoundsForAllMetricsModes(String metricsMode) {
ImmutableMap.of() /* no lower bounds */,
ImmutableMap.of() /* no upper bounds */);
- checkBounds(metrics, metricsConfig);
+ checkBounds(SCHEMA, metrics, metricsConfig);
+ }
+
+ @Test
+ public void testBoundsForNestedTypes() {
+ MetricsConfig metricsConfig = MetricsConfig.getDefault();
Review Comment:
Here we only use `MetricsConfig.getDefault()`, but the new key code path for a nested field is never exercised under non-default modes (none, counts, truncate).
--
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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#discussion_r3680985031
##########
core/src/test/java/org/apache/iceberg/TestFileGenerationUtil.java:
##########
@@ -101,12 +115,35 @@ void testBoundsForAllMetricsModes(String metricsMode) {
ImmutableMap.of() /* no lower bounds */,
ImmutableMap.of() /* no upper bounds */);
- checkBounds(metrics, metricsConfig);
+ checkBounds(SCHEMA, metrics, metricsConfig);
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {"none", "counts", "truncate(16)", "full"})
+ public void testBoundsForNestedTypes(String metricsMode) {
+ MetricsConfig metricsConfig =
+ MetricsConfig.from(
+ ImmutableMap.of(TableProperties.DEFAULT_WRITE_METRICS_MODE, metricsMode), SCHEMA, null);
Review Comment:
should this be using NESTED_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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#issuecomment-5107033273
@huaxingao @nastra if you want to take a look as well :)
--
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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#discussion_r3667456931
##########
core/src/test/java/org/apache/iceberg/FileGenerationUtil.java:
##########
@@ -207,6 +211,44 @@ public static Metrics generateRandomMetrics(
originalTypes);
}
+ // collects leaf primitive fields, recursing through struct/list/map
+ static List<Types.NestedField> leafFields(Schema schema) {
+ List<Types.NestedField> leaves = Lists.newArrayList();
+ TypeUtil.visit(
+ schema,
+ new TypeUtil.SchemaVisitor<Void>() {
Review Comment:
I think current behavior for leafFields will throw UnsupportedOperationException if running into a V3 table with variant type. To me, it seems reasonable as exception serve as a reminder to add variant support instead of silently missing metrics or bound. There's also change like #15629 to add dedicated benchmark for variant.
Prefer to keep the existing scope and revisit v3+ type later in follow up 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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#issuecomment-5258548652
@nastra Thanks for the review and approval. Do you think we can merge the 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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#discussion_r3664056559
##########
core/src/test/java/org/apache/iceberg/FileGenerationUtil.java:
##########
@@ -207,6 +211,44 @@ public static Metrics generateRandomMetrics(
originalTypes);
}
+ // collects leaf primitive fields, recursing through struct/list/map
+ static List<Types.NestedField> leafFields(Schema schema) {
+ List<Types.NestedField> leaves = Lists.newArrayList();
+ TypeUtil.visit(
+ schema,
+ new TypeUtil.SchemaVisitor<Void>() {
Review Comment:
We have struct / list / map here, but how about variant? With v3 adoption growing, a schema containing variant will throw rather than skip. Let's at least consider adding
```
@Override
public Void variant(Types.VariantType v) {
return 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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#discussion_r3668291709
##########
core/src/test/java/org/apache/iceberg/FileGenerationUtil.java:
##########
@@ -207,6 +211,44 @@ public static Metrics generateRandomMetrics(
originalTypes);
}
+ // collects leaf primitive fields, recursing through struct/list/map
+ static List<Types.NestedField> leafFields(Schema schema) {
+ List<Types.NestedField> leaves = Lists.newArrayList();
+ TypeUtil.visit(
+ schema,
+ new TypeUtil.SchemaVisitor<Void>() {
Review Comment:
there is a variand benchmark, we need reviews of 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: Support metrics with nested field in AppendBenchmark and FileGenerationUtil [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #17391:
URL: https://github.com/apache/iceberg/pull/17391#discussion_r3671294308
##########
core/src/test/java/org/apache/iceberg/FileGenerationUtil.java:
##########
@@ -207,6 +211,44 @@ public static Metrics generateRandomMetrics(
originalTypes);
}
+ // collects leaf primitive fields, recursing through struct/list/map
+ static List<Types.NestedField> leafFields(Schema schema) {
Review Comment:
I don't think we need this custom visitor. You should be able to achieve the same thing by using `TypeUtil.indexById(schema.asStruct())` and then just filtering on the field type
```
for (Types.NestedField field : TypeUtil.indexById(schema.asStruct()).values()) {
if (field.type().isPrimitiveType()) {
// ...
}
}
```
--
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