Copilot commented on code in PR #11958:
URL: https://github.com/apache/gravitino/pull/11958#discussion_r3555894353
##########
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:
`re.Pattern.match()` only checks a prefix match, so a string like
`geometry(EPSG:4326) trailing` would be accepted and partially parsed as
GeometryType. For type tokens we should require the whole string to match (Java
side uses `Matcher.matches()`).
##########
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:
Same issue as geometry: `re.Pattern.match()` allows trailing characters
after a valid-looking prefix. Use `fullmatch()` (or add `^...$` anchors) so
only a complete `geography(<crs>,<algorithm>)` token parses as GeographyType.
##########
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 test uses fully qualified `org.apache.iceberg.types.Types.*` /
`org.apache.iceberg.types.EdgeAlgorithm` in the new assertions. This appears
avoidable by importing the nested Iceberg types (`Types.GeometryType`,
`Types.GeographyType`, `EdgeAlgorithm`) to keep the test readable (see
AGENTS.md import guidance).
##########
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:
New geometry/geography branches add more fully qualified references to
`org.apache.gravitino.rel.types.Types.*` inside the method body. Per AGENTS.md
imports guidance, these can usually be avoided by importing the nested types
(e.g., `org.apache.gravitino.rel.types.Types.GeometryType`) to keep code
readable even when `Types` conflicts with Iceberg's `Types`.
##########
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:
New geometry/geography assertions introduce multiple fully qualified
`org.apache.gravitino.rel.types.*` references (e.g.,
`org.apache.gravitino.rel.types.Type`,
`org.apache.gravitino.rel.types.Types.GeometryType.class`). Given the existing
`Types` name conflict, consider importing `org.apache.gravitino.rel.types.Type`
plus the nested Gravitino types
(`org.apache.gravitino.rel.types.Types.GeometryType`, `...GeographyType`) to
avoid noisy FQNs (per AGENTS.md import guidance).
--
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]
