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.
Follow-up: https://github.com/apache/iceberg/pull/18545
##########
format/spec.md:
##########
@@ -295,6 +297,14 @@ 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
+
+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.
Review Comment:
I would remove the `non-null` word from the beginning of the sentence.
```suggestion
A `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:
##########
@@ -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:
@PFischbeck thanks for starting the Avro dev thread. There is no replies so
far. As agreed in the sync, Avro discussion shouldn't be a blocker.
##########
format/spec.md:
##########
@@ -295,6 +297,14 @@ 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
+
+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.
+
+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.
Review Comment:
"every value" does not say list; "size" is not the spec's word. Suggested
change is "even if every list instance contains exactly D elements."
```suggestion
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 if every list
instance contains exactly D elements.
```
--
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]
