dramaticlly commented on PR #16449: URL: https://github.com/apache/iceberg/pull/16449#issuecomment-4806641933
Want to callout the potential behavior change due to removal of the `SystemConfigs .NETFLIX_UNSAFE_PARQUET_ID_FALLBACK_ENABLED` config since 1.7.0, given #9324 suggested that we shall always use `NameMapping.empty()` as the the safe default for assigning column IDs by position if a file is missing IDs and there is no name mapping. Before we have https://github.com/apache/iceberg/blob/cfebc8ed16fcdee7a28ad92635adbb744b0258bc/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java#L1556-L1563, after this PR the default change from null to `NameMapping.empty()` | `mapping` value | Behavior when file has no field IDs | |---|---| | `null` | unsafe positional ID assignment | | `NameMapping.empty()` | strict resolution against an empty mapping, projected fields read as null / fail | #### Test migrations off positional fallback These tests wrote Parquet files via raw `AvroParquetWriter` / `ParquetWriteSupport` (no Iceberg field IDs) and read them back without a `NameMapping`. They had been passing only because the default for unset `NETFLIX_UNSAFE_PARQUET_ID_FALLBACK_ENABLED` was `true`. With the strict default in place, each is migrated to supply `withNameMapping(MappingUtil.create(schema))` — which is exactly the API the deprecation message pointed users at: - [`ac9ee16a64`](https://github.com/apache/iceberg/pull/16449/commits/ac9ee16a64) `TestGenericData.testTwoLevelList`, `TestParquetEncryptionWithWriteSupport.testTwoLevelList` + a new `TestGenericData.testReadWithoutFieldIdsOrNameMappingReturnsNullFields` that asserts the new strict-default behavior (previously **no test** covered the `NameMapping.empty()` path that the PR is shipping). - [`8da506219d`](https://github.com/apache/iceberg/pull/16449/commits/8da506219d) `TestFlinkParquetReader.testTwoLevelList` across `v1.20`, `v2.0`, `v2.1`. - [`8846661ea7`](https://github.com/apache/iceberg/pull/16449/commits/8846661ea7) `TestSparkParquetReader.testInt96TimestampProducedBySparkIsReadCorrectly` across `v3.5`, `v4.0`, `v4.1`. #### Production fixes auto-applying the table's `schema.name-mapping.default` While migrating the Spark tests we found two real product gaps where Iceberg APIs were not consulting the well-known `DEFAULT_NAME_MAPPING` table property even though the comparable APIs already did. Both gaps used to be invisible because the same callers relied on the unsafe positional fallback; with that path gone, the gaps surface as test failures. Each commit brings the API into line with the existing pattern in `AddFilesProcedure.ensureNameMappingPresent` / `BaseReader`: - [`9ebb273258`](https://github.com/apache/iceberg/pull/16449/commits/9ebb273258) — `SparkTableUtil.importSparkTable` now auto-sets `schema.name-mapping.default` on the target table when not already set, matching the long-standing behavior of `AddFilesProcedure` and `BaseTableCreationSparkAction`. Without this, every user who imported a non-Iceberg Parquet table needed to manually configure name mapping to read it back. - [`8846661ea7`](https://github.com/apache/iceberg/pull/16449/commits/8846661ea7) — `GenericReader` (used by `IcebergGenerics.read(table)`) now reads `schema.name-mapping.default` from the table properties and threads it through `ReadBuilder.withNameMapping(...)`, matching what Spark's `BaseReader` does. Without this, even tables that **do** have the property set would still fail to apply it on the generic read 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
