You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "kevinjqliu (via GitHub)" <gi...@apache.org> on 2026/06/13 18:46:23 UTC
[PR] Docs: Clarify decimal type serialization [iceberg]
kevinjqliu opened a new pull request, #16798:
URL: https://github.com/apache/iceberg/pull/16798
## Summary
Clarify the canonical JSON serialization for decimal type strings in Appendix C by documenting the space after the comma and keeping only the canonical decimal example.
## Context
Follows the discussion in apache/iceberg-rust#2538, where iceberg-rust aligned decimal type serialization with Java and Python output.
## Testing
- git diff --check
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3429904432
##########
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:
`iceberg-rust` was writing without space `decimal(P,S)`, https://github.com/apache/iceberg-rust/pull/2538 changed it to write with the space.
I think we should use "should" here. "must" would make accepting optional whitespace a hard conformance rule, while this is intended as reader-side compatibility for non-canonical 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3510716742
##########
format/spec.md:
##########
@@ -268,7 +268,7 @@ Supported primitive types are defined in the table below. Primitive types added
| | **`long`** | 64-bit signed integers | |
| | **`float`** | [32-bit IEEE 754](https://en.wikipedia.org/wiki/IEEE_754) floating point | Can promote to double |
| | **`double`** | [64-bit IEEE 754](https://en.wikipedia.org/wiki/IEEE_754) floating point | |
-| | **`decimal(P,S)`** | Fixed-point decimal; precision P, scale S | Scale is fixed, precision must be 38 or less |
+| | **`decimal(P, S)`** | Fixed-point decimal; precision P, scale S | Scale is fixed, precision must be 38 or less |
Review Comment:
I feel like its a good idea to document the "canonical" format, just to avoid any confusion. its a nit change, and im happy to revert it.
With the "hide whitespace" option, this PR becomes 2 lines of change + an optional formatting improvement
https://github.com/apache/iceberg/pull/16798/changes?w=1
Let me know what you think, I'm happy to move forward either way
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3508546098
##########
format/spec.md:
##########
@@ -268,7 +268,7 @@ Supported primitive types are defined in the table below. Primitive types added
| | **`long`** | 64-bit signed integers | |
| | **`float`** | [32-bit IEEE 754](https://en.wikipedia.org/wiki/IEEE_754) floating point | Can promote to double |
| | **`double`** | [64-bit IEEE 754](https://en.wikipedia.org/wiki/IEEE_754) floating point | |
-| | **`decimal(P,S)`** | Fixed-point decimal; precision P, scale S | Scale is fixed, precision must be 38 or less |
+| | **`decimal(P, S)`** | Fixed-point decimal; precision P, scale S | Scale is fixed, precision must be 38 or less |
Review Comment:
This feels unnecessary since it's documentation and it's about representation. I wouldn't say this needs to be updated.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3413939723
##########
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:
+1 for MUST on the canonical output, effectively all implementations (Java, PyIceberg, iceberg-rust, iceberg-go) already emit decimal(P, S), making it normative just codifies existing behavior.
Reader-side whitespace tolerance as a SHOULD makes sense too; all of them already parse leniently.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3408549397
##########
site/docs/assets/stylesheets/extra.css:
##########
@@ -231,6 +231,10 @@ header .md-search__input::placeholder {
font-family: "Source Sans Pro", sans-serif;
}
+.md-typeset table code {
Review Comment:
need to properly render table cells on the same line, otherwise longer strings like "decimal(P,S)" might be split into multiple lines
<img width="601" height="80" alt="Image" src="https://github.com/user-attachments/assets/6e602635-b43e-47cd-ba77-b2623b5eba60" />
##########
format/spec.md:
##########
@@ -268,7 +268,7 @@ Supported primitive types are defined in the table below. Primitive types added
| | **`long`** | 64-bit signed integers | |
| | **`float`** | [32-bit IEEE 754](https://en.wikipedia.org/wiki/IEEE_754) floating point | Can promote to double |
| | **`double`** | [64-bit IEEE 754](https://en.wikipedia.org/wiki/IEEE_754) floating point | |
-| | **`decimal(P,S)`** | Fixed-point decimal; precision P, scale S | Scale is fixed, precision must be 38 or less |
+| | **`decimal(P, S)`** | Fixed-point decimal; precision P, scale S | Scale is fixed, precision must be 38 or less |
Review Comment:
changed all `decimal(P,S)` to `decimal(P, S)`
##########
format/spec.md:
##########
Review Comment:
removed non-canonical example `decimal(9,2)`
and add a sentence below to explain that readers should accept from without space, `decimal(9,2)`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#issuecomment-5152278948
this is a clarification of the spec, going to merge as is. happy to follow up if necessary
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu merged PR #16798:
URL: https://github.com/apache/iceberg/pull/16798
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3410057715
##########
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:
SHOULD or MUST ?
what are the other lang implementations do ATM
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3555302380
##########
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:
What is the argument for capitalized versions? Is it not clear when they are not capitalized?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
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 woudln't be SPEC conformant today if we made this a MUST (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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#issuecomment-4755165981
What does everyone think about placing the regex itself into the spec (perhaps as a footnote for readability)?
The human readable examples are great, but translating human writing to regex can always lead to issues.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3545478631
##########
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:
i like the flare the capitalized version (`SHOULD`) gives, but it doesnt look like anywhere else in the spec use the capitalized form, so I'm going to skip the adding it 😄
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3555341550
##########
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:
i suspect its from how we write the design docs, i've seen SHOULD / MUST (capitalized version) there
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#issuecomment-4732485288
> I feel the readability of the content has decreased. We need to scroll to view the example.
I agree the table itself is harder to view due to the horizontal scrolling, lets address that as a follow up. I think we can reclaim a lot of horizontal real estate from both of the side bars
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#discussion_r3508546098
##########
format/spec.md:
##########
@@ -268,7 +268,7 @@ Supported primitive types are defined in the table below. Primitive types added
| | **`long`** | 64-bit signed integers | |
| | **`float`** | [32-bit IEEE 754](https://en.wikipedia.org/wiki/IEEE_754) floating point | Can promote to double |
| | **`double`** | [64-bit IEEE 754](https://en.wikipedia.org/wiki/IEEE_754) floating point | |
-| | **`decimal(P,S)`** | Fixed-point decimal; precision P, scale S | Scale is fixed, precision must be 38 or less |
+| | **`decimal(P, S)`** | Fixed-point decimal; precision P, scale S | Scale is fixed, precision must be 38 or less |
Review Comment:
This feels unnecessary since it's documentation and it's not about representation. I wouldn't say this needs to be updated.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Spec: Clarify decimal type serialization [iceberg]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #16798:
URL: https://github.com/apache/iceberg/pull/16798#issuecomment-4916740919
> What does everyone think about placing the regex itself into the spec (perhaps as a footnote for readability)?
I find regex more confusing tbh. I feel the current explanation is enough to decipher the supported forms
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org