stevenzwu commented on code in PR #18416:
URL: https://github.com/apache/iceberg/pull/18416#discussion_r4298992256


##########
format/spec.md:
##########
@@ -282,6 +283,7 @@ Supported primitive types are defined in the table below. 
Primitive types added
 |                  | **`uuid`**         | Universally unique identifiers       
                                    | Should use 16-byte fixed                  
       |
 |                  | **`fixed(L)`**     | Fixed-length byte array of length L  
                                    |                                           
       |
 |                  | **`binary`**       | Arbitrary-length byte array          
                                    |                                           
       |
+| [v4](#version-4) | **`vector(E, D)`** | Fixed-length collection of finite 
numeric values with element type E and dimension D | E must be `int`, `long`, 
`float`, or `double`; D must be positive |

Review Comment:
   It seems that my original comment on this line got buried deep by other 
reply comments. Restarting a new thread.
   
   Classifying `vector` as a primitive exposes two inconsistencies:
   
   **Identity:** In this spec, `identity` accepts any primitive except 
`geometry` and `geography`, so adding `vector` silently makes it a valid source 
type. Rather than extending the exclusion list, could we use an explicit 
allowlist of supported primitive types, as `bucket` already does? 
`format/expressions-spec.md` uses the same broad rule for `identity` and should 
be aligned.
   
   **Bucket:** No change is needed in this spec: `bucket` already has an 
explicit allowlist that does not include `vector`. However, 
`format/expressions-spec.md` defines `bucket` as accepting any primitive except 
`geometry`, `geography`, `boolean`, `float`, and `double`. That admits 
`vector(int, D)` and `vector(long, D)`, while Appendix B defines no vector 
hash. The expressions spec should use the same allowlist as this spec.
   
   `format/expressions-spec.md` is outside this diff, but both specs should 
remain consistent.
   
   



-- 
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]

Reply via email to