[Bug] Fix Iceberg metadata unreadable by Snowflake - add Avro schema/partition-spec metadata to manifest files - #9497
Conversation
…partition-spec metadata to manifest files
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
I found several interoperability blockers in the manifest metadata change. Details are attached inline.
|
|
||
| // Compute Iceberg schema and partition spec for Avro manifest metadata. | ||
| // Snowflake and other Iceberg readers require these in the manifest file header. | ||
| IcebergSchema icebergSchema = IcebergSchema.create(table.schema()); |
There was a problem hiding this comment.
[P1] This still writes field ID 0 into the Iceberg schema. Schema.Builder assigns the first Paimon column ID 0, and IcebergDataField(DataField) preserves it. I verified that a manifest produced by this PR has "id" : 0 in its schema header, which is the incompatibility reported in #9012. Adding the header therefore does not demonstrate that Snowflake can read the table. Please introduce a consistent positive-ID mapping everywhere Iceberg IDs are emitted (schema, partition source IDs, metrics maps, and any physical schema IDs), and cover it with a compatibility regression test.
| IcebergPartitionSpec partitionSpec = new IcebergPartitionSpec(partitionFields); | ||
| Map<String, String> avroMetadata = new HashMap<>(); | ||
| avroMetadata.put("schema", icebergSchema.toJson()); | ||
| avroMetadata.put("partition-spec", JsonSerdeUtil.toJson(partitionSpec)); |
There was a problem hiding this comment.
[P1] Iceberg's partition-spec manifest metadata is a JSON array of partition fields, not the complete partition-spec object. This serializes an unpartitioned spec as {"spec-id":0,"fields":[]}. I reproduced the resulting failure with Iceberg 1.6.1 ManifestFiles.read: Cannot parse partition spec fields, not an array. Please use the equivalent of PartitionSpecParser.toJsonFields(spec) here, keep partition-spec-id separate, and add a test that opens the generated manifest through Iceberg without supplying an external spec map.
| List<IcebergPartitionField> partitionFields = | ||
| getPartitionFields(table.schema().partitionKeys(), icebergSchema); | ||
| IcebergPartitionSpec partitionSpec = new IcebergPartitionSpec(partitionFields); | ||
| Map<String, String> avroMetadata = new HashMap<>(); |
There was a problem hiding this comment.
[P1] Iceberg v2/v3 manifests require a content header whose value is data or deletes, but this map omits it; the generated manifest has content = null. A single constructor-level value would also be insufficient because this IcebergManifestFile writes both Content.DATA and Content.DELETES, selected only by rollingWrite. Please build the metadata per writer from its Content (or use separate writer factories), and test both data and delete manifests.
| avroMetadata.put("partition-spec", JsonSerdeUtil.toJson(partitionSpec)); | ||
| avroMetadata.put("partition-spec-id", String.valueOf(IcebergPartitionSpec.SPEC_ID)); | ||
| avroMetadata.put("format-version", String.valueOf(formatVersion)); | ||
| this.manifestFile = IcebergManifestFile.create(table, pathFactory, avroMetadata); |
There was a problem hiding this comment.
[P2] This only adds metadata to manifests created after the upgrade. createMetadataWithBase retains baseDataManifestFileMetas for add-only commits and retains existing DV manifests when there is no new index, so an already affected table remains a mixture of new and legacy headerless manifests and Snowflake still has to traverse the legacy files. Please provide a one-time manifest rewrite/migration path (or an explicit operational migration) and add an upgrade test starting from existing manifests.
2 participants
What changes were proposed in this pull request?
Fix #9012: Paimon's Iceberg manifest files (Avro format) were missing the required Avro file-level metadata (
schema,partition-spec,partition-spec-id,format-version) that Snowflake and other Iceberg readers require.Changes
AvroFileFormat: Added
AVRO_METADATAconfig option andsetAvroMetadata()static method to allow setting Avro file-level metadata key-value pairs in the container file header.IcebergManifestFile: Added
create(FileStoreTable, IcebergPathFactory, Map<String, String>)overload that passes Avro metadata through to the Avro format writer.IcebergCommitCallback: Computes the Iceberg schema and partition spec from the table schema at construction time and passes them as Avro metadata when creating the manifest file.
How was this patch tested?
mvn -pl paimon-format,paimon-core -am -Pfast-build compile