rambleraptor commented on code in PR #2859:
URL: https://github.com/apache/iceberg-python/pull/2859#discussion_r2734209165
##########
mkdocs/docs/geospatial.md:
##########
@@ -0,0 +1,110 @@
+# Geospatial Types
Review Comment:
Having type information in the docs is really useful!
##########
mkdocs/docs/geospatial.md:
##########
@@ -0,0 +1,110 @@
+# Geospatial Types
+
+PyIceberg supports Iceberg v3 geospatial primitive types: `geometry` and
`geography`.
+
+## Overview
+
+Iceberg v3 introduces native support for spatial data types:
+
+- **`geometry(C)`**: Represents geometric shapes in a coordinate reference
system (CRS)
+- **`geography(C, A)`**: Represents geographic shapes with CRS and calculation
algorithm
+
+Both types store values as WKB (Well-Known Binary) bytes.
+
+## Requirements
+
+- Iceberg format version 3 or higher
+- `geoarrow-pyarrow` for full GeoArrow extension type support (optional: `pip
install pyiceberg[geoarrow]`)
Review Comment:
I'm a little confused by this sentence. Is `geoarrow-pyarrow` required or
optional?
##########
pyiceberg/types.py:
##########
@@ -61,6 +61,17 @@
FIXED = "fixed"
FIXED_PARSER = ParseNumberFromBrackets(FIXED)
+# Default CRS for geometry and geography types per Iceberg v3 spec
+DEFAULT_GEOMETRY_CRS = "OGC:CRS84"
+DEFAULT_GEOGRAPHY_CRS = "OGC:CRS84"
+DEFAULT_GEOGRAPHY_ALGORITHM = "spherical"
+
+# Regex patterns for parsing geometry and geography type strings
+# Matches: geometry, geometry('CRS'), geometry("CRS")
+GEOMETRY_REGEX = re.compile(r"geometry(?:\(\s*['\"]([^'\"]+)['\"]\s*\))?$")
+# Matches: geography, geography('CRS'), geography('CRS', 'algo')
Review Comment:
nit: same note as above
##########
pyiceberg/types.py:
##########
@@ -61,6 +61,17 @@
FIXED = "fixed"
FIXED_PARSER = ParseNumberFromBrackets(FIXED)
+# Default CRS for geometry and geography types per Iceberg v3 spec
+DEFAULT_GEOMETRY_CRS = "OGC:CRS84"
+DEFAULT_GEOGRAPHY_CRS = "OGC:CRS84"
+DEFAULT_GEOGRAPHY_ALGORITHM = "spherical"
+
+# Regex patterns for parsing geometry and geography type strings
+# Matches: geometry, geometry('CRS'), geometry("CRS")
Review Comment:
nit: can you add `geometry('crs')`?
The regex is permissive and will allow lowercase. Might as well make that
explicit, since regexes are hard to read.
##########
pyiceberg/types.py:
##########
@@ -124,6 +182,13 @@ def handle_primitive_type(cls, v: Any, handler:
ValidatorFunctionWrapHandler) ->
# Pydantic works mostly around dicts, and there seems to be something
# by not serializing into a RootModel, might revisit this.
if isinstance(v, str):
+ # When constructing GeometryType/GeographyType directly with
CRS/algorithm values,
+ # skip the type string parsing to avoid infinite recursion
+ if cls.__name__ == "GeometryType" and not v.startswith("geometry"):
Review Comment:
Can you explain what's going on here? I don't understand how this would
trigger a infinite recursion. None of our other types seem to run into this.
##########
mkdocs/docs/geospatial.md:
##########
@@ -0,0 +1,110 @@
+# Geospatial Types
+
+PyIceberg supports Iceberg v3 geospatial primitive types: `geometry` and
`geography`.
+
+## Overview
+
+Iceberg v3 introduces native support for spatial data types:
+
+- **`geometry(C)`**: Represents geometric shapes in a coordinate reference
system (CRS)
+- **`geography(C, A)`**: Represents geographic shapes with CRS and calculation
algorithm
+
+Both types store values as WKB (Well-Known Binary) bytes.
+
+## Requirements
+
+- Iceberg format version 3 or higher
+- `geoarrow-pyarrow` for full GeoArrow extension type support (optional: `pip
install pyiceberg[geoarrow]`)
Review Comment:
```suggestion
- Optional: `geoarrow-pyarrow` for full GeoArrow extension type support
(`pip install pyiceberg[geoarrow]`)
```
--
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]
