rdblue commented on code in PR #16958:
URL: https://github.com/apache/iceberg/pull/16958#discussion_r3540404401
##########
core/src/main/java/org/apache/iceberg/Partitioning.java:
##########
@@ -238,9 +238,18 @@ public static StructType groupingKeyType(Schema schema,
Collection<PartitionSpec
* @return the constructed unified partition type
*/
public static StructType partitionType(Table table) {
- Collection<PartitionSpec> specs = table.specs().values();
- return buildPartitionProjectionType(
- "table partition", specs, allActiveFieldIds(table.schema(), specs));
+ return partitionType(table.schema(), table.specs().values());
+ }
+
+ /**
+ * Builds a unified partition type from a schema and its specs, unioning
every partition field
+ * whose source column is present in the schema.
+ *
+ * @param schema the schema used to determine which partition fields are
active
+ * @param specs the partition specs to unify
+ */
+ static StructType partitionType(Schema schema, Collection<PartitionSpec>
specs) {
+ return buildPartitionProjectionType("table partition", specs,
allActiveFieldIds(schema, specs));
Review Comment:
I don't think using `allActiveFieldIds` is correct here.
That method will filter out partition fields when the source is no longer
part of the table schema. In some cases (bucket joins), that is fine because
the data is still partitioned by a subset of its partition fields.
However, for a partition type that can hold any partition tuple, it doesn't
work. Equality deletes are matched using partition tuple equality. For example,
say I have `file_a.parquet` in partition `(id_bucket=7, is_test=false)`, and I
also have a delete file `deletes.parquet` in partition `(id_bucket=7,
is_test=true)`. The delete file does not apply to the data file because they
are in different partitions.
With the use of `allActiveFieldIds` here, dropping the `is_test` identity
partition and then the `is_test` column from the table would result in a
unified partition type that only has `id_bucket`. As a result, checking whether
the partition matches (and the deletes should be applied) will pass for
`file_a.parquet` and `deletes.parquet`.
This needs to union all partition fields so that we don't drop tuple values
that define equality.
--
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]
