You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "tmater (via GitHub)" <gi...@apache.org> on 2025/11/18 16:49:04 UTC
[PR] Fix NameMapping loss in ParquetUtil.footerMetrics [iceberg]
tmater opened a new pull request, #14617:
URL: https://github.com/apache/iceberg/pull/14617
# Fix NameMapping loss in ParquetUtil.footerMetrics
## Summary
Fixed a bug where `ParquetUtil.footerMetrics` was losing field IDs when using NameMapping, resulting in empty metrics for Parquet files without embedded field IDs.
## Background
When `footerMetrics` is called with a NameMapping, it applies the mapping to get field IDs via `getParquetTypeWithIds()`, but then passed the original MessageType to `ParquetMetrics.metrics`. Later in the `metrics()` call, field IDs are extracted from the MessageType via `type.getColumnDescription().getPrimitiveType().getId()`, which returns null for the original MessageType without IDs, causing all metrics to be skipped.
## Changes
- Pass `parquetTypeWithIds` to `ParquetMetrics.metrics` to preserve field IDs from NameMapping
- Removed unused `messageType` variable
## Testing
- Added `testFooterMetricsWithNameMappingForFileWithoutIds` that verifies metrics are keyed by field IDs from NameMapping
--
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] Fix NameMapping loss in ParquetUtil.footerMetrics [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14617:
URL: https://github.com/apache/iceberg/pull/14617#issuecomment-3573233098
Thanks @tmater for the PR. Thanks @ebyhr @nandorKollar 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] Fix NameMapping loss in ParquetUtil.footerMetrics [iceberg]
Posted by "ebyhr (via GitHub)" <gi...@apache.org>.
ebyhr commented on code in PR #14617:
URL: https://github.com/apache/iceberg/pull/14617#discussion_r2540778041
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -264,6 +268,51 @@ public void testColumnStatisticsEnabled() throws Exception {
}
}
+ @Test
+ public void testFooterMetricsWithNameMappingForFileWithoutIds() throws IOException {
+ Schema schemaWithIds =
+ new Schema(
+ required(1, "id", Types.LongType.get()), optional(2, "data", Types.StringType.get()));
+
+ NameMapping nameMapping = MappingUtil.create(schemaWithIds);
+
+ File file = createTempFile(temp);
+
+ // Write a Parquet file WITHOUT field IDs using plain Avro schema
+ org.apache.avro.Schema avroSchemaWithoutIds =
+ org.apache.avro.SchemaBuilder.record("test")
+ .fields()
+ .requiredLong("id")
+ .optionalString("data")
+ .endRecord();
+
+ ParquetWriter<GenericData.Record> writer =
+ AvroParquetWriter.<GenericData.Record>builder(new org.apache.hadoop.fs.Path(file.toURI()))
+ .withDataModel(GenericData.get())
+ .withSchema(avroSchemaWithoutIds)
+ .build();
+
+ GenericData.Record record = new GenericData.Record(avroSchemaWithoutIds);
+ record.put("id", 1L);
+ record.put("data", "a");
+ writer.write(record);
+ writer.close();
+
+ InputFile inputFile = Files.localInput(file);
+
+ try (ParquetFileReader reader = ParquetFileReader.open(ParquetIO.file(inputFile))) {
+ MessageType parquetSchema = reader.getFooter().getFileMetaData().getSchema();
+ assertThat(ParquetSchemaUtil.hasIds(parquetSchema)).isFalse();
+
+ Metrics metrics =
+ ParquetUtil.footerMetrics(
+ reader.getFooter(), Stream.empty(), MetricsConfig.getDefault(), nameMapping);
+
+ // The key assertion: column sizes should be keyed by field IDs from NameMapping
+ assertThat(metrics.columnSizes()).containsKeys(1, 2);
Review Comment:
This assertion passes even when the `metrics.columnSizes()` contains additional keys. Could you use `containsOnlyKeys` instead?
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -264,6 +268,51 @@ public void testColumnStatisticsEnabled() throws Exception {
}
}
+ @Test
+ public void testFooterMetricsWithNameMappingForFileWithoutIds() throws IOException {
+ Schema schemaWithIds =
+ new Schema(
+ required(1, "id", Types.LongType.get()), optional(2, "data", Types.StringType.get()));
+
+ NameMapping nameMapping = MappingUtil.create(schemaWithIds);
+
+ File file = createTempFile(temp);
+
+ // Write a Parquet file WITHOUT field IDs using plain Avro schema
+ org.apache.avro.Schema avroSchemaWithoutIds =
+ org.apache.avro.SchemaBuilder.record("test")
+ .fields()
+ .requiredLong("id")
+ .optionalString("data")
+ .endRecord();
+
+ ParquetWriter<GenericData.Record> writer =
+ AvroParquetWriter.<GenericData.Record>builder(new org.apache.hadoop.fs.Path(file.toURI()))
Review Comment:
`builder(Path file)` is deprecated. Can we `builder(OutputFile file)` instead? You can refer to #14620
--
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] Fix NameMapping loss in ParquetUtil.footerMetrics [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao merged PR #14617:
URL: https://github.com/apache/iceberg/pull/14617
--
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] Fix NameMapping loss in ParquetUtil.footerMetrics [iceberg]
Posted by "nandorKollar (via GitHub)" <gi...@apache.org>.
nandorKollar commented on code in PR #14617:
URL: https://github.com/apache/iceberg/pull/14617#discussion_r2539872791
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestParquet.java:
##########
@@ -264,6 +268,51 @@ public void testColumnStatisticsEnabled() throws Exception {
}
}
+ @Test
+ public void testFooterMetricsWithNameMappingForFileWithoutIds() throws IOException {
+ Schema schemaWithIds =
+ new Schema(
+ required(1, "id", Types.LongType.get()), optional(2, "data", Types.StringType.get()));
+
+ NameMapping nameMapping = MappingUtil.create(schemaWithIds);
+
+ File file = createTempFile(temp);
+
+ // Write a Parquet file WITHOUT field IDs using plain Avro schema
+ org.apache.avro.Schema avroSchemaWithoutIds =
+ org.apache.avro.SchemaBuilder.record("test")
+ .fields()
+ .requiredLong("id")
+ .optionalString("data")
+ .endRecord();
+
+ ParquetWriter<GenericData.Record> writer =
Review Comment:
nit: you can use try-with-resource here too.
--
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] Fix NameMapping loss in ParquetUtil.footerMetrics [iceberg]
Posted by "tmater (via GitHub)" <gi...@apache.org>.
tmater commented on PR #14617:
URL: https://github.com/apache/iceberg/pull/14617#issuecomment-3573834240
Thanks @huaxingao , @nandorKollar, @ebyhr 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