stevenzwu commented on code in PR #15834: URL: https://github.com/apache/iceberg/pull/15834#discussion_r3243385778
########## format/spec.md: ########## @@ -242,10 +242,12 @@ For `geometry` type, the CRS does not affect geometric calculations, which are a The default CRS value `OGC:CRS84` means that the objects must be stored in longitude, latitude based on the WGS84 datum. -Custom CRS values can be specified by a string of the format `type:identifier`, where `type` is one of the following values: +Non-default CRS values are specified by any string that uniquely identifies a coordinate reference system associated with this type. +To maximize interoperability, suggested formats for CRS include, but are not limited to: +* `<context>:<identifier`: Identifies a CRS by name or other identifier in some well-documented context. Examples: `OGC:CRS84`, `EPSG:4326`, `IGNF:ATI` and `SRID:0` Review Comment: The placeholder syntax looks malformed here: ``<identifier>`` is missing the closing `>`. ########## format/spec.md: ########## @@ -242,10 +242,12 @@ For `geometry` type, the CRS does not affect geometric calculations, which are a The default CRS value `OGC:CRS84` means that the objects must be stored in longitude, latitude based on the WGS84 datum. -Custom CRS values can be specified by a string of the format `type:identifier`, where `type` is one of the following values: +Non-default CRS values are specified by any string that uniquely identifies a coordinate reference system associated with this type. +To maximize interoperability, suggested formats for CRS include, but are not limited to: +* `<context>:<identifier`: Identifies a CRS by name or other identifier in some well-documented context. Examples: `OGC:CRS84`, `EPSG:4326`, `IGNF:ATI` and `SRID:0` +* `projjson:<property-name>` - where <property-name> refers to a table property where CRS definition in [PROJJSON](https://proj.org/en/stable/specifications/projjson.html) format is stored. -* `srid`: [Spatial reference identifier](https://en.wikipedia.org/wiki/Spatial_reference_system#Identifier), `identifier` is the SRID itself. -* `projjson`: [PROJJSON](https://proj.org/en/stable/specifications/projjson.html), `identifier` is the name of a table property where the projjson string is stored. +CRS value must not contain inlined PROJJSON definitions and implementations must not parse the contents of the CRS as PROJJSON. PROJJSON definition are very verbose, hence inlining them as part of schema would cause significant performance degradation. If intention is for PROJJSON definition to be part of the table metadata, then it must be stored in a table property and referenced from the CRS field using the `projjson:<table_property_name>` form described above. Review Comment: A few grammar nits in this paragraph: `PROJJSON definition are` -> `PROJJSON definitions are`, and `If intention is for PROJJSON definition...` would read more naturally as `If the intention is for a PROJJSON definition...`. ########## format/spec.md: ########## @@ -242,10 +242,12 @@ For `geometry` type, the CRS does not affect geometric calculations, which are a The default CRS value `OGC:CRS84` means that the objects must be stored in longitude, latitude based on the WGS84 datum. -Custom CRS values can be specified by a string of the format `type:identifier`, where `type` is one of the following values: +Non-default CRS values are specified by any string that uniquely identifies a coordinate reference system associated with this type. +To maximize interoperability, suggested formats for CRS include, but are not limited to: +* `<context>:<identifier`: Identifies a CRS by name or other identifier in some well-documented context. Examples: `OGC:CRS84`, `EPSG:4326`, `IGNF:ATI` and `SRID:0` Review Comment: We changed `type` to `context` here. I am not a geo domain expert. I am just wondering if `context` is a widely accepted term in the geo space. is `authority` too restrictive? ########## format/spec.md: ########## @@ -242,10 +242,12 @@ For `geometry` type, the CRS does not affect geometric calculations, which are a The default CRS value `OGC:CRS84` means that the objects must be stored in longitude, latitude based on the WGS84 datum. -Custom CRS values can be specified by a string of the format `type:identifier`, where `type` is one of the following values: +Non-default CRS values are specified by any string that uniquely identifies a coordinate reference system associated with this type. +To maximize interoperability, suggested formats for CRS include, but are not limited to: +* `<context>:<identifier`: Identifies a CRS by name or other identifier in some well-documented context. Examples: `OGC:CRS84`, `EPSG:4326`, `IGNF:ATI` and `SRID:0` +* `projjson:<property-name>` - where <property-name> refers to a table property where CRS definition in [PROJJSON](https://proj.org/en/stable/specifications/projjson.html) format is stored. Review Comment: This uses `projjson:<property-name>`, but the normative sentence below refers to `projjson:<table_property_name>`. Could we pick one placeholder and use it consistently throughout? -- 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]
