apache / iceberg

Apache Iceberg
https://iceberg.apache.org/
Apache License 2.0
6.46k stars 2.23k forks source link

[Spark] Identity partition on required column generates nullable partition tuple in manifest file #11300

Open mosenberg opened 1 month ago

mosenberg commented 1 month ago

Apache Iceberg version

None

Query engine

Spark

Please describe the bug 🐞

The issue repros using the following SQL:

CREATE TABLE iceberg.NullabilityPartition(
  group STRING NOT NULL,
  val INTEGER
)
Partitioned BY (group)
TBLPROPERTIES(`format-version`=2,
              `write.parquet.compression-codec`='snappy',
               write.delete.format.default='parquet',
               write.delete.mode='merge-on-read',
               write.update.mode='merge-on-read',
               write.merge.mode='merge-on-read');

INSERT INTO iceberg.NullabilityPartition Select * from VALUES ('foo',1),('foo',2);

As per the above SQL, the column group is defined as NOT NULL (i.e. required) column in the Iceberg metadata schema. However, in the generated avro manifest file, the partition tuple - which stores the value of the group column by which the table is identity-partitioned - the partition value is stored as an avro union type ["null", "string"].

As per my understanding of the Iceberg spec, this is not correct: The output value of an identity partition transform is equal to the source type - in this case STRING NOT NULL. The section on manifest files further states:

Partition data tuple, schema based on the partition spec output using partition field ids for the struct field ids

Hence the schema of the partition tuple should be "string" and not ["null","string"].

Willingness to contribute

RussellSpitzer commented 1 week ago

I don't know if this is really a bug since there is no behavior change really associated with it. I believe this is a leftover from V1 where we were unable to ever remove partition tuple elements so every element had to be essentially nullable incase a new partition spec that no longer included the tuple element.

We could change this for V2 metadata files though. Not sure it's a high priority though unless there is something actually breaking because of this