sungwy commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3444034055
##########
format/spec.md:
##########
@@ -1679,14 +1675,16 @@ Types are serialized according to this table:
|**`uuid`**|`JSON string: "uuid"`|`"uuid"`|
|**`fixed(L)`**|`JSON string: "fixed[<L>]"`|`"fixed[16]"`|
|**`binary`**|`JSON string: "binary"`|`"binary"`|
-|**`decimal(P, S)`**|`JSON string: "decimal(<P>,<S>)"`|`"decimal(9,2)"`,<br
/>`"decimal(9, 2)"`|
+|**`decimal(P, S)`**|`JSON string: "decimal(<P>, <S>)"`|`"decimal(9, 2)"`|
|**`struct`**|`JSON object: {`<br /> `"type": "struct",`<br
/> `"fields": [ {`<br /> `"id": <field id
int>,`<br /> `"name": <name string>,`<br
/> `"required": <boolean>,`<br
/> `"type": <type JSON>,`<br
/> `"doc": <comment string>,`<br
/> `"initial-default": <JSON encoding of default
value>,`<br /> `"write-default": <JSON encoding of
default value>`<br /> `}, ...`<br /> `]
}`|`{`<br /> `"type": "struct",`<br /> `"fields": [ {`<br
/> `"id": 1,`<br /> `"name":
"id",`<br /> `"required": true,`<br
/> `"type": "uuid",`<br
/> `"initial-default":
"0db3e2a8-9d1d-42b9-aa7b-74ebe558dceb",`<br
/> `"write-default": "ec5911be
-b0a7-458c-8438-c9a3e53cffae"`<br /> `}, {`<br
/> `"id": 2,`<br /> `"name":
"data",`<br /> `"required": false,`<br
/> `"type": {`<br
/> `"type": "list",`<br
/> `...`<br /> `}`<br
/> `} ]`<br />`}`|
|**`list`**|`JSON object: {`<br /> `"type": "list",`<br
/> `"element-id": <id int>,`<br /> `"element-required":
<bool>`<br /> `"element": <type JSON>`<br />`}`|`{`<br
/> `"type": "list",`<br /> `"element-id": 3,`<br
/> `"element-required": true,`<br /> `"element":
"string"`<br />`}`|
|**`map`**|`JSON object: {`<br /> `"type": "map",`<br
/> `"key-id": <key id int>,`<br /> `"key": <type
JSON>,`<br /> `"value-id": <val id int>,`<br
/> `"value-required": <bool>`<br /> `"value": <type
JSON>`<br />`}`|`{`<br /> `"type": "map",`<br
/> `"key-id": 4,`<br /> `"key": "string",`<br
/> `"value-id": 5,`<br /> `"value-required": false,`<br
/> `"value": "double"`<br />`}`|
| **`variant`**| `JSON string: "variant"`|`"variant"`|
| **`geometry(C)`** |`JSON string: "geometry(<C>)"`|`"geometry(srid:4326)"`|
| **`geography(C, A)`** |`JSON string:
"geography(<C>,<E>)"`|`"geography(srid:4326,spherical)"`|
+The schema JSON type strings in this table are the canonical serialized forms.
Readers should accept optional whitespace around parameters and separators in
parameterized type strings.
Review Comment:
nit: I agree with @singhpk234 and @laskoviymishka 's points - I think we
should use normative `SHOULD` or `MUST`
I looked up all our language library readers, and here's an analysis of our
current parsing mechanisms. Based on the proposed language that requires
Readers to support optional whitespace around _parameters_ and separators,
PyIceberg isn't SPEC conformant today (It can parse `decimal(9, 2)` but not
`decimal( 9 , 2 )`). Hence, my vote is for this to be `SHOULD`.
| impl | mechanism | whitespace allowed |
|---|---|---|
| Java | `decimal\(\s*(\d+)\s*,\s*(\d+)\s*\)` | around params + separator |
| Go | `decimal\(\s*(\d+)\s*,\s*(\d+)\s*\)` | around params + separator |
| C++ | `decimal\(\s*(\d+)\s*,\s*(\d+)\s*\)` (+ explicit `decimal( 10, 2 )`
test) | around params + separator |
| Rust | `split_once(',')` then `.trim()` each part | around params +
separator |
| PyIceberg | `decimal\((\d+),\s*(\d+)\)` | only after separator |
```suggestion
The schema JSON type strings in this table are the canonical serialized
forms. Readers SHOULD accept optional whitespace around parameters and
separators in parameterized type strings.
```
--
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]
