stevenzwu commented on code in PR #18416: URL: https://github.com/apache/iceberg/pull/18416#discussion_r4214206653
########## format/spec.md: ########## @@ -295,6 +297,12 @@ For details on how to serialize a schema to JSON, see Appendix C. [1001]: <https://portal.ogc.org/files/?artifact_id=25355> "OGC Simple feature access" +##### Vector + +Every non-null `vector(E, D)` value must contain exactly D non-null elements of type E. Floating-point elements must not be `NaN`, positive infinity, or negative infinity. The vector value itself may be null when the containing field is optional. Review Comment: The last sentence is doing more work than it looks like, and "containing field" makes it harder to read than it needs to be. Suggested: > A non-null `vector(E, D)` contains exactly D elements of type E, and every element is non-null. `float` and `double` elements must be finite: `NaN`, `+Infinity`, and `-Infinity` are invalid. The vector value may be null only when the vector field is optional. ########## 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: `identity` still allows every primitive except `geometry` and `geography`. We probably should exclude `vector` there. ########## format/spec.md: ########## @@ -1562,6 +1571,7 @@ Lists must use the [3-level representation](https://github.com/apache/parquet-fo | **`binary`** | `binary` | | | | **`struct`** | `group` | | | | **`list`** | `3-level list` | `LIST` | See Parquet docs for 3-level representation. | +| **`vector(E, D)`** | `3-level list` | `VECTOR(D)` | The element must be required and use the Parquet mapping for E. See [apache/parquet-format#624](https://github.com/apache/parquet-format/pull/624). | Review Comment: We need to restate the physical contract here instead of using an unmerged Parquet PR as the normative text. We can update it later as the Parquet spec finalizes. I am putting some keywords here. `VECTOR(D)`, repeated group `list`, required primitive `element` mapped from E, exactly D elements, elements non-null and finite. ########## format/spec.md: ########## @@ -1896,6 +1910,8 @@ The binary single-value serialization can be used to store the lower and upper b ### Version 4 +The `vector` type is added in v4. Writing `vector` into a v3 or earlier schema is invalid. Review Comment: this seems unnecessary. I don't think we say this for other new types introduced. ``` Writing `vector` into a v3 or earlier schema is invalid. ``` ########## format/spec.md: ########## @@ -295,6 +297,12 @@ For details on how to serialize a schema to JSON, see Appendix C. [1001]: <https://portal.ogc.org/files/?artifact_id=25355> "OGC Simple feature access" +##### Vector + +Every non-null `vector(E, D)` value must contain exactly D non-null elements of type E. Floating-point elements must not be `NaN`, positive infinity, or negative infinity. The vector value itself may be null when the containing field is optional. + +No type promotion to or from `vector` is defined, including changes to its element type or dimension. Lower and upper bounds are not defined for vector values. Review Comment: +1. I want to add two more points * "No type promotion is defined" reads as unspecified, not forbidden. * The `unknown` exception is the v3+ rule that `unknown` promotes to any type. Without it, this sentence contradicts that table. > A `vector` must not be promoted to another type. No type other than `unknown` may be promoted to `vector`. The element type and dimension must not change. A `list` of element type E is not a `vector(E, D)`, even when every value has length D. I would also add more details along with the upper and lower bounds > Equality of vector values is element-wise, using the equality rules of element type. A vector must not be an identifier field. No sort order is defined for a vector value. Writers must not write lower or upper bounds for a vector field. ########## format/spec.md: ########## @@ -1507,6 +1515,7 @@ Maps with non-string keys must use an array representation with the `map` logica |**`binary`**|`bytes`|| |**`struct`**|`record`|| |**`list`**|`array`|| +|**`vector(E, D)`**|`array`|Array elements must use the Avro mapping for E and must not be null. Readers and writers must enforce the vector dimension and element requirements.| Review Comment: Avro stores both `list` and `vector` as `array`. The file type alone does not say which one it is. Iceberg marks a list by writing `element-id` on the Avro array. That is the list element's field id. A vector has no element field id, so its array must not have `element-id`. A writer that adds one makes the file look like a list. Suggested note on the Avro row: > Array elements must use the Avro mapping for E and must not be null. The array must not have `element-id`. Readers and writers enforce dimension D and the element rules from the Iceberg schema. An array of length D is not a vector. -- 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]
