szehon-ho commented on code in PR #12346:
URL: https://github.com/apache/iceberg/pull/12346#discussion_r2003918915
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -543,6 +565,134 @@ public int hashCode() {
}
}
+ public static class GeometryType extends PrimitiveType {
+
+ private final String crs;
+
+ private GeometryType(String crs) {
+ if (crs != null) {
+ Preconditions.checkArgument(!crs.isEmpty(), "Invalid CRS: (empty
string)");
Review Comment:
why the extra parens around 'empty string'?
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -543,6 +565,134 @@ public int hashCode() {
}
}
+ public static class GeometryType extends PrimitiveType {
+
+ private final String crs;
+
+ private GeometryType(String crs) {
+ if (crs != null) {
+ Preconditions.checkArgument(!crs.isEmpty(), "Invalid CRS: (empty
string)");
+ Preconditions.checkArgument(
+ crs.trim().equals(crs), "CRS must not have leading or trailing
spaces: '%s'", crs);
+ }
+ this.crs = crs;
+ }
+
+ public static GeometryType crs84() {
+ return new GeometryType(null);
+ }
+
+ public static GeometryType of(String crs) {
+ return new GeometryType(crs);
+ }
+
+ @Override
+ public TypeID typeId() {
+ return TypeID.GEOMETRY;
+ }
+
+ public String crs() {
+ return crs;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ } else if (!(o instanceof GeometryType)) {
+ return false;
+ }
+
+ GeometryType that = (GeometryType) o;
+ return Objects.equals(crs, that.crs);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(GeometryType.class, crs);
+ }
+
+ @Override
+ public String toString() {
+ if (crs == null) {
+ return "geometry";
+ }
+
+ return String.format("geometry(%s)", crs);
+ }
+ }
+
+ public static class GeographyType extends PrimitiveType {
+
+ public static final String DEFAULT_CRS = "OGC:CRS84";
+
+ private final String crs;
+ private final EdgeAlgorithm algorithm;
+
+ private GeographyType(String crs, EdgeAlgorithm algorithm) {
+ if (crs != null) {
+ Preconditions.checkArgument(!crs.isEmpty(), "Invalid CRS: (empty
string)");
+ Preconditions.checkArgument(
+ crs.trim().equals(crs), "CRS must not have leading or trailing
spaces: '%s'", crs);
+ }
+ this.crs = crs;
Review Comment:
nit: put this in the if block (same for geometry?)
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -543,6 +565,134 @@ public int hashCode() {
}
}
+ public static class GeometryType extends PrimitiveType {
+
+ private final String crs;
+
+ private GeometryType(String crs) {
+ if (crs != null) {
+ Preconditions.checkArgument(!crs.isEmpty(), "Invalid CRS: (empty
string)");
+ Preconditions.checkArgument(
+ crs.trim().equals(crs), "CRS must not have leading or trailing
spaces: '%s'", crs);
+ }
+ this.crs = crs;
+ }
+
+ public static GeometryType crs84() {
+ return new GeometryType(null);
Review Comment:
style: what do you think to have a private GeometryType() constructor that
does nothing? to avoid having null passed in explicitly
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -543,6 +561,120 @@ public int hashCode() {
}
}
+ public static class GeometryType extends PrimitiveType {
+
+ private final String crs;
+
+ private GeometryType(String crs) {
+ this.crs = crs;
Review Comment:
Curious, why not always have toString show the default value like Geography?
Geometry("OGC:CRS84"). Is it to save some bytes ? Seems to be easier imo.
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -543,6 +565,134 @@ public int hashCode() {
}
}
+ public static class GeometryType extends PrimitiveType {
+
+ private final String crs;
+
+ private GeometryType(String crs) {
+ if (crs != null) {
+ Preconditions.checkArgument(!crs.isEmpty(), "Invalid CRS: (empty
string)");
+ Preconditions.checkArgument(
Review Comment:
i might have missed this, but why not just trim it?
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -543,6 +565,134 @@ public int hashCode() {
}
}
+ public static class GeometryType extends PrimitiveType {
+
+ private final String crs;
+
+ private GeometryType(String crs) {
+ if (crs != null) {
+ Preconditions.checkArgument(!crs.isEmpty(), "Invalid CRS: (empty
string)");
+ Preconditions.checkArgument(
+ crs.trim().equals(crs), "CRS must not have leading or trailing
spaces: '%s'", crs);
+ }
+ this.crs = crs;
+ }
+
+ public static GeometryType crs84() {
+ return new GeometryType(null);
+ }
+
+ public static GeometryType of(String crs) {
+ return new GeometryType(crs);
+ }
+
+ @Override
+ public TypeID typeId() {
+ return TypeID.GEOMETRY;
+ }
+
+ public String crs() {
+ return crs;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ } else if (!(o instanceof GeometryType)) {
+ return false;
+ }
+
+ GeometryType that = (GeometryType) o;
+ return Objects.equals(crs, that.crs);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(GeometryType.class, crs);
+ }
+
+ @Override
+ public String toString() {
+ if (crs == null) {
+ return "geometry";
+ }
+
+ return String.format("geometry(%s)", crs);
+ }
+ }
+
+ public static class GeographyType extends PrimitiveType {
+
+ public static final String DEFAULT_CRS = "OGC:CRS84";
+
+ private final String crs;
+ private final EdgeAlgorithm algorithm;
+
+ private GeographyType(String crs, EdgeAlgorithm algorithm) {
+ if (crs != null) {
+ Preconditions.checkArgument(!crs.isEmpty(), "Invalid CRS: (empty
string)");
Review Comment:
same comment, i would remove the parens.
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -543,6 +563,122 @@ public int hashCode() {
}
}
+ public static class GeometryType extends PrimitiveType {
+
+ private final String crs;
+
+ private GeometryType(String crs) {
+ Preconditions.checkNotNull(crs, "CRS cannot be null");
+ this.crs = crs;
+ }
+
+ public static GeometryType get() {
+ return new GeometryType("");
+ }
+
+ public static GeometryType of(String crs) {
+ return new GeometryType(crs == null ? "" : crs);
+ }
+
+ @Override
+ public TypeID typeId() {
+ return TypeID.GEOMETRY;
+ }
+
+ public String crs() {
+ return crs;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ } else if (!(o instanceof GeometryType)) {
+ return false;
+ }
+
+ GeometryType that = (GeometryType) o;
+ return crs.equals(that.crs);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(GeometryType.class, crs);
+ }
+
+ @Override
+ public String toString() {
+ return String.format("geometry(%s)", crs);
+ }
+ }
+
+ public static class GeographyType extends PrimitiveType {
+
+ private final String crs;
+ private final EdgeInterpolationAlgorithm algorithm;
+
+ private GeographyType(String crs, EdgeInterpolationAlgorithm algorithm) {
+ Preconditions.checkNotNull(crs, "CRS cannot be null");
+ this.crs = crs;
+ this.algorithm = algorithm;
+ }
+
+ public static GeographyType get() {
+ return new GeographyType("", null);
+ }
+
+ public static GeographyType of(String crs) {
Review Comment:
if we have two methods (the other one taking crs/algorithm), why not rename
this to forCRS now?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
