szehon-ho commented on code in PR #16765:
URL: https://github.com/apache/iceberg/pull/16765#discussion_r3472195384
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -584,12 +585,12 @@ public static GeometryType of(String crs) {
private final String crs;
private GeometryType() {
- crs = null;
+ crs = DEFAULT_CRS;
}
private GeometryType(String crs) {
Preconditions.checkArgument(crs == null || !crs.isEmpty(), "Invalid CRS:
(empty string)");
- this.crs = DEFAULT_CRS.equalsIgnoreCase(crs) ? null : crs;
+ this.crs = crs != null ? crs : DEFAULT_CRS;
Review Comment:
Since the default-CRS match is case-insensitive
(`DEFAULT_CRS.equalsIgnoreCase(...)`), a default column written by another
engine/version in any casing — `geometry`, `geometry(OGC:CRS84)`, or
`geometry(ogc:crs84)` — all resolve to the same default-equal type. That's good
behavior and worth keeping.
Minor suggestion: instead of resolving in `crs()` and routing
`equals`/`hashCode`/`toString` through the getter, could we canonicalize once
in the constructor and keep the stored field canonical?
```java
private GeometryType(String crs) {
Preconditions.checkArgument(crs == null || !crs.isEmpty(), "Invalid CRS:
(empty string)");
this.crs = crs == null || DEFAULT_CRS.equalsIgnoreCase(crs) ? DEFAULT_CRS
: crs;
}
```
Then `crs()` is just `return crs;`, `equals`/`hashCode` compare the field
directly, and `toString` becomes `String.format("%s(%s)", NAME, crs)` — no
getter indirection and the explanatory comments drop out. Same idea for
`GeographyType` (you already resolve the algorithm default in the constructor).
Observable behavior of `crs()`, `equals`, `hashCode`, and `toString` is
identical to the current approach for every input (`null`, `OGC:CRS84`,
`ogc:crs84`, non-default), so the added tests still pass.
--
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]
