Kontinuation commented on code in PR #12533:
URL: https://github.com/apache/iceberg/pull/12533#discussion_r2000389268


##########
format/spec.md:
##########
@@ -1395,8 +1395,8 @@ Types are serialized according to this table:
 |**`list`**|`JSON object: {`<br />&nbsp;&nbsp;`"type": "list",`<br 
/>&nbsp;&nbsp;`"element-id": <id int>,`<br />&nbsp;&nbsp;`"element-required": 
<bool>`<br />&nbsp;&nbsp;`"element": <type JSON>`<br />`}`|`{`<br 
/>&nbsp;&nbsp;`"type": "list",`<br />&nbsp;&nbsp;`"element-id": 3,`<br 
/>&nbsp;&nbsp;`"element-required": true,`<br />&nbsp;&nbsp;`"element": 
"string"`<br />`}`|
 |**`map`**|`JSON object: {`<br />&nbsp;&nbsp;`"type": "map",`<br 
/>&nbsp;&nbsp;`"key-id": <key id int>,`<br />&nbsp;&nbsp;`"key": <type 
JSON>,`<br />&nbsp;&nbsp;`"value-id": <val id int>,`<br 
/>&nbsp;&nbsp;`"value-required": <bool>`<br />&nbsp;&nbsp;`"value": <type 
JSON>`<br />`}`|`{`<br />&nbsp;&nbsp;`"type": "map",`<br 
/>&nbsp;&nbsp;`"key-id": 4,`<br />&nbsp;&nbsp;`"key": "string",`<br 
/>&nbsp;&nbsp;`"value-id": 5,`<br />&nbsp;&nbsp;`"value-required": false,`<br 
/>&nbsp;&nbsp;`"value": "double"`<br />`}`|
 | **`variant`**| `JSON string: "variant"`|`"variant"`|
-| **`geometry(C)`** | `JSON object: {`<br />&nbsp;&nbsp;`"type": 
"geometry",`<br />&nbsp;&nbsp;`"crs": <C>`<br />`}` | `{`<br 
/>&nbsp;&nbsp;`"type": "geometry",`<br />&nbsp;&nbsp;`"crs": "srid:4326"`<br 
/>`}`  |
-| **`geography(C, A)`** | `JSON object: {`<br />&nbsp;&nbsp;`"type": 
"geography",`<br />&nbsp;&nbsp;`"crs": <C>,`<br />&nbsp;&nbsp;`"algorithm": 
<A>`<br />}` | `{`<br />&nbsp;&nbsp;`"type": "geography",`<br 
/>&nbsp;&nbsp;`"crs": "srid:4326",`<br />&nbsp;&nbsp;`"algorithm": "spherical"` 
<br /> `}`  |
+| **`geometry(C)`** |`JSON string: "geometry(<C>)"`|`"geometry(srid:4326)"`|
+| **`geography(C, A)`** |`JSON string: 
"geography(<C>,<E>)"`|`"geography(srid:4326,spherical)"`|

Review Comment:
   Is it better to add another example with space in between C and A? Such as 
`"geography(srid:4326, spherical)"`.
   
   Also, C and A could be omitted. We can add examples for all possible cases:
   
   * The JSON representation for geography types with both C and A omitted 
should be `"geography"`. 
   * If only A is omitted, it should be `"geography(C)"`
   * If only C is omitted, it should be `"geography(OGC:CRS84, C)"`



##########
format/spec.md:
##########
@@ -1395,8 +1395,8 @@ Types are serialized according to this table:
 |**`list`**|`JSON object: {`<br />&nbsp;&nbsp;`"type": "list",`<br 
/>&nbsp;&nbsp;`"element-id": <id int>,`<br />&nbsp;&nbsp;`"element-required": 
<bool>`<br />&nbsp;&nbsp;`"element": <type JSON>`<br />`}`|`{`<br 
/>&nbsp;&nbsp;`"type": "list",`<br />&nbsp;&nbsp;`"element-id": 3,`<br 
/>&nbsp;&nbsp;`"element-required": true,`<br />&nbsp;&nbsp;`"element": 
"string"`<br />`}`|
 |**`map`**|`JSON object: {`<br />&nbsp;&nbsp;`"type": "map",`<br 
/>&nbsp;&nbsp;`"key-id": <key id int>,`<br />&nbsp;&nbsp;`"key": <type 
JSON>,`<br />&nbsp;&nbsp;`"value-id": <val id int>,`<br 
/>&nbsp;&nbsp;`"value-required": <bool>`<br />&nbsp;&nbsp;`"value": <type 
JSON>`<br />`}`|`{`<br />&nbsp;&nbsp;`"type": "map",`<br 
/>&nbsp;&nbsp;`"key-id": 4,`<br />&nbsp;&nbsp;`"key": "string",`<br 
/>&nbsp;&nbsp;`"value-id": 5,`<br />&nbsp;&nbsp;`"value-required": false,`<br 
/>&nbsp;&nbsp;`"value": "double"`<br />`}`|
 | **`variant`**| `JSON string: "variant"`|`"variant"`|
-| **`geometry(C)`** | `JSON object: {`<br />&nbsp;&nbsp;`"type": 
"geometry",`<br />&nbsp;&nbsp;`"crs": <C>`<br />`}` | `{`<br 
/>&nbsp;&nbsp;`"type": "geometry",`<br />&nbsp;&nbsp;`"crs": "srid:4326"`<br 
/>`}`  |
-| **`geography(C, A)`** | `JSON object: {`<br />&nbsp;&nbsp;`"type": 
"geography",`<br />&nbsp;&nbsp;`"crs": <C>,`<br />&nbsp;&nbsp;`"algorithm": 
<A>`<br />}` | `{`<br />&nbsp;&nbsp;`"type": "geography",`<br 
/>&nbsp;&nbsp;`"crs": "srid:4326",`<br />&nbsp;&nbsp;`"algorithm": "spherical"` 
<br /> `}`  |
+| **`geometry(C)`** |`JSON string: "geometry(<C>)"`|`"geometry(srid:4326)"`|
+| **`geography(C, A)`** |`JSON string: 
"geography(<C>,<E>)"`|`"geography(srid:4326,spherical)"`|

Review Comment:
   Is it better to add another example with space in between C and A? Such as 
`"geography(srid:4326, spherical)"`.
   
   Also, C and A could be omitted. We can add examples for all possible cases:
   
   * The JSON representation for geography types with both C and A omitted 
should be `"geography"`. 
   * If only A is omitted, it should be `"geography(C)"`
   * If only C is omitted, it should be `"geography(OGC:CRS84, A)"`



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

Reply via email to