anoopj commented on code in PR #16958:
URL: https://github.com/apache/iceberg/pull/16958#discussion_r3555857111
##########
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:
Fixed. `partitionType` now unions all partition fields across specs and the
reader's builder no longer takes a table schema. I left the existing
`partitionType(Table)` unchanged since fixing it has wider implications; we can
revisit separately. Added a test that a partition field survives dropping its
source column.
I agree the output type is only underivable for identity and truncate, but
`PartitionSpec.partitionType()` currently returns `unknown` whenever the source
column is missing so today even a bucket field degrades. If it makes sense, I
can do a follow-up that only falls back to unknown for source-dependent
transforms, which shrinks this to the rare case you described. I also did some
code reading in Iceberg rust, which seems to have a hard failure, possibly
because `unknown` type is not supported yet in Iceberg Rust.
BTW +1 to storing the output type in metadata. v4 seems like the right time
to add it since we're already revising the spec. It seems like a good overall
improvement. Happy to write that up.
--
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]
