nevzheng commented on code in PR #11958:
URL: https://github.com/apache/gravitino/pull/11958#discussion_r3562741292
##########
clients/client-python/gravitino/api/rel/types/json_serdes/_helper/serdes_utils.py:
##########
@@ -177,11 +177,27 @@ def read_data_type(cls, type_data: Json) -> Type:
return Types.UnparsedType.of(unparsed_type=json.dumps(type_data))
@classmethod
- def from_primitive_type_string(cls, type_string: str) -> Type:
+ def from_primitive_type_string(
+ cls, type_string: str, original_type_string: str = None
+ ) -> Type:
type_instance = cls.TYPES.get(type_string)
if type_instance is not None:
return type_instance
+ # Match against the original (non-lowercased) string so the CRS keeps
its case.
+ original = (
+ original_type_string if original_type_string is not None else
type_string
+ )
+ geometry_matched = cls.GEOMETRY_PATTERN.match(original)
+ if geometry_matched:
+ return Types.GeometryType.of(geometry_matched.group(1))
Review Comment:
Good catch — fixed in 375ed7a0c: switched geometry/geography parsing to
`fullmatch()` so a valid-looking prefix with trailing characters can no longer
partially parse, matching the Java side's `Matcher.matches()`. Added a
regression test asserting such strings deserialize to `UnparsedType`.
##########
clients/client-python/gravitino/api/rel/types/json_serdes/_helper/serdes_utils.py:
##########
@@ -177,11 +177,27 @@ def read_data_type(cls, type_data: Json) -> Type:
return Types.UnparsedType.of(unparsed_type=json.dumps(type_data))
@classmethod
- def from_primitive_type_string(cls, type_string: str) -> Type:
+ def from_primitive_type_string(
+ cls, type_string: str, original_type_string: str = None
+ ) -> Type:
type_instance = cls.TYPES.get(type_string)
if type_instance is not None:
return type_instance
+ # Match against the original (non-lowercased) string so the CRS keeps
its case.
+ original = (
+ original_type_string if original_type_string is not None else
type_string
+ )
+ geometry_matched = cls.GEOMETRY_PATTERN.match(original)
+ if geometry_matched:
+ return Types.GeometryType.of(geometry_matched.group(1))
+
+ geography_matched = cls.GEOGRAPHY_PATTERN.match(original)
+ if geography_matched:
+ return Types.GeographyType.of(
+ geography_matched.group(1), geography_matched.group(2)
+ )
Review Comment:
Good catch — fixed in 375ed7a0c: switched geometry/geography parsing to
`fullmatch()` so a valid-looking prefix with trailing characters can no longer
partially parse, matching the Java side's `Matcher.matches()`. Added a
regression test asserting such strings deserialize to `UnparsedType`.
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/converter/ToIcebergType.java:
##########
@@ -164,6 +165,14 @@ public Type
atomic(org.apache.gravitino.rel.types.Type.PrimitiveType primitive)
return Types.UUIDType.get();
} else if (primitive instanceof
org.apache.gravitino.rel.types.Types.VariantType) {
return Types.VariantType.get();
+ } else if (primitive instanceof
org.apache.gravitino.rel.types.Types.GeometryType) {
+ return Types.GeometryType.of(
+ ((org.apache.gravitino.rel.types.Types.GeometryType)
primitive).crs());
+ } else if (primitive instanceof
org.apache.gravitino.rel.types.Types.GeographyType) {
+ org.apache.gravitino.rel.types.Types.GeographyType geography =
Review Comment:
This file deliberately fully-qualifies one `Types` throughout because
`org.apache.iceberg.types.Types` and `org.apache.gravitino.rel.types.Types`
collide — including the nested `GeometryType`/`GeographyType` (and
`BooleanType`, `IntegerType`, …), so both `Types`'-nested classes cannot be
imported together. The new code follows the file's existing FQN convention;
importing only the new nested types would make the same simple name refer to
two different classes on adjacent lines and break local consistency. Keeping
as-is.
##########
catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/integration/test/CatalogIcebergBaseIT.java:
##########
@@ -646,10 +646,18 @@ void testV3TypeConversionViaIcebergClient() {
// like Spark 4 would, directly through the Iceberg catalog, then load
through the native
// metadata interface.
//
- // variant has native support and loads as VariantType. The other V3
net-new types are not
- // modeled in Gravitino's unified type system yet and load as
ExternalType; native support for
- // them is pending and tracked in apache/gravitino#11929.
+ // variant loads as VariantType; geometry and geography load as their
native types. The
+ // remaining V3 net-new types are not modeled in Gravitino's unified type
system yet and load
+ // as ExternalType; native support for them is pending and tracked in
apache/gravitino#11929.
assertV3LoadsAsVariant("v3_variant");
+ assertV3LoadsAsGeometry("v3_geometry",
org.apache.iceberg.types.Types.GeometryType.crs84());
+ assertV3LoadsAsGeometry(
+ "v3_geometry_srid",
org.apache.iceberg.types.Types.GeometryType.of("srid:3857"));
+ assertV3LoadsAsGeography("v3_geography",
org.apache.iceberg.types.Types.GeographyType.crs84());
+ assertV3LoadsAsGeography(
Review Comment:
This file deliberately fully-qualifies one `Types` throughout because
`org.apache.iceberg.types.Types` and `org.apache.gravitino.rel.types.Types`
collide — including the nested `GeometryType`/`GeographyType` (and
`BooleanType`, `IntegerType`, …), so both `Types`'-nested classes cannot be
imported together. The new code follows the file's existing FQN convention;
importing only the new nested types would make the same simple name refer to
two different classes on adjacent lines and break local consistency. Keeping
as-is.
##########
catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/converter/TestConvertUtil.java:
##########
@@ -54,6 +55,47 @@ public void testVariantType() {
instanceof Types.VariantType);
}
+ @Test
+ public void testGeometryType() {
+ // Iceberg V3 geometry <-> Gravitino GeometryType, preserving the CRS in
both directions.
+ org.apache.gravitino.rel.types.Type defaultFromIceberg =
+ CONVERTER.toGravitino(Types.GeometryType.crs84());
+ Assertions.assertInstanceOf(
+ org.apache.gravitino.rel.types.Types.GeometryType.class,
defaultFromIceberg);
+ Assertions.assertEquals(
Review Comment:
This file deliberately fully-qualifies one `Types` throughout because
`org.apache.iceberg.types.Types` and `org.apache.gravitino.rel.types.Types`
collide — including the nested `GeometryType`/`GeographyType` (and
`BooleanType`, `IntegerType`, …), so both `Types`'-nested classes cannot be
imported together. The new code follows the file's existing FQN convention;
importing only the new nested types would make the same simple name refer to
two different classes on adjacent lines and break local consistency. Keeping
as-is.
--
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]
