JingsongLi commented on code in PR #9497:
URL: https://github.com/apache/paimon/pull/9497#discussion_r3893797934
##########
paimon-core/src/main/java/org/apache/paimon/iceberg/IcebergCommitCallback.java:
##########
@@ -200,6 +199,21 @@ public IcebergCommitCallback(FileStoreTable table, String
commitUser) {
"Unsupported iceberg format version! Only version 2 or version
3 is valid, but current version is ",
formatVersion);
+ // 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());
+ List<IcebergPartitionField> partitionFields =
+ getPartitionFields(table.schema().partitionKeys(),
icebergSchema);
+ IcebergPartitionSpec partitionSpec = new
IcebergPartitionSpec(partitionFields);
+ Map<String, String> avroMetadata = new HashMap<>();
+ avroMetadata.put("schema", icebergSchema.toJson());
+ avroMetadata.put("partition-spec",
JsonSerdeUtil.toJson(partitionSpec));
Review 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.
##########
paimon-core/src/main/java/org/apache/paimon/iceberg/IcebergCommitCallback.java:
##########
@@ -200,6 +199,21 @@ public IcebergCommitCallback(FileStoreTable table, String
commitUser) {
"Unsupported iceberg format version! Only version 2 or version
3 is valid, but current version is ",
formatVersion);
+ // 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());
+ List<IcebergPartitionField> partitionFields =
+ getPartitionFields(table.schema().partitionKeys(),
icebergSchema);
+ IcebergPartitionSpec partitionSpec = new
IcebergPartitionSpec(partitionFields);
+ Map<String, String> avroMetadata = new HashMap<>();
Review 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.
##########
paimon-core/src/main/java/org/apache/paimon/iceberg/IcebergCommitCallback.java:
##########
@@ -200,6 +199,21 @@ public IcebergCommitCallback(FileStoreTable table, String
commitUser) {
"Unsupported iceberg format version! Only version 2 or version
3 is valid, but current version is ",
formatVersion);
+ // 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());
+ List<IcebergPartitionField> partitionFields =
+ getPartitionFields(table.schema().partitionKeys(),
icebergSchema);
+ IcebergPartitionSpec partitionSpec = new
IcebergPartitionSpec(partitionFields);
+ Map<String, String> avroMetadata = new HashMap<>();
+ avroMetadata.put("schema", icebergSchema.toJson());
+ 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);
Review 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.
##########
paimon-core/src/main/java/org/apache/paimon/iceberg/IcebergCommitCallback.java:
##########
@@ -200,6 +199,21 @@ public IcebergCommitCallback(FileStoreTable table, String
commitUser) {
"Unsupported iceberg format version! Only version 2 or version
3 is valid, but current version is ",
formatVersion);
+ // 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());
Review 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.
--
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]
