szehon-ho commented on code in PR #16765:
URL: https://github.com/apache/iceberg/pull/16765#discussion_r3471482710
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -680,23 +686,18 @@ public boolean equals(Object o) {
}
GeographyType that = (GeographyType) o;
- return Objects.equals(crs, that.crs) && Objects.equals(algorithm,
that.algorithm);
+ // compare the resolved CRS and algorithm so an explicit default is
equal to an omitted one
+ return Objects.equals(crs(), that.crs()) && Objects.equals(algorithm(),
that.algorithm());
}
@Override
public int hashCode() {
- return Objects.hash(GeographyType.class, crs, algorithm);
+ return Objects.hash(GeographyType.class, crs(), algorithm());
}
@Override
public String toString() {
- if (algorithm != null) {
- return String.format("geography(%s, %s)", crs != null ? crs :
DEFAULT_CRS, algorithm);
- } else if (crs != null) {
- return String.format("geography(%s)", crs);
- } else {
- return "geography";
- }
+ return String.format("%s(%s, %s)", NAME, crs, algorithm);
Review Comment:
Same point as `GeometryType.toString()` — use the resolved getters so it
matches `equals`/`hashCode` and produces canonical output. (The CRS-casing case
is the one that actually diverges; `algorithm()` returns the field as-is, but
using it keeps the two `toString()`s uniform.)
```suggestion
return String.format("%s(%s, %s)", NAME, crs(), algorithm());
```
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -610,27 +615,24 @@ public boolean equals(Object o) {
}
GeometryType that = (GeometryType) o;
- return Objects.equals(crs, that.crs);
+ return Objects.equals(crs(), that.crs());
}
@Override
public int hashCode() {
- return Objects.hash(GeometryType.class, crs);
+ return Objects.hash(GeometryType.class, crs());
}
@Override
public String toString() {
- if (crs == null) {
- return "geometry";
- }
-
- return String.format("geometry(%s)", crs);
+ return String.format("%s(%s)", NAME, crs);
Review Comment:
`toString()` formats the raw `crs` field, but `equals`/`hashCode` just above
now compare the resolved `crs()`. These diverge for a default CRS supplied with
non-canonical casing (e.g. `geometry(ogc:crs84)`): the type is `.equals()` to
`crs84()` yet serializes differently (`geometry(ogc:crs84)` vs
`geometry(OGC:CRS84)`). Since `SchemaParser` writes `toString()` straight into
table metadata, the non-canonical form is what gets persisted. It still
round-trips (the parser is case-insensitive), so this is cosmetic, but
formatting from the getter keeps equal types serializing identically and output
canonical:
```suggestion
return String.format("%s(%s)", NAME, crs());
```
--
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]
