RussellSpitzer commented on code in PR #10831: URL: https://github.com/apache/iceberg/pull/10831#discussion_r1826190048
########## format/spec.md: ########## @@ -444,6 +449,9 @@ Sorting floating-point numbers should produce the following behavior: `-NaN` < ` A data or delete file is associated with a sort order by the sort order's id within [a manifest](#manifests). Therefore, the table must declare all the sort orders for lookup. A table could also be configured with a default sort order id, indicating how the new data should be sorted by default. Writers should use this default sort order to sort the data on write, but are not required to if the default order is prohibitively expensive, as it would be for streaming writes. +Note: + +1. The ability to sort `variant` columns and the specific sort order is determined by the engines. Review Comment: I think we should specifically forbid sort orders containing a variant. I think we actually are underdetermined in the spec here. We have the following checks in the Reference Implementation https://github.com/apache/iceberg/blob/8a16a417492eb6ccb1895b4b6db3536ff2a8b67d/api/src/main/java/org/apache/iceberg/SortOrder.java#L301-L311 So currently, even though we don't specify this here, you cannot make a sort order with array or map. I think we should explicitly call this out and add variant as well. My real concern here is that we add the ability to sort on something but don't define what that sorting actually looks like. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
