From d9274eedb1291e39e2e78354294764c11ecfb98d Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Thu, 20 Feb 2025 13:23:55 +0800 Subject: [PATCH 01/16] add geometry and geography types to iceberg-api and iceberg-core --- .../java/org/apache/iceberg/Geography.java | 127 +++++++++ .../main/java/org/apache/iceberg/Schema.java | 4 +- .../apache/iceberg/expressions/Literal.java | 10 + .../apache/iceberg/expressions/Literals.java | 65 ++++- .../org/apache/iceberg/types/Conversions.java | 64 +++++ .../java/org/apache/iceberg/types/Type.java | 4 + .../java/org/apache/iceberg/types/Types.java | 139 +++++++++ .../org/apache/iceberg/util/GeometryUtil.java | 221 +++++++++++++++ .../expressions/TestLiteralSerialization.java | 12 + .../TestMiscLiteralConversions.java | 13 +- .../apache/iceberg/types/TestConversions.java | 105 +++++++ .../iceberg/types/TestReadabilityChecks.java | 8 +- .../iceberg/types/TestSerializableTypes.java | 8 +- .../org/apache/iceberg/types/TestTypes.java | 46 +++ .../apache/iceberg/util/TestGeometryUtil.java | 266 ++++++++++++++++++ build.gradle | 1 + .../java/org/apache/iceberg/SchemaParser.java | 34 ++- .../org/apache/iceberg/SingleValueParser.java | 26 ++ .../apache/iceberg/TestGeospatialTable.java | 68 +++++ .../org/apache/iceberg/TestSchemaParser.java | 34 ++- .../org/apache/iceberg/TestSchemaUpdate.java | 6 +- .../apache/iceberg/TestSingleValueParser.java | 19 ++ .../org/apache/iceberg/TestTableMetadata.java | 37 +++ gradle/libs.versions.toml | 2 + 24 files changed, 1311 insertions(+), 8 deletions(-) create mode 100644 api/src/main/java/org/apache/iceberg/Geography.java create mode 100644 api/src/main/java/org/apache/iceberg/util/GeometryUtil.java create mode 100644 api/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java create mode 100644 core/src/test/java/org/apache/iceberg/TestGeospatialTable.java diff --git a/api/src/main/java/org/apache/iceberg/Geography.java b/api/src/main/java/org/apache/iceberg/Geography.java new file mode 100644 index 000000000000..f60183b0e76c --- /dev/null +++ b/api/src/main/java/org/apache/iceberg/Geography.java @@ -0,0 +1,127 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg; + +import java.io.Serializable; +import java.util.Locale; +import java.util.Objects; +import org.locationtech.jts.geom.Geometry; + +/** + * Geospatial features from OGC – Simple feature access. The geometry is on a spherical or + * ellipsoidal surface. An edge-interpolation algorithm is used to evaluate spatial predicates. + */ +public class Geography implements Comparable, Serializable { + + /** The algorithm for interpolating edges. */ + public enum EdgeInterpolationAlgorithm { + /** Edges are interpolated as geodesics on a sphere. */ + SPHERICAL("spherical"), + /** See Vincenty's formulae */ + VINCENTY("vincenty"), + /** + * Thomas, Paul D. Spheroidal geodesics, reference systems, & local geometry. US Naval + * Oceanographic Office, 1970. + */ + THOMAS("thomas"), + /** + * Thomas, Paul D. Mathematical models for navigation systems. US Naval Oceanographic Office, + * 1965. + */ + ANDOYER("andoyer"), + /** + * Karney, Charles + * FF. "Algorithms for geodesics." Journal of Geodesy 87 (2013): 43-55 , and GeographicLib. + */ + KARNEY("karney"); + + private final String value; + + EdgeInterpolationAlgorithm(String value) { + this.value = value; + } + + public String value() { + return value; + } + + public static EdgeInterpolationAlgorithm fromName(String algorithmName) { + try { + return EdgeInterpolationAlgorithm.valueOf(algorithmName.toUpperCase(Locale.ENGLISH)); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException( + String.format("Invalid edge interpolation algorithm name: %s", algorithmName), e); + } + } + } + + private final Geometry geometry; + + public Geography(Geometry geometry) { + this.geometry = geometry; + } + + public Geometry geometry() { + return geometry; + } + + @Override + public String toString() { + return "Geography(" + geometry + ")"; + } + + @Override + public int compareTo(Geography o) { + return geometry.compareTo(o.geometry); + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof Geography)) { + return false; + } + Geography geography = (Geography) o; + return Objects.equals(geometry, geography.geometry); + } + + @Override + public int hashCode() { + return Objects.hashCode(geometry); + } + + public boolean intersects(Geography other, EdgeInterpolationAlgorithm algorithm) { + if (algorithm != EdgeInterpolationAlgorithm.SPHERICAL) { + throw new UnsupportedOperationException( + "Interpolation algorithm other than spherical is not supported yet"); + } + + // TODO: implement a correct spherical intersection algorithm using S2 + return geometry.intersects(other.geometry); + } + + public boolean covers(Geography other, EdgeInterpolationAlgorithm algorithm) { + if (algorithm != EdgeInterpolationAlgorithm.SPHERICAL) { + throw new UnsupportedOperationException( + "Interpolation algorithm other than spherical is not supported yet"); + } + // TODO: implement a correct spherical covers algorithm using S2 + return geometry.covers(other.geometry); + } +} diff --git a/api/src/main/java/org/apache/iceberg/Schema.java b/api/src/main/java/org/apache/iceberg/Schema.java index 07ed44b65cf7..e497b8e69afc 100644 --- a/api/src/main/java/org/apache/iceberg/Schema.java +++ b/api/src/main/java/org/apache/iceberg/Schema.java @@ -63,7 +63,9 @@ public class Schema implements Serializable { ImmutableMap.of( Type.TypeID.TIMESTAMP_NANO, 3, Type.TypeID.VARIANT, 3, - Type.TypeID.UNKNOWN, 3); + Type.TypeID.UNKNOWN, 3, + Type.TypeID.GEOMETRY, 3, + Type.TypeID.GEOGRAPHY, 3); private final StructType struct; private final int schemaId; diff --git a/api/src/main/java/org/apache/iceberg/expressions/Literal.java b/api/src/main/java/org/apache/iceberg/expressions/Literal.java index b5d6f72f74d0..2de5061c0250 100644 --- a/api/src/main/java/org/apache/iceberg/expressions/Literal.java +++ b/api/src/main/java/org/apache/iceberg/expressions/Literal.java @@ -23,7 +23,9 @@ import java.nio.ByteBuffer; import java.util.Comparator; import java.util.UUID; +import org.apache.iceberg.Geography; import org.apache.iceberg.types.Type; +import org.locationtech.jts.geom.Geometry; /** * Represents a literal fixed value in an expression predicate @@ -71,6 +73,14 @@ static Literal of(BigDecimal value) { return new Literals.DecimalLiteral(value); } + static Literal of(Geometry value) { + return new Literals.GeometryLiteral(value); + } + + static Literal of(Geography value) { + return new Literals.GeographyLiteral(value); + } + /** Returns the value wrapped by this literal. */ T value(); diff --git a/api/src/main/java/org/apache/iceberg/expressions/Literals.java b/api/src/main/java/org/apache/iceberg/expressions/Literals.java index ee47035b1e72..b54c4768e10f 100644 --- a/api/src/main/java/org/apache/iceberg/expressions/Literals.java +++ b/api/src/main/java/org/apache/iceberg/expressions/Literals.java @@ -32,6 +32,7 @@ import java.util.Comparator; import java.util.Objects; import java.util.UUID; +import org.apache.iceberg.Geography; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding; import org.apache.iceberg.types.Comparators; @@ -41,6 +42,7 @@ import org.apache.iceberg.util.ByteBuffers; import org.apache.iceberg.util.DateTimeUtil; import org.apache.iceberg.util.NaNUtil; +import org.locationtech.jts.geom.Geometry; class Literals { private Literals() {} @@ -55,7 +57,7 @@ private Literals() {} * @param Java type of value * @return a Literal for the given value */ - @SuppressWarnings("unchecked") + @SuppressWarnings({"unchecked", "checkstyle:CyclomaticComplexity"}) static Literal from(T value) { Preconditions.checkNotNull(value, "Cannot create expression literal from null"); Preconditions.checkArgument(!NaNUtil.isNaN(value), "Cannot create expression literal from NaN"); @@ -80,6 +82,10 @@ static Literal from(T value) { return (Literal) new Literals.BinaryLiteral((ByteBuffer) value); } else if (value instanceof BigDecimal) { return (Literal) new Literals.DecimalLiteral((BigDecimal) value); + } else if (value instanceof Geometry) { + return (Literal) new Literals.GeometryLiteral((Geometry) value); + } else if (value instanceof Geography) { + return (Literal) new Literals.GeographyLiteral((Geography) value); } throw new IllegalArgumentException( @@ -687,4 +693,61 @@ public String toString() { return "X'" + BaseEncoding.base16().encode(bytes) + "'"; } } + + static class GeometryLiteral extends BaseLiteral { + @SuppressWarnings("unchecked") + private static final Comparator CMP = + Comparators.nullsFirst().thenComparing(Comparator.naturalOrder()); + + GeometryLiteral(Geometry value) { + super(value); + } + + @Override + @SuppressWarnings("unchecked") + public Literal to(Type type) { + if (type.typeId() == Type.TypeID.GEOMETRY) { + return (Literal) this; + } + return null; + } + + @Override + public Comparator comparator() { + return CMP; + } + + @Override + protected Type.TypeID typeId() { + return Type.TypeID.GEOMETRY; + } + } + + static class GeographyLiteral extends BaseLiteral { + private static final Comparator CMP = + Comparators.nullsFirst().thenComparing(Comparator.naturalOrder()); + + GeographyLiteral(Geography value) { + super(value); + } + + @Override + @SuppressWarnings("unchecked") + public Literal to(Type type) { + if (type.typeId() == Type.TypeID.GEOGRAPHY) { + return (Literal) this; + } + return null; + } + + @Override + public Comparator comparator() { + return CMP; + } + + @Override + protected Type.TypeID typeId() { + return Type.TypeID.GEOGRAPHY; + } + } } diff --git a/api/src/main/java/org/apache/iceberg/types/Conversions.java b/api/src/main/java/org/apache/iceberg/types/Conversions.java index e18c7b4362e6..4993d04e8eb2 100644 --- a/api/src/main/java/org/apache/iceberg/types/Conversions.java +++ b/api/src/main/java/org/apache/iceberg/types/Conversions.java @@ -29,9 +29,17 @@ import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.UUID; +import org.apache.iceberg.Geography; import org.apache.iceberg.exceptions.RuntimeIOException; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.util.UUIDUtil; +import org.locationtech.jts.geom.Coordinate; +import org.locationtech.jts.geom.CoordinateXY; +import org.locationtech.jts.geom.CoordinateXYM; +import org.locationtech.jts.geom.CoordinateXYZM; +import org.locationtech.jts.geom.Geometry; +import org.locationtech.jts.geom.GeometryFactory; +import org.locationtech.jts.geom.Point; public class Conversions { @@ -39,6 +47,8 @@ private Conversions() {} private static final String HIVE_NULL = "__HIVE_DEFAULT_PARTITION__"; + private static final GeometryFactory FACTORY = new GeometryFactory(); + public static Object fromPartitionString(Type type, String asString) { if (asString == null || HIVE_NULL.equals(asString)) { return null; @@ -117,6 +127,10 @@ public static ByteBuffer toByteBuffer(Type.TypeID typeId, Object value) { return (ByteBuffer) value; case DECIMAL: return ByteBuffer.wrap(((BigDecimal) value).unscaledValue().toByteArray()); + case GEOMETRY: + return geometryToByteBuffer((Geometry) value); + case GEOGRAPHY: + return geometryToByteBuffer(((Geography) value).geometry()); default: throw new UnsupportedOperationException("Cannot serialize type: " + typeId); } @@ -177,8 +191,58 @@ private static Object internalFromByteBuffer(Type type, ByteBuffer buffer) { byte[] unscaledBytes = new byte[buffer.remaining()]; tmp.get(unscaledBytes); return new BigDecimal(new BigInteger(unscaledBytes), decimal.scale()); + case GEOMETRY: + case GEOGRAPHY: + Coordinate coordinate = coordinateFromByteBuffer(tmp); + Geometry geometry = FACTORY.createPoint(coordinate); + if (type.typeId() == Type.TypeID.GEOMETRY) { + return geometry; + } else { + return new Geography(geometry); + } default: throw new UnsupportedOperationException("Cannot deserialize type: " + type); } } + + private static ByteBuffer geometryToByteBuffer(Geometry value) { + if (value instanceof Point) { + Coordinate coordinate = value.getCoordinate(); + return coordinateToByteBuffer(coordinate); + } else { + throw new IllegalArgumentException("Only point geometry can be converted to byte buffer"); + } + } + + private static ByteBuffer coordinateToByteBuffer(Coordinate coordinate) { + // The getZ() and getM() for a coordinate will return NaN if the value is not set. + // This is conformant with the Bound Serialization spec. + // See https://iceberg.apache.org/spec/#bound-serialization + return ByteBuffer.allocate(32) + .order(ByteOrder.LITTLE_ENDIAN) + .putDouble(0, coordinate.getX()) + .putDouble(8, coordinate.getY()) + .putDouble(16, coordinate.getZ()) + .putDouble(24, coordinate.getM()); + } + + private static Coordinate coordinateFromByteBuffer(ByteBuffer tmp) { + double coordX = tmp.getDouble(0); + double coordY = tmp.getDouble(8); + double coordZ = tmp.getDouble(16); + double coordM = tmp.getDouble(24); + boolean hasZ = !Double.isNaN(coordZ); + boolean hasM = !Double.isNaN(coordM); + Coordinate coordinate; + if (hasZ && hasM) { + coordinate = new CoordinateXYZM(coordX, coordY, coordZ, coordM); + } else if (hasZ) { + coordinate = new Coordinate(coordX, coordY, coordZ); + } else if (hasM) { + coordinate = new CoordinateXYM(coordX, coordY, coordM); + } else { + coordinate = new CoordinateXY(coordX, coordY); + } + return coordinate; + } } diff --git a/api/src/main/java/org/apache/iceberg/types/Type.java b/api/src/main/java/org/apache/iceberg/types/Type.java index 184a17416eae..d221a2455f7b 100644 --- a/api/src/main/java/org/apache/iceberg/types/Type.java +++ b/api/src/main/java/org/apache/iceberg/types/Type.java @@ -25,8 +25,10 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import org.apache.iceberg.Geography; import org.apache.iceberg.StructLike; import org.apache.iceberg.variants.Variant; +import org.locationtech.jts.geom.Geometry; public interface Type extends Serializable { enum TypeID { @@ -44,6 +46,8 @@ enum TypeID { FIXED(ByteBuffer.class), BINARY(ByteBuffer.class), DECIMAL(BigDecimal.class), + GEOMETRY(Geometry.class), + GEOGRAPHY(Geography.class), STRUCT(StructLike.class), LIST(List.class), MAP(Map.class), diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index a866a31ea005..29fcbf92a5fc 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -26,6 +26,7 @@ import java.util.Objects; import java.util.regex.Matcher; import java.util.regex.Pattern; +import org.apache.iceberg.Geography; import org.apache.iceberg.Schema; import org.apache.iceberg.expressions.Expressions; import org.apache.iceberg.expressions.Literal; @@ -61,6 +62,10 @@ private Types() {} .buildOrThrow(); private static final Pattern FIXED = Pattern.compile("fixed\\[\\s*(\\d+)\\s*\\]"); + private static final Pattern GEOMETRY_PARAMETERS = + Pattern.compile("(?:\\(\\s*([^, ]+)?\\s*\\))?"); + private static final Pattern GEOGRAPHY_PARAMETERS = + Pattern.compile("(?:\\(\\s*([^, ]+)?\\s*(?:,\\s*(\\w+)\\s*)?\\))?"); private static final Pattern DECIMAL = Pattern.compile("decimal\\(\\s*(\\d+)\\s*,\\s*(\\d+)\\s*\\)"); @@ -70,6 +75,20 @@ public static Type fromTypeName(String typeString) { return TYPES.get(lowerTypeString); } + if (lowerTypeString.startsWith("geometry")) { + Matcher geometry = GEOMETRY_PARAMETERS.matcher(typeString.substring(8)); + if (geometry.matches()) { + return GeometryType.of(geometry.group(1)); + } + } + + if (lowerTypeString.startsWith("geography")) { + Matcher geography = GEOGRAPHY_PARAMETERS.matcher(typeString.substring(9)); + if (geography.matches()) { + return GeographyType.of(geography.group(1), geography.group(2)); + } + } + Matcher fixed = FIXED.matcher(lowerTypeString); if (fixed.matches()) { return FixedType.ofLength(Integer.parseInt(fixed.group(1))); @@ -543,6 +562,126 @@ public int hashCode() { } } + public static class GeometryType extends PrimitiveType { + + public static final String DEFAULT_CRS = "OGC:CRS84"; + + private final String crs; + + private GeometryType(String crs) { + this.crs = crs; + } + + public static GeometryType get() { + return of(DEFAULT_CRS); + } + + public static GeometryType of(String crs) { + return new GeometryType(crs == null ? DEFAULT_CRS : 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 { + + public static final String DEFAULT_CRS = "OGC:CRS84"; + public static final Geography.EdgeInterpolationAlgorithm DEFAULT_ALGORITHM = + Geography.EdgeInterpolationAlgorithm.SPHERICAL; + + private final String crs; + private final Geography.EdgeInterpolationAlgorithm algorithm; + + private GeographyType(String crs, Geography.EdgeInterpolationAlgorithm algorithm) { + this.crs = crs; + this.algorithm = algorithm; + } + + public static GeographyType get() { + return of(DEFAULT_CRS); + } + + public static GeographyType of(String crs) { + return of(crs, DEFAULT_ALGORITHM); + } + + public static GeographyType of(String crs, Geography.EdgeInterpolationAlgorithm algorithm) { + return new GeographyType(crs, algorithm); + } + + public static GeographyType of(String crs, String algorithmName) { + Geography.EdgeInterpolationAlgorithm algorithm = + (algorithmName == null + ? DEFAULT_ALGORITHM + : Geography.EdgeInterpolationAlgorithm.fromName(algorithmName)); + return new GeographyType(crs == null ? DEFAULT_CRS : crs, algorithm); + } + + @Override + public TypeID typeId() { + return TypeID.GEOGRAPHY; + } + + public String crs() { + return crs; + } + + public Geography.EdgeInterpolationAlgorithm algorithm() { + return algorithm; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } else if (!(o instanceof GeographyType)) { + return false; + } + + GeographyType that = (GeographyType) o; + return crs.equals(that.crs) && algorithm.equals(that.algorithm); + } + + @Override + public int hashCode() { + return Objects.hash(GeographyType.class, crs, algorithm); + } + + @Override + public String toString() { + return String.format("geography(%s, %s)", crs, algorithm.value()); + } + } + public static class NestedField implements Serializable { public static NestedField optional(int id, String name, Type type) { return new NestedField(true, id, name, type, null, null, null); diff --git a/api/src/main/java/org/apache/iceberg/util/GeometryUtil.java b/api/src/main/java/org/apache/iceberg/util/GeometryUtil.java new file mode 100644 index 000000000000..e161605d12a8 --- /dev/null +++ b/api/src/main/java/org/apache/iceberg/util/GeometryUtil.java @@ -0,0 +1,221 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.util; + +import org.apache.iceberg.Geography; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.locationtech.jts.geom.Coordinate; +import org.locationtech.jts.geom.Envelope; +import org.locationtech.jts.geom.Geometry; +import org.locationtech.jts.geom.GeometryFactory; +import org.locationtech.jts.geom.Point; +import org.locationtech.jts.io.WKBReader; +import org.locationtech.jts.io.WKBWriter; +import org.locationtech.jts.io.WKTReader; +import org.locationtech.jts.io.WKTWriter; + +public class GeometryUtil { + + private GeometryUtil() {} + + private static final GeometryFactory FACTORY = new GeometryFactory(); + + public static byte[] toWKB(Geometry geom) { + WKBWriter wkbWriter = new WKBWriter(getOutputDimension(geom), false); + return wkbWriter.write(geom); + } + + public static String toWKT(Geometry geom) { + WKTWriter wktWriter = new WKTWriter(getOutputDimension(geom)); + return wktWriter.write(geom); + } + + public static Geometry fromWKB(byte[] wkb) { + WKBReader reader = new WKBReader(); + try { + return reader.read(wkb); + } catch (Exception e) { + throw new IllegalArgumentException("Failed to parse WKB", e); + } + } + + public static Geometry fromWKT(String wkt) { + WKTReader reader = new WKTReader(); + try { + return reader.read(wkt); + } catch (Exception e) { + throw new IllegalArgumentException("Failed to parse WKT", e); + } + } + + public static int getOutputDimension(Geometry geom) { + int dimension = 2; + Coordinate coordinate = geom.getCoordinate(); + + // We need to set outputDimension = 4 for XYM geometries to make JTS WKTWriter or WKBWriter work + // correctly. + // The WKB/WKT writers will ignore Z ordinate for XYM geometries. + if (!Double.isNaN(coordinate.getZ())) { + dimension = 3; + } + if (!Double.isNaN(coordinate.getM())) { + dimension = 4; + } + return dimension; + } + + /** + * Check if the geometry may intersect with the given bound. The bound represents a rectangle + * crossing the anti-meridian when the x of lower bound is greater than the x of upper bound. + * + * @param lowerBound The lower-left point of the bound + * @param upperBound The upper-right point of the bound + * @param geom The geometry to check + * @return true if the geometry may intersect with the bound; false if the geometry definitely + * does not intersect with the bound + */ + public static boolean boundMayIntersects( + Geometry lowerBound, Geometry upperBound, Geometry geom) { + Preconditions.checkArgument(lowerBound instanceof Point, "Lower bound must be a point"); + Preconditions.checkArgument(upperBound instanceof Point, "Upper bound must be a point"); + + Coordinate lowerCoordinate = lowerBound.getCoordinate(); + Coordinate upperCoordinate = upperBound.getCoordinate(); + if (lowerCoordinate.x <= upperCoordinate.x) { + // Not crossing the anti-meridian + Envelope envelope = new Envelope(lowerBound.getCoordinate(), upperBound.getCoordinate()); + return geom.intersects(FACTORY.toGeometry(envelope)); + } else { + // Crossing the anti-meridian. Use the envelope of geom to evaluate the intersection with + // false positives + Envelope envelope = geom.getEnvelopeInternal(); + if (envelope.getMinY() > upperCoordinate.y || envelope.getMaxY() < lowerCoordinate.y) { + return false; + } + return (envelope.getMinX() <= upperCoordinate.x || envelope.getMaxX() >= lowerCoordinate.x); + } + } + + /** + * Check if the geography may intersect with the given bound. The bound represents a rectangle + * crossing the anti-meridian when the x of lower bound is greater than the x of upper bound. + * + * @param lowerBound The lower-left point of the bound + * @param upperBound The upper-right point of the bound + * @param geog The geography to check + * @return true if the geography may intersect with the bound; false if the geography definitely + * does not intersect with the bound + */ + public static boolean boundMayIntersects( + Geography lowerBound, Geography upperBound, Geography geog) { + // TODO: implement a correct spherical intersection algorithm + return boundMayIntersects(lowerBound.geometry(), upperBound.geometry(), geog.geometry()); + } + + /** + * Check if the bound may cover the geometry. The bound represents a rectangle crossing the + * anti-meridian when the x of lower bound is greater than the x of upper bound. + * + * @param lowerBound The lower-left point of the bound + * @param upperBound The upper-right point of the bound + * @param geom The geometry to check + * @return true if the bound may cover the geometry; false if the bound definitely does not cover + * the geometry + */ + public static boolean boundMayCovers(Geometry lowerBound, Geometry upperBound, Geometry geom) { + Preconditions.checkArgument(lowerBound instanceof Point, "Lower bound must be a point"); + Preconditions.checkArgument(upperBound instanceof Point, "Upper bound must be a point"); + + Coordinate lowerCoordinate = lowerBound.getCoordinate(); + Coordinate upperCoordinate = upperBound.getCoordinate(); + if (lowerCoordinate.x <= upperCoordinate.x) { + // Not crossing the anti-meridian + Envelope envelope = new Envelope(lowerBound.getCoordinate(), upperBound.getCoordinate()); + return FACTORY.toGeometry(envelope).covers(geom); + } else { + // Crossing the anti-meridian. Use the envelope of geom to evaluate the covers with false + // positives + Envelope envelope = geom.getEnvelopeInternal(); + if (envelope.getMinY() < lowerCoordinate.y || envelope.getMaxY() > upperCoordinate.y) { + return false; + } + return (envelope.getMaxX() <= upperCoordinate.x || envelope.getMinX() >= lowerCoordinate.x); + } + } + + /** + * Check if the bound may cover the geography. The bound represents a rectangle crossing the + * anti-meridian when the x of lower bound is greater than the x of upper bound. + * + * @param lowerBound The lower-left point of the bound + * @param upperBound The upper-right point of the bound + * @param geog The geography to check + * @return true if the bound may cover the geography; false if the bound definitely does not cover + * the geography + */ + public static boolean boundMayCovers(Geography lowerBound, Geography upperBound, Geography geog) { + // TODO: implement a correct spherical covers algorithm + return boundMayCovers(lowerBound.geometry(), upperBound.geometry(), geog.geometry()); + } + + /** + * Check if we are sure that the bound must be covered by the geometry. The bound represents a + * rectangle crossing the anti-meridian when the x of lower bound is greater than the x of upper + * bound. + * + * @param lowerBound The lower-left point of the bound + * @param upperBound The upper-right point of the bound + * @param geom The geometry to check + * @return true if the bound is definitely covered by the geometry; false if the bound may or may + * not cover the geometry + */ + public static boolean boundMustBeCoveredBy( + Geometry lowerBound, Geometry upperBound, Geometry geom) { + Preconditions.checkArgument(lowerBound instanceof Point, "Lower bound must be a point"); + Preconditions.checkArgument(upperBound instanceof Point, "Upper bound must be a point"); + + Coordinate lowerCoordinate = lowerBound.getCoordinate(); + Coordinate upperCoordinate = upperBound.getCoordinate(); + if (lowerCoordinate.x <= upperCoordinate.x) { + // Not crossing the anti-meridian + Envelope envelope = new Envelope(lowerBound.getCoordinate(), upperBound.getCoordinate()); + return FACTORY.toGeometry(envelope).coveredBy(geom); + } else { + // Crossing the anti-meridian. This case can be tricky so we always return false to be safe. + return false; + } + } + + /** + * Check if we are sure that the bound must be covered by the geography. The bound represents a + * rectangle crossing the anti-meridian when the x of lower bound is greater than the x of upper + * bound. + * + * @param lowerBound The lower-left point of the bound + * @param upperBound The upper-right point of the bound + * @param geog The geography to check + * @return true if the bound is definitely covered by the geography; false if the bound may or may + * not cover the geography + */ + public static boolean boundMustBeCoveredBy( + Geography lowerBound, Geography upperBound, Geography geog) { + // TODO: implement a correct spherical covered-by algorithm + return boundMustBeCoveredBy(lowerBound.geometry(), upperBound.geometry(), geog.geometry()); + } +} diff --git a/api/src/test/java/org/apache/iceberg/expressions/TestLiteralSerialization.java b/api/src/test/java/org/apache/iceberg/expressions/TestLiteralSerialization.java index 24fc458b37b4..18b9ba3abd58 100644 --- a/api/src/test/java/org/apache/iceberg/expressions/TestLiteralSerialization.java +++ b/api/src/test/java/org/apache/iceberg/expressions/TestLiteralSerialization.java @@ -25,10 +25,16 @@ import org.apache.iceberg.TestHelpers; import org.apache.iceberg.types.Types; import org.junit.jupiter.api.Test; +import org.locationtech.jts.geom.Coordinate; +import org.locationtech.jts.geom.CoordinateXY; +import org.locationtech.jts.geom.CoordinateXYM; +import org.locationtech.jts.geom.CoordinateXYZM; +import org.locationtech.jts.geom.GeometryFactory; public class TestLiteralSerialization { @Test public void testLiterals() throws Exception { + GeometryFactory factory = new GeometryFactory(); Literal[] literals = new Literal[] { Literal.of(false), @@ -47,6 +53,12 @@ public void testLiterals() throws Exception { Literal.of(new byte[] {1, 2, 3}).to(Types.FixedType.ofLength(3)), Literal.of(new byte[] {3, 4, 5, 6}).to(Types.BinaryType.get()), Literal.of(new BigDecimal("122.50")), + Literal.of(factory.createPoint()), + Literal.of(factory.createPoint(new CoordinateXY(10, 20))), + Literal.of(factory.createPoint(new Coordinate(10, 20))), + Literal.of(factory.createPoint(new Coordinate(10, 20, 30))), + Literal.of(factory.createPoint(new CoordinateXYM(10, 20, 30))), + Literal.of(factory.createPoint(new CoordinateXYZM(10, 20, 30, 40))) }; for (Literal lit : literals) { diff --git a/api/src/test/java/org/apache/iceberg/expressions/TestMiscLiteralConversions.java b/api/src/test/java/org/apache/iceberg/expressions/TestMiscLiteralConversions.java index e2611ddb281f..3d2419ffca04 100644 --- a/api/src/test/java/org/apache/iceberg/expressions/TestMiscLiteralConversions.java +++ b/api/src/test/java/org/apache/iceberg/expressions/TestMiscLiteralConversions.java @@ -25,11 +25,16 @@ import java.util.Arrays; import java.util.List; import java.util.UUID; +import org.apache.iceberg.Geography; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.junit.jupiter.api.Test; +import org.locationtech.jts.geom.Envelope; +import org.locationtech.jts.geom.GeometryFactory; public class TestMiscLiteralConversions { + private static final GeometryFactory FACTORY = new GeometryFactory(); + @Test public void testIdentityConversions() { List, Type>> pairs = @@ -48,7 +53,13 @@ public void testIdentityConversions() { Pair.of(Literal.of("abc"), Types.StringType.get()), Pair.of(Literal.of(UUID.randomUUID()), Types.UUIDType.get()), Pair.of(Literal.of(new byte[] {0, 1, 2}), Types.FixedType.ofLength(3)), - Pair.of(Literal.of(ByteBuffer.wrap(new byte[] {0, 1, 2})), Types.BinaryType.get())); + Pair.of(Literal.of(ByteBuffer.wrap(new byte[] {0, 1, 2})), Types.BinaryType.get()), + Pair.of( + Literal.of(FACTORY.toGeometry(new Envelope(1, 2, 10, 20))), + Types.GeometryType.get()), + Pair.of( + Literal.of(new Geography(FACTORY.toGeometry(new Envelope(1, 2, 10, 20)))), + Types.GeographyType.get())); for (Pair, Type> pair : pairs) { Literal lit = pair.first(); diff --git a/api/src/test/java/org/apache/iceberg/types/TestConversions.java b/api/src/test/java/org/apache/iceberg/types/TestConversions.java index e207cfd8d59a..6f74e426b798 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestConversions.java +++ b/api/src/test/java/org/apache/iceberg/types/TestConversions.java @@ -25,6 +25,7 @@ import java.nio.CharBuffer; import java.nio.charset.StandardCharsets; import java.util.UUID; +import org.apache.iceberg.Geography; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.types.Types.BinaryType; import org.apache.iceberg.types.Types.BooleanType; @@ -41,6 +42,12 @@ import org.apache.iceberg.types.Types.TimestampType; import org.apache.iceberg.types.Types.UUIDType; import org.junit.jupiter.api.Test; +import org.locationtech.jts.geom.Coordinate; +import org.locationtech.jts.geom.CoordinateXY; +import org.locationtech.jts.geom.CoordinateXYM; +import org.locationtech.jts.geom.CoordinateXYZM; +import org.locationtech.jts.geom.Geometry; +import org.locationtech.jts.geom.GeometryFactory; public class TestConversions { @@ -191,6 +198,104 @@ public void testByteBufferConversions() { .isEqualTo(new byte[] {11}); } + @Test + public void testByteBufferConversionsForGeometryType() { + // geometry lower/upper boundaries are stored as 4 8-bytes floating point numbers in little + // endian. + // The 4 components are [x, y, optional z, optional m]. If z and m are not present, NaN is + // filled in. + GeometryFactory factory = new GeometryFactory(); + Geometry pointXY = factory.createPoint(new CoordinateXY(10, 20)); + assertConversion( + pointXY, + Types.GeometryType.get(), + new byte[] { + 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, + 0, 0, -8, 127 + }); + pointXY = factory.createPoint(new Coordinate(10, 20)); + assertConversion( + pointXY, + Types.GeometryType.get(), + new byte[] { + 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, + 0, 0, -8, 127 + }); + Geometry pointXYZ = factory.createPoint(new Coordinate(10, 20, 30)); + assertConversion( + pointXYZ, + Types.GeometryType.get(), + new byte[] { + 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, 62, 64, 0, 0, 0, 0, + 0, 0, -8, 127 + }); + Geometry pointXYM = factory.createPoint(new CoordinateXYM(10, 20, 30)); + assertConversion( + pointXYM, + Types.GeometryType.get(), + new byte[] { + 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, + 0, 0, 62, 64 + }); + Geometry pointXYZM = factory.createPoint(new CoordinateXYZM(10, 20, 30, 40)); + assertConversion( + pointXYZM, + Types.GeometryType.get(), + new byte[] { + 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, 62, 64, 0, 0, 0, 0, + 0, 0, 68, 64 + }); + } + + @Test + public void testByteBufferConversionsForGeographyType() { + // geography lower/upper boundaries are stored as 4 8-bytes floating point numbers in little + // endian. This is the same as geometry lower/upper boundaries. + // The 4 components are [x, y, optional z, optional m]. If z and m are not present, NaN is + // filled in. + GeometryFactory factory = new GeometryFactory(); + Geography pointXY = new Geography(factory.createPoint(new CoordinateXY(10, 20))); + assertConversion( + pointXY, + Types.GeographyType.get(), + new byte[] { + 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, + 0, 0, -8, 127 + }); + pointXY = new Geography(factory.createPoint(new Coordinate(10, 20))); + assertConversion( + pointXY, + Types.GeographyType.get(), + new byte[] { + 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, + 0, 0, -8, 127 + }); + Geography pointXYZ = new Geography(factory.createPoint(new Coordinate(10, 20, 30))); + assertConversion( + pointXYZ, + Types.GeographyType.get(), + new byte[] { + 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, 62, 64, 0, 0, 0, 0, + 0, 0, -8, 127 + }); + Geography pointXYM = new Geography(factory.createPoint(new CoordinateXYM(10, 20, 30))); + assertConversion( + pointXYM, + Types.GeographyType.get(), + new byte[] { + 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, + 0, 0, 62, 64 + }); + Geography pointXYZM = new Geography(factory.createPoint(new CoordinateXYZM(10, 20, 30, 40))); + assertConversion( + pointXYZM, + Types.GeographyType.get(), + new byte[] { + 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, 62, 64, 0, 0, 0, 0, + 0, 0, 68, 64 + }); + } + private void assertConversion(T value, Type type, byte[] expectedBinary) { ByteBuffer byteBuffer = Conversions.toByteBuffer(type, value); assertThat(byteBuffer.array()).isEqualTo(expectedBinary); diff --git a/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java b/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java index debb9c9dc1d6..e6984b2a20aa 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java +++ b/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java @@ -25,6 +25,7 @@ import java.util.Arrays; import java.util.List; import java.util.stream.Stream; +import org.apache.iceberg.Geography; import org.apache.iceberg.Schema; import org.apache.iceberg.types.Type.PrimitiveType; import org.junit.jupiter.api.Test; @@ -53,7 +54,12 @@ public class TestReadabilityChecks { Types.BinaryType.get(), Types.DecimalType.of(9, 2), Types.DecimalType.of(11, 2), - Types.DecimalType.of(9, 3) + Types.DecimalType.of(9, 3), + Types.GeometryType.get(), + Types.GeometryType.of("srid:3857"), + Types.GeographyType.get(), + Types.GeographyType.of("srid:4269"), + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY), }; @Test diff --git a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java index 790f59587c59..7abc90448244 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java @@ -22,6 +22,7 @@ import static org.apache.iceberg.types.Types.NestedField.required; import static org.assertj.core.api.Assertions.assertThat; +import org.apache.iceberg.Geography; import org.apache.iceberg.Schema; import org.apache.iceberg.TestHelpers; import org.junit.jupiter.api.Test; @@ -63,7 +64,12 @@ public void testEqualTypes() throws Exception { Types.DecimalType.of(9, 3), Types.DecimalType.of(11, 0), Types.FixedType.ofLength(4), - Types.FixedType.ofLength(34) + Types.FixedType.ofLength(34), + Types.GeometryType.get(), + Types.GeometryType.of("srid:3857"), + Types.GeographyType.get(), + Types.GeographyType.of("srid:4269"), + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY), }; for (Type type : equalityPrimitives) { diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypes.java b/api/src/test/java/org/apache/iceberg/types/TestTypes.java index f8ee4e2ccbd4..90e02baa90dc 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypes.java @@ -23,6 +23,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import org.apache.iceberg.Geography; import org.junit.jupiter.api.Test; public class TestTypes { @@ -90,5 +91,50 @@ public void testNestedFieldBuilderIdCheck() { assertThatExceptionOfType(NullPointerException.class) .isThrownBy(() -> required("field").ofType(Types.StringType.get()).build()) .withMessage("Id cannot be null"); + + assertThat(Types.fromPrimitiveString("geometry")).isEqualTo(Types.GeometryType.get()); + assertThat(Types.fromPrimitiveString("geometry()")).isEqualTo(Types.GeometryType.get()); + assertThat(Types.fromPrimitiveString("geometry(srid:3857)")) + .isEqualTo(Types.GeometryType.of("srid:3857")); + assertThat(Types.fromPrimitiveString("geometry( srid:3857 )")) + .isEqualTo(Types.GeometryType.of("srid:3857")); + + assertThat(Types.fromPrimitiveString("geography")).isEqualTo(Types.GeographyType.get()); + assertThat(Types.fromPrimitiveString("geography()")).isEqualTo(Types.GeographyType.get()); + assertThat(Types.fromPrimitiveString("geography(srid:4269)")) + .isEqualTo(Types.GeographyType.of("srid:4269")); + assertThat(Types.fromPrimitiveString("geography(srid:4269, spherical)")) + .isEqualTo( + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.SPHERICAL)); + assertThat(Types.fromPrimitiveString("geography(srid:4269, vincenty)")) + .isEqualTo( + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.VINCENTY)); + assertThat(Types.fromPrimitiveString("geography(srid:4269, thomas)")) + .isEqualTo( + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.THOMAS)); + assertThat(Types.fromPrimitiveString("geography(srid:4269, andoyer)")) + .isEqualTo( + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.ANDOYER)); + assertThat(Types.fromPrimitiveString("geography(srid:4269, karney)")) + .isEqualTo( + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY)); + + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> Types.fromPrimitiveString("geography(srid:4269, BadAlgorithm)")) + .withMessageContaining("Invalid edge interpolation algorithm name") + .withMessageContaining("BadAlgorithm"); + + // Test geography type with various spacing + assertThat(Types.fromPrimitiveString("geography( srid:4269 )")) + .isEqualTo(Types.GeographyType.of("srid:4269")); + assertThat(Types.fromPrimitiveString("geography( srid:4269 , spherical )")) + .isEqualTo( + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.SPHERICAL)); + assertThat(Types.fromPrimitiveString("geography(srid:4269,vincenty)")) + .isEqualTo( + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.VINCENTY)); + assertThat(Types.fromPrimitiveString("geography( srid:4269 , karney )")) + .isEqualTo( + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY)); } } diff --git a/api/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java b/api/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java new file mode 100644 index 000000000000..6dfb8d9e5023 --- /dev/null +++ b/api/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java @@ -0,0 +1,266 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.util; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; +import org.locationtech.jts.geom.Coordinate; +import org.locationtech.jts.geom.CoordinateXY; +import org.locationtech.jts.geom.CoordinateXYM; +import org.locationtech.jts.geom.CoordinateXYZM; +import org.locationtech.jts.geom.Envelope; +import org.locationtech.jts.geom.Geometry; +import org.locationtech.jts.geom.GeometryFactory; +import org.locationtech.jts.geom.Point; + +public class TestGeometryUtil { + private static final GeometryFactory FACTORY = new GeometryFactory(); + + @Test + public void testToWKB() { + Geometry geometry = FACTORY.createPoint(new Coordinate(1.0, 2.0)); + byte[] wkb = GeometryUtil.toWKB(geometry); + Geometry readGeometry = GeometryUtil.fromWKB(wkb); + assertThat(geometry).isEqualTo(readGeometry); + Coordinate coordinate = readGeometry.getCoordinate(); + assertThat(coordinate.getZ()).isNaN(); + assertThat(coordinate.getM()).isNaN(); + } + + @Test + public void testXYToWKB() { + Geometry geometry = FACTORY.createPoint(new CoordinateXY(1.0, 2.0)); + byte[] wkb = GeometryUtil.toWKB(geometry); + Geometry readGeometry = GeometryUtil.fromWKB(wkb); + assertThat(geometry).isEqualTo(readGeometry); + Coordinate coordinate = readGeometry.getCoordinate(); + assertThat(coordinate.getZ()).isNaN(); + assertThat(coordinate.getM()).isNaN(); + } + + @Test + public void testXYZToWKB() { + Geometry geometry = FACTORY.createPoint(new Coordinate(1.0, 2.0, 3.0)); + byte[] wkb = GeometryUtil.toWKB(geometry); + Geometry readGeometry = GeometryUtil.fromWKB(wkb); + assertThat(geometry).isEqualTo(readGeometry); + Coordinate coordinate = readGeometry.getCoordinate(); + assertThat(coordinate.getZ()).isEqualTo(3.0); + assertThat(coordinate.getM()).isNaN(); + } + + @Test + @Disabled("https://github.com/locationtech/jts/issues/733") + public void testXYMToWKB() { + Geometry geometry = FACTORY.createPoint(new CoordinateXYM(1.0, 2.0, 3.0)); + byte[] wkb = GeometryUtil.toWKB(geometry); + Geometry readGeometry = GeometryUtil.fromWKB(wkb); + assertThat(geometry).isEqualTo(readGeometry); + Coordinate coordinate = readGeometry.getCoordinate(); + assertThat(coordinate.getZ()).isNaN(); + assertThat(coordinate.getM()).isEqualTo(3.0); + } + + @Test + public void testXYZMToWKB() { + Geometry geometry = FACTORY.createPoint(new CoordinateXYZM(1.0, 2.0, 3.0, 4.0)); + byte[] wkb = GeometryUtil.toWKB(geometry); + Geometry readGeometry = GeometryUtil.fromWKB(wkb); + assertThat(geometry).isEqualTo(readGeometry); + Coordinate coordinate = readGeometry.getCoordinate(); + assertThat(coordinate.getZ()).isEqualTo(3.0); + assertThat(coordinate.getM()).isEqualTo(4.0); + } + + @Test + public void testToWKT() { + Geometry geometry = FACTORY.createPoint(new Coordinate(1.0, 2.0)); + String wkt = GeometryUtil.toWKT(geometry); + Geometry readGeometry = GeometryUtil.fromWKT(wkt); + assertThat(geometry).isEqualTo(readGeometry); + Coordinate coordinate = readGeometry.getCoordinate(); + assertThat(coordinate.getZ()).isNaN(); + assertThat(coordinate.getM()).isNaN(); + } + + @Test + public void testXYToWKT() { + Geometry geometry = FACTORY.createPoint(new CoordinateXY(1.0, 2.0)); + String wkt = GeometryUtil.toWKT(geometry); + Geometry readGeometry = GeometryUtil.fromWKT(wkt); + assertThat(geometry).isEqualTo(readGeometry); + Coordinate coordinate = readGeometry.getCoordinate(); + assertThat(coordinate.getZ()).isNaN(); + assertThat(coordinate.getM()).isNaN(); + } + + @Test + public void testXYZToWKT() { + Geometry geometry = FACTORY.createPoint(new Coordinate(1.0, 2.0, 3.0)); + String wkt = GeometryUtil.toWKT(geometry); + Geometry readGeometry = GeometryUtil.fromWKT(wkt); + assertThat(geometry).isEqualTo(readGeometry); + Coordinate coordinate = readGeometry.getCoordinate(); + assertThat(coordinate.getZ()).isEqualTo(3.0); + assertThat(coordinate.getM()).isNaN(); + } + + @Test + public void testXYMToWKT() { + Geometry geometry = FACTORY.createPoint(new CoordinateXYM(1.0, 2.0, 3.0)); + String wkt = GeometryUtil.toWKT(geometry); + Geometry readGeometry = GeometryUtil.fromWKT(wkt); + assertThat(geometry).isEqualTo(readGeometry); + Coordinate coordinate = readGeometry.getCoordinate(); + assertThat(coordinate.getZ()).isNaN(); + assertThat(coordinate.getM()).isEqualTo(3.0); + } + + @Test + public void testXYZMToWKT() { + Geometry geometry = FACTORY.createPoint(new CoordinateXYZM(1.0, 2.0, 3.0, 4.0)); + String wkt = GeometryUtil.toWKT(geometry); + Geometry readGeometry = GeometryUtil.fromWKT(wkt); + assertThat(geometry).isEqualTo(readGeometry); + Coordinate coordinate = readGeometry.getCoordinate(); + assertThat(coordinate.getZ()).isEqualTo(3.0); + assertThat(coordinate.getM()).isEqualTo(4.0); + } + + @Test + public void testBoundMayIntersects() { + GeometryFactory factory = new GeometryFactory(); + + // Test regular case (not crossing anti-meridian) + Point lowerBound = factory.createPoint(new Coordinate(0, 0)); + Point upperBound = factory.createPoint(new Coordinate(10, 10)); + + // Envelope completely inside bound + Geometry geom = factory.toGeometry(new Envelope(2, 8, 2, 8)); + assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isTrue(); + + // Envelope partially overlapping bound + geom = factory.toGeometry(new Envelope(5, 15, 5, 15)); + assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isTrue(); + + // Envelope completely outside bound + geom = factory.toGeometry(new Envelope(15, 20, 15, 20)); + assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isFalse(); + + // Test anti-meridian crossing case + lowerBound = factory.createPoint(new Coordinate(170, 0)); + upperBound = factory.createPoint(new Coordinate(-170, 10)); + + // Envelope in the western part of the bound + geom = factory.toGeometry(new Envelope(172, 178, 2, 8)); + assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isTrue(); + + // Envelope in the eastern part of the bound + geom = factory.toGeometry(new Envelope(-178, -172, 2, 8)); + assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isTrue(); + + // Envelope crossing the anti-meridian within the bound + geom = factory.toGeometry(new Envelope(175, -175, 2, 8)); + assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isTrue(); + + // Envelope outside the bound (latitude) + geom = factory.toGeometry(new Envelope(172, 178, 12, 15)); + assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isFalse(); + + // Envelope outside the bound (longitude) + geom = factory.toGeometry(new Envelope(160, 165, 2, 8)); + assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isFalse(); + } + + @Test + public void testBoundMayCovers() { + GeometryFactory factory = new GeometryFactory(); + + // Test regular case (not crossing anti-meridian) + Point lowerBound = factory.createPoint(new Coordinate(0, 0)); + Point upperBound = factory.createPoint(new Coordinate(10, 10)); + + // Envelope completely inside bound + Geometry geom = factory.toGeometry(new Envelope(2, 8, 2, 8)); + assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isTrue(); + + // Envelope partially inside bound + geom = factory.toGeometry(new Envelope(5, 15, 5, 15)); + assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isFalse(); + + // Envelope completely outside bound + geom = factory.toGeometry(new Envelope(15, 20, 15, 20)); + assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isFalse(); + + // Test anti-meridian crossing case + lowerBound = factory.createPoint(new Coordinate(170, 0)); + upperBound = factory.createPoint(new Coordinate(-170, 10)); + + // Envelope in the western part of the bound + geom = factory.toGeometry(new Envelope(172, 178, 2, 8)); + assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isTrue(); + + // Envelope in the eastern part of the bound + geom = factory.toGeometry(new Envelope(-178, -172, 2, 8)); + assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isTrue(); + + // Envelope partially outside the bound (latitude) + geom = factory.toGeometry(new Envelope(172, 178, -2, 12)); + assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isFalse(); + + // Envelope outside the bound (longitude) + geom = factory.toGeometry(new Envelope(160, 165, 2, 8)); + assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isFalse(); + } + + @Test + public void testBoundMustBeCoveredBy() { + GeometryFactory factory = new GeometryFactory(); + + // Test regular case (not crossing anti-meridian) + Point lowerBound = factory.createPoint(new Coordinate(2, 2)); + Point upperBound = factory.createPoint(new Coordinate(8, 8)); + + // Envelope completely covering the bound + Geometry geom = factory.toGeometry(new Envelope(0, 10, 0, 10)); + assertThat(GeometryUtil.boundMustBeCoveredBy(lowerBound, upperBound, geom)).isTrue(); + + // Envelope partially covering the bound + geom = factory.toGeometry(new Envelope(3, 10, 0, 10)); + assertThat(GeometryUtil.boundMustBeCoveredBy(lowerBound, upperBound, geom)).isFalse(); + + // Envelope not covering the bound + geom = factory.toGeometry(new Envelope(0, 5, 0, 5)); + assertThat(GeometryUtil.boundMustBeCoveredBy(lowerBound, upperBound, geom)).isFalse(); + + // Test anti-meridian crossing case - should always return false + lowerBound = factory.createPoint(new Coordinate(170, 0)); + upperBound = factory.createPoint(new Coordinate(-170, 10)); + + // Large envelope covering the entire region + geom = factory.toGeometry(new Envelope(160, -160, -10, 20)); + assertThat(GeometryUtil.boundMustBeCoveredBy(lowerBound, upperBound, geom)).isFalse(); + + // Envelope exactly matching the bound coordinates + geom = factory.toGeometry(new Envelope(170, -170, 0, 10)); + assertThat(GeometryUtil.boundMustBeCoveredBy(lowerBound, upperBound, geom)).isFalse(); + } +} diff --git a/build.gradle b/build.gradle index 35723b57df4a..c4944c8f2429 100644 --- a/build.gradle +++ b/build.gradle @@ -292,6 +292,7 @@ project(':iceberg-api') { dependencies { implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow') + api libs.jts.core compileOnly libs.errorprone.annotations compileOnly libs.findbugs.jsr305 testImplementation libs.avro.avro diff --git a/core/src/main/java/org/apache/iceberg/SchemaParser.java b/core/src/main/java/org/apache/iceberg/SchemaParser.java index d7c756795711..f49c09fee19e 100644 --- a/core/src/main/java/org/apache/iceberg/SchemaParser.java +++ b/core/src/main/java/org/apache/iceberg/SchemaParser.java @@ -44,6 +44,8 @@ private SchemaParser() {} private static final String STRUCT = "struct"; private static final String LIST = "list"; private static final String MAP = "map"; + private static final String GEOMETRY = "geometry"; + private static final String GEOGRAPHY = "geography"; private static final String FIELDS = "fields"; private static final String ELEMENT = "element"; private static final String KEY = "key"; @@ -141,7 +143,22 @@ static void toJson(Types.MapType map, JsonGenerator generator) throws IOExceptio } static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws IOException { - generator.writeString(primitive.toString()); + if (primitive.typeId() == Type.TypeID.GEOMETRY) { + Types.GeometryType geometryType = (Types.GeometryType) primitive; + generator.writeStartObject(); + generator.writeStringField("type", "geometry"); + generator.writeStringField("crs", geometryType.crs()); + generator.writeEndObject(); + } else if (primitive.typeId() == Type.TypeID.GEOGRAPHY) { + Types.GeographyType geographyType = (Types.GeographyType) primitive; + generator.writeStartObject(); + generator.writeStringField("type", "geography"); + generator.writeStringField("crs", geographyType.crs()); + generator.writeStringField("algorithm", geographyType.algorithm().name()); + generator.writeEndObject(); + } else { + generator.writeString(primitive.toString()); + } } static void toJson(Type type, JsonGenerator generator) throws IOException { @@ -192,6 +209,10 @@ private static Type typeFromJson(JsonNode json) { return listFromJson(json); } else if (MAP.equals(type)) { return mapFromJson(json); + } else if (GEOMETRY.equals(type)) { + return geometryFromJson(json); + } else if (GEOGRAPHY.equals(type)) { + return geographyFromJson(json); } } } @@ -277,6 +298,17 @@ private static Types.MapType mapFromJson(JsonNode json) { } } + private static Types.GeometryType geometryFromJson(JsonNode json) { + String crs = JsonUtil.getStringOrNull("crs", json); + return Types.GeometryType.of(crs); + } + + private static Types.GeographyType geographyFromJson(JsonNode json) { + String crs = JsonUtil.getStringOrNull("crs", json); + String algorithm = JsonUtil.getStringOrNull("algorithm", json); + return Types.GeographyType.of(crs, algorithm); + } + public static Schema fromJson(JsonNode json) { Type type = typeFromJson(json); Preconditions.checkArgument( diff --git a/core/src/main/java/org/apache/iceberg/SingleValueParser.java b/core/src/main/java/org/apache/iceberg/SingleValueParser.java index 3de6a0bcc663..1e3998e75a81 100644 --- a/core/src/main/java/org/apache/iceberg/SingleValueParser.java +++ b/core/src/main/java/org/apache/iceberg/SingleValueParser.java @@ -38,7 +38,9 @@ import org.apache.iceberg.types.Types; import org.apache.iceberg.util.ByteBuffers; import org.apache.iceberg.util.DateTimeUtil; +import org.apache.iceberg.util.GeometryUtil; import org.apache.iceberg.util.JsonUtil; +import org.locationtech.jts.geom.Geometry; public class SingleValueParser { private SingleValueParser() {} @@ -160,6 +162,20 @@ public static Object fromJson(Type type, JsonNode defaultValue) { byte[] binaryBytes = BaseEncoding.base16().decode(defaultValue.textValue().toUpperCase(Locale.ROOT)); return ByteBuffer.wrap(binaryBytes); + case GEOMETRY: + case GEOGRAPHY: + Preconditions.checkArgument( + defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue); + try { + Geometry geom = GeometryUtil.fromWKT(defaultValue.textValue()); + if (type.typeId() == Type.TypeID.GEOGRAPHY) { + return new Geography(geom); + } + return geom; + } catch (Exception e) { + throw new IllegalArgumentException( + String.format("Cannot parse default as a %s value: %s", type, defaultValue), e); + } case LIST: return listFromJson(type, defaultValue); case MAP: @@ -335,6 +351,16 @@ public static void toJson(Type type, Object defaultValue, JsonGenerator generato generator.writeString(decimalValue.toString()); } break; + case GEOMETRY: + Preconditions.checkArgument( + defaultValue instanceof Geometry, "Invalid default %s value: %s", type, defaultValue); + generator.writeString(GeometryUtil.toWKT((Geometry) defaultValue)); + break; + case GEOGRAPHY: + Preconditions.checkArgument( + defaultValue instanceof Geography, "Invalid default %s value: %s", type, defaultValue); + generator.writeString(GeometryUtil.toWKT(((Geography) defaultValue).geometry())); + break; case LIST: Preconditions.checkArgument( defaultValue instanceof List, "Invalid default %s value: %s", type, defaultValue); diff --git a/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java b/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java new file mode 100644 index 000000000000..504afc73ad74 --- /dev/null +++ b/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg; + +import static org.apache.iceberg.types.Types.NestedField.required; +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.IOException; +import java.util.Map; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.hadoop.HadoopCatalog; +import org.apache.iceberg.hadoop.HadoopTableTestBase; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.types.Type; +import org.apache.iceberg.types.Types; +import org.junit.jupiter.api.Test; + +public class TestGeospatialTable extends HadoopTableTestBase { + + @Test + public void testCreateGeospatialTable() throws IOException { + Schema schema = + new Schema( + required(3, "id", Types.IntegerType.get(), "unique ID"), + required(4, "data", Types.StringType.get()), + required(5, "geom", Types.GeometryType.of("srid:3857"), "geometry column"), + required( + 6, + "geog", + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY), + "geography column")); + + TableIdentifier identifier = TableIdentifier.of("a", "geos_t1"); + try (HadoopCatalog catalog = hadoopCatalog()) { + Map properties = ImmutableMap.of(TableProperties.FORMAT_VERSION, "3"); + catalog.createTable(identifier, schema, PartitionSpec.unpartitioned(), properties); + Table table = catalog.loadTable(identifier); + + Types.NestedField geomField = table.schema().findField("geom"); + assertThat(geomField.type().typeId()).isEqualTo(Type.TypeID.GEOMETRY); + Types.GeometryType geomType = (Types.GeometryType) geomField.type(); + assertThat(geomType.crs()).isEqualTo("srid:3857"); + + Types.NestedField geogField = table.schema().findField("geog"); + assertThat(geogField.type().typeId()).isEqualTo(Type.TypeID.GEOGRAPHY); + Types.GeographyType geogType = (Types.GeographyType) geogField.type(); + assertThat(geogType.crs()).isEqualTo("srid:4269"); + assertThat(geogType.algorithm()).isEqualTo(Geography.EdgeInterpolationAlgorithm.KARNEY); + assertThat(catalog.dropTable(identifier)).isTrue(); + } + } +} diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaParser.java b/core/src/test/java/org/apache/iceberg/TestSchemaParser.java index cf6b03ee1417..43c114319e4f 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaParser.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaParser.java @@ -34,6 +34,7 @@ import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.DateTimeUtil; +import org.apache.iceberg.util.GeometryUtil; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -120,7 +121,11 @@ private static Stream primitiveTypesAndDefaults() { Types.FixedType.ofLength(4), Literal.of(ByteBuffer.wrap(new byte[] {0x0a, 0x0b, 0x0c, 0x0d}))), Arguments.of(Types.BinaryType.get(), Literal.of(ByteBuffer.wrap(new byte[] {0x0a, 0x0b}))), - Arguments.of(Types.DecimalType.of(9, 2), Literal.of(new BigDecimal("12.34")))); + Arguments.of(Types.DecimalType.of(9, 2), Literal.of(new BigDecimal("12.34"))), + Arguments.of(Types.GeometryType.get(), Literal.of(GeometryUtil.fromWKT("POINT (1 2)"))), + Arguments.of( + Types.GeographyType.get(), + Literal.of(new Geography(GeometryUtil.fromWKT("POINT (1 2)"))))); } @ParameterizedTest @@ -142,4 +147,31 @@ public void testPrimitiveTypeDefaultValues(Type.PrimitiveType type, Literal d assertThat(serialized.findField("col_with_default").writeDefault()) .isEqualTo(defaultValue.value()); } + + @Test + public void testVariantType() throws IOException { + Schema schema = + new Schema( + Types.NestedField.required(1, "id", Types.IntegerType.get()), + Types.NestedField.optional(2, "data", Types.VariantType.get())); + + writeAndValidate(schema); + } + + @Test + public void testSpatialType() throws IOException { + Schema schema = + new Schema( + Types.NestedField.required(1, "id", Types.IntegerType.get()), + Types.NestedField.optional(2, "geom0", Types.GeometryType.get()), + Types.NestedField.optional(3, "geom1", Types.GeometryType.of("srid:3857")), + Types.NestedField.optional(4, "geog0", Types.GeographyType.get()), + Types.NestedField.optional(5, "geog1", Types.GeographyType.of("srid:4269")), + Types.NestedField.optional( + 6, + "geog2", + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY))); + + writeAndValidate(schema); + } } diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java index d1591f80d836..411e6eee1e15 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java @@ -364,7 +364,11 @@ public void testUpdateFailure() { Types.FixedType.ofLength(4), Types.DecimalType.of(9, 2), Types.DecimalType.of(9, 3), - Types.DecimalType.of(18, 2)); + Types.DecimalType.of(18, 2), + Types.GeometryType.get(), + Types.GeometryType.of("srid:3857"), + Types.GeographyType.get(), + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY)); for (Type.PrimitiveType fromType : primitives) { for (Type.PrimitiveType toType : primitives) { diff --git a/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java b/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java index cc1578b0e081..3c5b3a9fc7d8 100644 --- a/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java +++ b/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java @@ -53,6 +53,16 @@ public void testValidDefaults() throws IOException { {Types.DecimalType.of(9, 4), "\"123.4500\""}, {Types.DecimalType.of(9, 0), "\"2\""}, {Types.DecimalType.of(9, -20), "\"2E+20\""}, + {Types.GeometryType.get(), "\"POINT (1 2)\""}, + {Types.GeometryType.get(), "\"POINT Z(1 2 3)\""}, + {Types.GeometryType.get(), "\"POINT M(1 2 3)\""}, + {Types.GeometryType.get(), "\"POINT ZM(1 2 3 4)\""}, + {Types.GeometryType.of("srid:3857"), "\"POINT (1 2)\""}, + {Types.GeographyType.get(), "\"POINT ZM(1 2 3 4)\""}, + { + Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY), + "\"POINT ZM(1 2 3 4)\"" + }, {Types.ListType.ofOptional(1, Types.IntegerType.get()), "[1, 2, 3]"}, { Types.MapType.ofOptional(2, 3, Types.IntegerType.get(), Types.StringType.get()), @@ -157,6 +167,15 @@ public void testInvalidTimestamptz() { .hasMessageStartingWith("Cannot parse default as a timestamptz value"); } + @Test + public void testInvalidGeometry() { + Type expectedType = Types.GeometryType.get(); + String defaultJson = "\"POINT (1 2 3 4 5 6)\""; + assertThatThrownBy(() -> defaultValueParseAndUnParseRoundTrip(expectedType, defaultJson)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageMatching("Cannot parse default as a geometry.* value.*"); + } + // serialize to json and deserialize back should return the same result private static String defaultValueParseAndUnParseRoundTrip(Type type, String defaultValue) { Object javaDefaultValue = SingleValueParser.fromJson(type, defaultValue); diff --git a/core/src/test/java/org/apache/iceberg/TestTableMetadata.java b/core/src/test/java/org/apache/iceberg/TestTableMetadata.java index 07b4b0591646..59f4aab3d8c2 100644 --- a/core/src/test/java/org/apache/iceberg/TestTableMetadata.java +++ b/core/src/test/java/org/apache/iceberg/TestTableMetadata.java @@ -1825,6 +1825,43 @@ public void testConstructV3Metadata() { 3); } + @Test + public void testV3GeometryTypeSupport() { + Schema v3SchemaGeom = + new Schema( + Types.NestedField.required(3, "id", Types.LongType.get()), + Types.NestedField.required(4, "geom", Types.GeometryType.get())); + Schema v3SchemaGeog = + new Schema( + Types.NestedField.required(3, "id", Types.LongType.get()), + Types.NestedField.required(4, "geog", Types.GeographyType.get())); + + for (Schema schema : ImmutableList.of(v3SchemaGeom, v3SchemaGeog)) { + for (int unsupportedFormatVersion : ImmutableList.of(1, 2)) { + assertThatThrownBy( + () -> + TableMetadata.newTableMetadata( + schema, + PartitionSpec.unpartitioned(), + SortOrder.unsorted(), + TEST_LOCATION, + ImmutableMap.of(), + unsupportedFormatVersion)) + .isInstanceOf(IllegalStateException.class) + .hasMessageContaining("not supported until v3"); + } + + // should be allowed in v3 + TableMetadata.newTableMetadata( + schema, + PartitionSpec.unpartitioned(), + SortOrder.unsorted(), + TEST_LOCATION, + ImmutableMap.of(), + 3); + } + } + @Test public void onlyMetadataLocationIsUpdatedWithoutTimestampAndMetadataLogEntry() { String uuid = "386b9f01-002b-4d8c-b77f-42c3fd3b7c9b"; diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 4fffa1e14d26..0d788c830131 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -62,6 +62,7 @@ jakarta-servlet-api = "6.1.0" jaxb-api = "2.3.1" jaxb-runtime = "2.3.9" jetty = "11.0.24" +jts-core = "1.20.0" junit = "5.11.4" junit-platform = "1.11.4" kafka = "3.9.0" @@ -146,6 +147,7 @@ jackson214-bom = { module = "com.fasterxml.jackson:jackson-bom", version.ref = jackson215-bom = { module = "com.fasterxml.jackson:jackson-bom", version.ref = "jackson215" } jaxb-api = { module = "javax.xml.bind:jaxb-api", version.ref = "jaxb-api" } jaxb-runtime = { module = "org.glassfish.jaxb:jaxb-runtime", version.ref = "jaxb-runtime" } +jts-core = { module = "org.locationtech.jts:jts-core", version.ref = "jts-core" } kafka-clients = { module = "org.apache.kafka:kafka-clients", version.ref = "kafka" } kafka-connect-api = { module = "org.apache.kafka:connect-api", version.ref = "kafka" } kafka-connect-json = { module = "org.apache.kafka:connect-json", version.ref = "kafka" } From cccb56366cdc6abcfe3ff0043ebbd596fc055ff5 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Tue, 4 Mar 2025 12:46:06 +0800 Subject: [PATCH 02/16] Remove spatial predicate evaluation code from GeometryUtil --- .../org/apache/iceberg/util/GeometryUtil.java | 146 ------------------ .../apache/iceberg/util/TestGeometryUtil.java | 121 --------------- 2 files changed, 267 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/util/GeometryUtil.java b/api/src/main/java/org/apache/iceberg/util/GeometryUtil.java index e161605d12a8..e871933a9b2e 100644 --- a/api/src/main/java/org/apache/iceberg/util/GeometryUtil.java +++ b/api/src/main/java/org/apache/iceberg/util/GeometryUtil.java @@ -18,13 +18,8 @@ */ package org.apache.iceberg.util; -import org.apache.iceberg.Geography; -import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.locationtech.jts.geom.Coordinate; -import org.locationtech.jts.geom.Envelope; import org.locationtech.jts.geom.Geometry; -import org.locationtech.jts.geom.GeometryFactory; -import org.locationtech.jts.geom.Point; import org.locationtech.jts.io.WKBReader; import org.locationtech.jts.io.WKBWriter; import org.locationtech.jts.io.WKTReader; @@ -34,8 +29,6 @@ public class GeometryUtil { private GeometryUtil() {} - private static final GeometryFactory FACTORY = new GeometryFactory(); - public static byte[] toWKB(Geometry geom) { WKBWriter wkbWriter = new WKBWriter(getOutputDimension(geom), false); return wkbWriter.write(geom); @@ -79,143 +72,4 @@ public static int getOutputDimension(Geometry geom) { } return dimension; } - - /** - * Check if the geometry may intersect with the given bound. The bound represents a rectangle - * crossing the anti-meridian when the x of lower bound is greater than the x of upper bound. - * - * @param lowerBound The lower-left point of the bound - * @param upperBound The upper-right point of the bound - * @param geom The geometry to check - * @return true if the geometry may intersect with the bound; false if the geometry definitely - * does not intersect with the bound - */ - public static boolean boundMayIntersects( - Geometry lowerBound, Geometry upperBound, Geometry geom) { - Preconditions.checkArgument(lowerBound instanceof Point, "Lower bound must be a point"); - Preconditions.checkArgument(upperBound instanceof Point, "Upper bound must be a point"); - - Coordinate lowerCoordinate = lowerBound.getCoordinate(); - Coordinate upperCoordinate = upperBound.getCoordinate(); - if (lowerCoordinate.x <= upperCoordinate.x) { - // Not crossing the anti-meridian - Envelope envelope = new Envelope(lowerBound.getCoordinate(), upperBound.getCoordinate()); - return geom.intersects(FACTORY.toGeometry(envelope)); - } else { - // Crossing the anti-meridian. Use the envelope of geom to evaluate the intersection with - // false positives - Envelope envelope = geom.getEnvelopeInternal(); - if (envelope.getMinY() > upperCoordinate.y || envelope.getMaxY() < lowerCoordinate.y) { - return false; - } - return (envelope.getMinX() <= upperCoordinate.x || envelope.getMaxX() >= lowerCoordinate.x); - } - } - - /** - * Check if the geography may intersect with the given bound. The bound represents a rectangle - * crossing the anti-meridian when the x of lower bound is greater than the x of upper bound. - * - * @param lowerBound The lower-left point of the bound - * @param upperBound The upper-right point of the bound - * @param geog The geography to check - * @return true if the geography may intersect with the bound; false if the geography definitely - * does not intersect with the bound - */ - public static boolean boundMayIntersects( - Geography lowerBound, Geography upperBound, Geography geog) { - // TODO: implement a correct spherical intersection algorithm - return boundMayIntersects(lowerBound.geometry(), upperBound.geometry(), geog.geometry()); - } - - /** - * Check if the bound may cover the geometry. The bound represents a rectangle crossing the - * anti-meridian when the x of lower bound is greater than the x of upper bound. - * - * @param lowerBound The lower-left point of the bound - * @param upperBound The upper-right point of the bound - * @param geom The geometry to check - * @return true if the bound may cover the geometry; false if the bound definitely does not cover - * the geometry - */ - public static boolean boundMayCovers(Geometry lowerBound, Geometry upperBound, Geometry geom) { - Preconditions.checkArgument(lowerBound instanceof Point, "Lower bound must be a point"); - Preconditions.checkArgument(upperBound instanceof Point, "Upper bound must be a point"); - - Coordinate lowerCoordinate = lowerBound.getCoordinate(); - Coordinate upperCoordinate = upperBound.getCoordinate(); - if (lowerCoordinate.x <= upperCoordinate.x) { - // Not crossing the anti-meridian - Envelope envelope = new Envelope(lowerBound.getCoordinate(), upperBound.getCoordinate()); - return FACTORY.toGeometry(envelope).covers(geom); - } else { - // Crossing the anti-meridian. Use the envelope of geom to evaluate the covers with false - // positives - Envelope envelope = geom.getEnvelopeInternal(); - if (envelope.getMinY() < lowerCoordinate.y || envelope.getMaxY() > upperCoordinate.y) { - return false; - } - return (envelope.getMaxX() <= upperCoordinate.x || envelope.getMinX() >= lowerCoordinate.x); - } - } - - /** - * Check if the bound may cover the geography. The bound represents a rectangle crossing the - * anti-meridian when the x of lower bound is greater than the x of upper bound. - * - * @param lowerBound The lower-left point of the bound - * @param upperBound The upper-right point of the bound - * @param geog The geography to check - * @return true if the bound may cover the geography; false if the bound definitely does not cover - * the geography - */ - public static boolean boundMayCovers(Geography lowerBound, Geography upperBound, Geography geog) { - // TODO: implement a correct spherical covers algorithm - return boundMayCovers(lowerBound.geometry(), upperBound.geometry(), geog.geometry()); - } - - /** - * Check if we are sure that the bound must be covered by the geometry. The bound represents a - * rectangle crossing the anti-meridian when the x of lower bound is greater than the x of upper - * bound. - * - * @param lowerBound The lower-left point of the bound - * @param upperBound The upper-right point of the bound - * @param geom The geometry to check - * @return true if the bound is definitely covered by the geometry; false if the bound may or may - * not cover the geometry - */ - public static boolean boundMustBeCoveredBy( - Geometry lowerBound, Geometry upperBound, Geometry geom) { - Preconditions.checkArgument(lowerBound instanceof Point, "Lower bound must be a point"); - Preconditions.checkArgument(upperBound instanceof Point, "Upper bound must be a point"); - - Coordinate lowerCoordinate = lowerBound.getCoordinate(); - Coordinate upperCoordinate = upperBound.getCoordinate(); - if (lowerCoordinate.x <= upperCoordinate.x) { - // Not crossing the anti-meridian - Envelope envelope = new Envelope(lowerBound.getCoordinate(), upperBound.getCoordinate()); - return FACTORY.toGeometry(envelope).coveredBy(geom); - } else { - // Crossing the anti-meridian. This case can be tricky so we always return false to be safe. - return false; - } - } - - /** - * Check if we are sure that the bound must be covered by the geography. The bound represents a - * rectangle crossing the anti-meridian when the x of lower bound is greater than the x of upper - * bound. - * - * @param lowerBound The lower-left point of the bound - * @param upperBound The upper-right point of the bound - * @param geog The geography to check - * @return true if the bound is definitely covered by the geography; false if the bound may or may - * not cover the geography - */ - public static boolean boundMustBeCoveredBy( - Geography lowerBound, Geography upperBound, Geography geog) { - // TODO: implement a correct spherical covered-by algorithm - return boundMustBeCoveredBy(lowerBound.geometry(), upperBound.geometry(), geog.geometry()); - } } diff --git a/api/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java b/api/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java index 6dfb8d9e5023..3ab4fcf2525c 100644 --- a/api/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java +++ b/api/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java @@ -26,10 +26,8 @@ import org.locationtech.jts.geom.CoordinateXY; import org.locationtech.jts.geom.CoordinateXYM; import org.locationtech.jts.geom.CoordinateXYZM; -import org.locationtech.jts.geom.Envelope; import org.locationtech.jts.geom.Geometry; import org.locationtech.jts.geom.GeometryFactory; -import org.locationtech.jts.geom.Point; public class TestGeometryUtil { private static final GeometryFactory FACTORY = new GeometryFactory(); @@ -144,123 +142,4 @@ public void testXYZMToWKT() { assertThat(coordinate.getZ()).isEqualTo(3.0); assertThat(coordinate.getM()).isEqualTo(4.0); } - - @Test - public void testBoundMayIntersects() { - GeometryFactory factory = new GeometryFactory(); - - // Test regular case (not crossing anti-meridian) - Point lowerBound = factory.createPoint(new Coordinate(0, 0)); - Point upperBound = factory.createPoint(new Coordinate(10, 10)); - - // Envelope completely inside bound - Geometry geom = factory.toGeometry(new Envelope(2, 8, 2, 8)); - assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isTrue(); - - // Envelope partially overlapping bound - geom = factory.toGeometry(new Envelope(5, 15, 5, 15)); - assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isTrue(); - - // Envelope completely outside bound - geom = factory.toGeometry(new Envelope(15, 20, 15, 20)); - assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isFalse(); - - // Test anti-meridian crossing case - lowerBound = factory.createPoint(new Coordinate(170, 0)); - upperBound = factory.createPoint(new Coordinate(-170, 10)); - - // Envelope in the western part of the bound - geom = factory.toGeometry(new Envelope(172, 178, 2, 8)); - assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isTrue(); - - // Envelope in the eastern part of the bound - geom = factory.toGeometry(new Envelope(-178, -172, 2, 8)); - assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isTrue(); - - // Envelope crossing the anti-meridian within the bound - geom = factory.toGeometry(new Envelope(175, -175, 2, 8)); - assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isTrue(); - - // Envelope outside the bound (latitude) - geom = factory.toGeometry(new Envelope(172, 178, 12, 15)); - assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isFalse(); - - // Envelope outside the bound (longitude) - geom = factory.toGeometry(new Envelope(160, 165, 2, 8)); - assertThat(GeometryUtil.boundMayIntersects(lowerBound, upperBound, geom)).isFalse(); - } - - @Test - public void testBoundMayCovers() { - GeometryFactory factory = new GeometryFactory(); - - // Test regular case (not crossing anti-meridian) - Point lowerBound = factory.createPoint(new Coordinate(0, 0)); - Point upperBound = factory.createPoint(new Coordinate(10, 10)); - - // Envelope completely inside bound - Geometry geom = factory.toGeometry(new Envelope(2, 8, 2, 8)); - assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isTrue(); - - // Envelope partially inside bound - geom = factory.toGeometry(new Envelope(5, 15, 5, 15)); - assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isFalse(); - - // Envelope completely outside bound - geom = factory.toGeometry(new Envelope(15, 20, 15, 20)); - assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isFalse(); - - // Test anti-meridian crossing case - lowerBound = factory.createPoint(new Coordinate(170, 0)); - upperBound = factory.createPoint(new Coordinate(-170, 10)); - - // Envelope in the western part of the bound - geom = factory.toGeometry(new Envelope(172, 178, 2, 8)); - assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isTrue(); - - // Envelope in the eastern part of the bound - geom = factory.toGeometry(new Envelope(-178, -172, 2, 8)); - assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isTrue(); - - // Envelope partially outside the bound (latitude) - geom = factory.toGeometry(new Envelope(172, 178, -2, 12)); - assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isFalse(); - - // Envelope outside the bound (longitude) - geom = factory.toGeometry(new Envelope(160, 165, 2, 8)); - assertThat(GeometryUtil.boundMayCovers(lowerBound, upperBound, geom)).isFalse(); - } - - @Test - public void testBoundMustBeCoveredBy() { - GeometryFactory factory = new GeometryFactory(); - - // Test regular case (not crossing anti-meridian) - Point lowerBound = factory.createPoint(new Coordinate(2, 2)); - Point upperBound = factory.createPoint(new Coordinate(8, 8)); - - // Envelope completely covering the bound - Geometry geom = factory.toGeometry(new Envelope(0, 10, 0, 10)); - assertThat(GeometryUtil.boundMustBeCoveredBy(lowerBound, upperBound, geom)).isTrue(); - - // Envelope partially covering the bound - geom = factory.toGeometry(new Envelope(3, 10, 0, 10)); - assertThat(GeometryUtil.boundMustBeCoveredBy(lowerBound, upperBound, geom)).isFalse(); - - // Envelope not covering the bound - geom = factory.toGeometry(new Envelope(0, 5, 0, 5)); - assertThat(GeometryUtil.boundMustBeCoveredBy(lowerBound, upperBound, geom)).isFalse(); - - // Test anti-meridian crossing case - should always return false - lowerBound = factory.createPoint(new Coordinate(170, 0)); - upperBound = factory.createPoint(new Coordinate(-170, 10)); - - // Large envelope covering the entire region - geom = factory.toGeometry(new Envelope(160, -160, -10, 20)); - assertThat(GeometryUtil.boundMustBeCoveredBy(lowerBound, upperBound, geom)).isFalse(); - - // Envelope exactly matching the bound coordinates - geom = factory.toGeometry(new Envelope(170, -170, 0, 10)); - assertThat(GeometryUtil.boundMustBeCoveredBy(lowerBound, upperBound, geom)).isFalse(); - } } From 8c1c6d568676d764e1079b09fa394a2e49dc499f Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Tue, 4 Mar 2025 16:07:44 +0800 Subject: [PATCH 03/16] Fix interpretation of default CRS, disallow identity, bucketing and sort spec for geospatial types --- .../java/org/apache/iceberg/Geography.java | 29 ++++++++++--------- .../apache/iceberg/transforms/Identity.java | 10 ++++++- .../org/apache/iceberg/types/TypeUtil.java | 2 ++ .../java/org/apache/iceberg/types/Types.java | 11 +++---- .../iceberg/TestPartitionSpecValidation.java | 28 ++++++++++++++++-- .../iceberg/transforms/TestBucketing.java | 28 ++++++++++++++++++ .../iceberg/transforms/TestIdentity.java | 28 ++++++++++++++++++ .../java/org/apache/iceberg/SchemaParser.java | 14 +++++++-- .../org/apache/iceberg/util/GeometryUtil.java | 2 +- .../org/apache/iceberg/TestSortOrder.java | 16 ++++++++++ .../apache/iceberg/util/TestGeometryUtil.java | 0 11 files changed, 141 insertions(+), 27 deletions(-) rename {api => core}/src/main/java/org/apache/iceberg/util/GeometryUtil.java (97%) rename {api => core}/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java (100%) diff --git a/api/src/main/java/org/apache/iceberg/Geography.java b/api/src/main/java/org/apache/iceberg/Geography.java index f60183b0e76c..4e446f4edce3 100644 --- a/api/src/main/java/org/apache/iceberg/Geography.java +++ b/api/src/main/java/org/apache/iceberg/Geography.java @@ -106,22 +106,25 @@ public int hashCode() { return Objects.hashCode(geometry); } + /** + * Returns true if this geography intersects with the other geography. + * + * @param other the other geography + * @param algorithm the edge interpolation algorithm + * @return true if this geography intersects with the other geography + */ public boolean intersects(Geography other, EdgeInterpolationAlgorithm algorithm) { - if (algorithm != EdgeInterpolationAlgorithm.SPHERICAL) { - throw new UnsupportedOperationException( - "Interpolation algorithm other than spherical is not supported yet"); - } - - // TODO: implement a correct spherical intersection algorithm using S2 - return geometry.intersects(other.geometry); + throw new UnsupportedOperationException("Geography.intersects is not implemented yet"); } + /** + * Returns true if this geography contains the other geography. + * + * @param other the other geography + * @param algorithm the edge interpolation algorithm + * @return true if this geography contains the other geography + */ public boolean covers(Geography other, EdgeInterpolationAlgorithm algorithm) { - if (algorithm != EdgeInterpolationAlgorithm.SPHERICAL) { - throw new UnsupportedOperationException( - "Interpolation algorithm other than spherical is not supported yet"); - } - // TODO: implement a correct spherical covers algorithm using S2 - return geometry.covers(other.geometry); + throw new UnsupportedOperationException("Geography.covers is not implemented yet"); } } diff --git a/api/src/main/java/org/apache/iceberg/transforms/Identity.java b/api/src/main/java/org/apache/iceberg/transforms/Identity.java index 099a99cc3cf4..c2a6483b4ef2 100644 --- a/api/src/main/java/org/apache/iceberg/transforms/Identity.java +++ b/api/src/main/java/org/apache/iceberg/transforms/Identity.java @@ -19,16 +19,21 @@ package org.apache.iceberg.transforms; import java.io.ObjectStreamException; +import java.util.Set; import org.apache.iceberg.expressions.BoundPredicate; import org.apache.iceberg.expressions.Expressions; import org.apache.iceberg.expressions.UnboundPredicate; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; import org.apache.iceberg.types.Type; import org.apache.iceberg.util.SerializableFunction; class Identity implements Transform { private static final Identity INSTANCE = new Identity<>(); + private static final Set UNSUPPORTED_TYPES = + ImmutableSet.of(Type.TypeID.VARIANT, Type.TypeID.GEOMETRY, Type.TypeID.GEOGRAPHY); + private final Type type; /** @@ -39,7 +44,7 @@ class Identity implements Transform { @Deprecated public static Identity get(Type type) { Preconditions.checkArgument( - type.typeId() != Type.TypeID.VARIANT, "Unsupported type for identity: %s", type); + !UNSUPPORTED_TYPES.contains(type.typeId()), "Unsupported type for identity: %s", type); return new Identity<>(type); } @@ -93,6 +98,9 @@ public SerializableFunction bind(Type type) { @Override public boolean canTransform(Type maybePrimitive) { + if (UNSUPPORTED_TYPES.contains(maybePrimitive.typeId())) { + return false; + } return maybePrimitive.isPrimitiveType(); } diff --git a/api/src/main/java/org/apache/iceberg/types/TypeUtil.java b/api/src/main/java/org/apache/iceberg/types/TypeUtil.java index 91a922063fe3..990aa0834cf0 100644 --- a/api/src/main/java/org/apache/iceberg/types/TypeUtil.java +++ b/api/src/main/java/org/apache/iceberg/types/TypeUtil.java @@ -539,6 +539,8 @@ private static int estimateSize(Type type) { return ((Types.FixedType) type).length(); case BINARY: case VARIANT: + case GEOMETRY: + case GEOGRAPHY: return 80; case UNKNOWN: // Consider Unknown as null diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index 29fcbf92a5fc..833a94fb96f0 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -564,8 +564,6 @@ public int hashCode() { public static class GeometryType extends PrimitiveType { - public static final String DEFAULT_CRS = "OGC:CRS84"; - private final String crs; private GeometryType(String crs) { @@ -573,11 +571,11 @@ private GeometryType(String crs) { } public static GeometryType get() { - return of(DEFAULT_CRS); + return of(""); } public static GeometryType of(String crs) { - return new GeometryType(crs == null ? DEFAULT_CRS : crs); + return new GeometryType(crs == null ? "" : crs); } @Override @@ -614,7 +612,6 @@ public String toString() { public static class GeographyType extends PrimitiveType { - public static final String DEFAULT_CRS = "OGC:CRS84"; public static final Geography.EdgeInterpolationAlgorithm DEFAULT_ALGORITHM = Geography.EdgeInterpolationAlgorithm.SPHERICAL; @@ -627,7 +624,7 @@ private GeographyType(String crs, Geography.EdgeInterpolationAlgorithm algorithm } public static GeographyType get() { - return of(DEFAULT_CRS); + return of(""); } public static GeographyType of(String crs) { @@ -643,7 +640,7 @@ public static GeographyType of(String crs, String algorithmName) { (algorithmName == null ? DEFAULT_ALGORITHM : Geography.EdgeInterpolationAlgorithm.fromName(algorithmName)); - return new GeographyType(crs == null ? DEFAULT_CRS : crs, algorithm); + return new GeographyType(crs == null ? "" : crs, algorithm); } @Override diff --git a/api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java b/api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java index 125b0b519fbc..1e2436e648cc 100644 --- a/api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java +++ b/api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java @@ -37,7 +37,9 @@ public class TestPartitionSpecValidation { NestedField.required(5, "another_d", Types.TimestampType.withZone()), NestedField.required(6, "s", Types.StringType.get()), NestedField.required(7, "v", Types.VariantType.get()), - NestedField.optional(8, "u", Types.UnknownType.get())); + NestedField.required(8, "geom", Types.GeometryType.get()), + NestedField.required(9, "geog", Types.GeographyType.get()), + NestedField.optional(10, "u", Types.UnknownType.get())); @Test public void testMultipleTimestampPartitions() { @@ -327,12 +329,34 @@ public void testVariantUnsupported() { .hasMessage("Cannot partition by non-primitive source field: variant"); } + @Test + public void testGeometryUnsupported() { + assertThatThrownBy( + () -> + PartitionSpec.builderFor(SCHEMA) + .add(8, 1005, "geom_partition1", Transforms.bucket(5)) + .build()) + .isInstanceOf(ValidationException.class) + .hasMessageMatching("Invalid source type geometry.* for transform: bucket.*"); + } + + @Test + public void testGeographyUnsupported() { + assertThatThrownBy( + () -> + PartitionSpec.builderFor(SCHEMA) + .add(9, 1005, "geog_partition1", Transforms.bucket(5)) + .build()) + .isInstanceOf(ValidationException.class) + .hasMessageMatching("Invalid source type geography.* for transform: bucket.*"); + } + @Test public void testUnknownUnsupported() { assertThatThrownBy( () -> PartitionSpec.builderFor(SCHEMA) - .add(8, 1005, "unknown_partition1", Transforms.bucket(5)) + .add(10, 1005, "unknown_partition1", Transforms.bucket(5)) .build()) .isInstanceOf(ValidationException.class) .hasMessage("Invalid source type unknown for transform: bucket[5]"); diff --git a/api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java b/api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java index 3c8ff93a85a3..574b6df513bd 100644 --- a/api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java +++ b/api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java @@ -431,6 +431,34 @@ public void testVariantUnsupported() { assertThat(bucket.canTransform(Types.VariantType.get())).isFalse(); } + @Test + public void testGeometryUnsupported() { + assertThatThrownBy(() -> Transforms.bucket(Types.GeometryType.get(), 3)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Cannot bucket by type: geometry"); + + Transform bucket = Transforms.bucket(3); + assertThatThrownBy(() -> bucket.bind(Types.GeometryType.get())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Cannot bucket by type: geometry"); + + assertThat(bucket.canTransform(Types.GeometryType.get())).isFalse(); + } + + @Test + public void testGeographyUnsupported() { + assertThatThrownBy(() -> Transforms.bucket(Types.GeographyType.get(), 3)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Cannot bucket by type: geography"); + + Transform bucket = Transforms.bucket(3); + assertThatThrownBy(() -> bucket.bind(Types.GeographyType.get())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Cannot bucket by type: geography"); + + assertThat(bucket.canTransform(Types.GeographyType.get())).isFalse(); + } + @Test public void testUnknownUnsupported() { assertThatThrownBy(() -> Transforms.bucket(Types.UnknownType.get(), 3)) diff --git a/api/src/test/java/org/apache/iceberg/transforms/TestIdentity.java b/api/src/test/java/org/apache/iceberg/transforms/TestIdentity.java index fc24be8d5698..a226ed5f17ab 100644 --- a/api/src/test/java/org/apache/iceberg/transforms/TestIdentity.java +++ b/api/src/test/java/org/apache/iceberg/transforms/TestIdentity.java @@ -183,4 +183,32 @@ public void testVariantUnsupported() { assertThat(Transforms.identity().canTransform(Types.VariantType.get())).isFalse(); } + + @Test + public void testGeometryUnsupported() { + assertThatThrownBy(() -> Transforms.identity().bind(Types.GeometryType.get())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Cannot bind to unsupported type: geometry"); + assertThatThrownBy(() -> Transforms.fromString(Types.GeometryType.get(), "identity")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Unsupported type for identity: geometry"); + assertThatThrownBy(() -> Transforms.identity(Types.GeometryType.get())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Unsupported type for identity: geometry"); + assertThat(Transforms.identity().canTransform(Types.GeometryType.get())).isFalse(); + } + + @Test + public void testGeographyUnsupported() { + assertThatThrownBy(() -> Transforms.identity().bind(Types.GeographyType.get())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Cannot bind to unsupported type: geography"); + assertThatThrownBy(() -> Transforms.fromString(Types.GeographyType.get(), "identity")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Unsupported type for identity: geography"); + assertThatThrownBy(() -> Transforms.identity(Types.GeographyType.get())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Unsupported type for identity: geography"); + assertThat(Transforms.identity().canTransform(Types.GeographyType.get())).isFalse(); + } } diff --git a/core/src/main/java/org/apache/iceberg/SchemaParser.java b/core/src/main/java/org/apache/iceberg/SchemaParser.java index f49c09fee19e..cdfce244816b 100644 --- a/core/src/main/java/org/apache/iceberg/SchemaParser.java +++ b/core/src/main/java/org/apache/iceberg/SchemaParser.java @@ -147,13 +147,19 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws Types.GeometryType geometryType = (Types.GeometryType) primitive; generator.writeStartObject(); generator.writeStringField("type", "geometry"); - generator.writeStringField("crs", geometryType.crs()); + String crs = geometryType.crs(); + if (!crs.isEmpty()) { + generator.writeStringField("crs", geometryType.crs()); + } generator.writeEndObject(); } else if (primitive.typeId() == Type.TypeID.GEOGRAPHY) { Types.GeographyType geographyType = (Types.GeographyType) primitive; generator.writeStartObject(); generator.writeStringField("type", "geography"); - generator.writeStringField("crs", geographyType.crs()); + String crs = geographyType.crs(); + if (!crs.isEmpty()) { + generator.writeStringField("crs", geographyType.crs()); + } generator.writeStringField("algorithm", geographyType.algorithm().name()); generator.writeEndObject(); } else { @@ -162,7 +168,9 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws } static void toJson(Type type, JsonGenerator generator) throws IOException { - if (type.isPrimitiveType() || type.isVariantType()) { + if (type.isPrimitiveType()) { + toJson(type.asPrimitiveType(), generator); + } else if (type.isVariantType()) { generator.writeString(type.toString()); } else { Type.NestedType nested = type.asNestedType(); diff --git a/api/src/main/java/org/apache/iceberg/util/GeometryUtil.java b/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java similarity index 97% rename from api/src/main/java/org/apache/iceberg/util/GeometryUtil.java rename to core/src/main/java/org/apache/iceberg/util/GeometryUtil.java index e871933a9b2e..da195229609b 100644 --- a/api/src/main/java/org/apache/iceberg/util/GeometryUtil.java +++ b/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java @@ -57,7 +57,7 @@ public static Geometry fromWKT(String wkt) { } } - public static int getOutputDimension(Geometry geom) { + private static int getOutputDimension(Geometry geom) { int dimension = 2; Coordinate coordinate = geom.getCoordinate(); diff --git a/core/src/test/java/org/apache/iceberg/TestSortOrder.java b/core/src/test/java/org/apache/iceberg/TestSortOrder.java index 7d0688e9da96..b8e570c1c8fc 100644 --- a/core/src/test/java/org/apache/iceberg/TestSortOrder.java +++ b/core/src/test/java/org/apache/iceberg/TestSortOrder.java @@ -343,6 +343,22 @@ public void testVariantUnsupported() { .hasMessage("Unsupported type for identity: variant"); } + @TestTemplate + public void testGeospatialUnsupported() { + Schema v3Schema = + new Schema( + Types.NestedField.required(3, "id", Types.LongType.get()), + Types.NestedField.required(4, "geom", Types.GeometryType.get()), + Types.NestedField.required(5, "geog", Types.GeographyType.get())); + + assertThatThrownBy(() -> SortOrder.builderFor(v3Schema).withOrderId(10).asc("geom").build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Unsupported type for identity: geometry"); + assertThatThrownBy(() -> SortOrder.builderFor(v3Schema).withOrderId(10).asc("geog").build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Unsupported type for identity: geography"); + } + @Test public void testUnknownSupported() { int fieldId = 22; diff --git a/api/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java b/core/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java similarity index 100% rename from api/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java rename to core/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java From 73f6022a69b6246bfafd2fe718708c309234b002 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Wed, 5 Mar 2025 11:28:08 +0800 Subject: [PATCH 04/16] Fix review issues --- .../java/org/apache/iceberg/types/Types.java | 12 ++--- .../iceberg/types/TestSerializableTypes.java | 2 + .../java/org/apache/iceberg/SchemaParser.java | 49 +++++++++++-------- 3 files changed, 36 insertions(+), 27 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index 833a94fb96f0..52658c06f810 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -65,7 +65,7 @@ private Types() {} private static final Pattern GEOMETRY_PARAMETERS = Pattern.compile("(?:\\(\\s*([^, ]+)?\\s*\\))?"); private static final Pattern GEOGRAPHY_PARAMETERS = - Pattern.compile("(?:\\(\\s*([^, ]+)?\\s*(?:,\\s*(\\w+)\\s*)?\\))?"); + Pattern.compile("(?:\\(\\s*([^, ]+)?\\s*(?:,\\s*(\\w*)\\s*)?\\))?"); private static final Pattern DECIMAL = Pattern.compile("decimal\\(\\s*(\\d+)\\s*,\\s*(\\d+)\\s*\\)"); @@ -628,7 +628,7 @@ public static GeographyType get() { } public static GeographyType of(String crs) { - return of(crs, DEFAULT_ALGORITHM); + return new GeographyType(crs, null); } public static GeographyType of(String crs, Geography.EdgeInterpolationAlgorithm algorithm) { @@ -637,8 +637,8 @@ public static GeographyType of(String crs, Geography.EdgeInterpolationAlgorithm public static GeographyType of(String crs, String algorithmName) { Geography.EdgeInterpolationAlgorithm algorithm = - (algorithmName == null - ? DEFAULT_ALGORITHM + ((algorithmName == null || algorithmName.isEmpty()) + ? null : Geography.EdgeInterpolationAlgorithm.fromName(algorithmName)); return new GeographyType(crs == null ? "" : crs, algorithm); } @@ -665,7 +665,7 @@ public boolean equals(Object o) { } GeographyType that = (GeographyType) o; - return crs.equals(that.crs) && algorithm.equals(that.algorithm); + return Objects.equals(crs, that.crs) && Objects.equals(algorithm, that.algorithm); } @Override @@ -675,7 +675,7 @@ public int hashCode() { @Override public String toString() { - return String.format("geography(%s, %s)", crs, algorithm.value()); + return String.format("geography(%s, %s)", crs, algorithm != null ? algorithm.value() : ""); } } diff --git a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java index 7abc90448244..281da6025f38 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java @@ -67,8 +67,10 @@ public void testEqualTypes() throws Exception { Types.FixedType.ofLength(34), Types.GeometryType.get(), Types.GeometryType.of("srid:3857"), + Types.GeometryType.of("projjson:Test_Identifier"), Types.GeographyType.get(), Types.GeographyType.of("srid:4269"), + Types.GeographyType.of("projjson:Test_Identifier"), Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY), }; diff --git a/core/src/main/java/org/apache/iceberg/SchemaParser.java b/core/src/main/java/org/apache/iceberg/SchemaParser.java index cdfce244816b..97fe0944d9dc 100644 --- a/core/src/main/java/org/apache/iceberg/SchemaParser.java +++ b/core/src/main/java/org/apache/iceberg/SchemaParser.java @@ -46,6 +46,8 @@ private SchemaParser() {} private static final String MAP = "map"; private static final String GEOMETRY = "geometry"; private static final String GEOGRAPHY = "geography"; + private static final String CRS = "crs"; + private static final String ALGORITHM = "algorithm"; private static final String FIELDS = "fields"; private static final String ELEMENT = "element"; private static final String KEY = "key"; @@ -143,27 +145,32 @@ static void toJson(Types.MapType map, JsonGenerator generator) throws IOExceptio } static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws IOException { - if (primitive.typeId() == Type.TypeID.GEOMETRY) { - Types.GeometryType geometryType = (Types.GeometryType) primitive; - generator.writeStartObject(); - generator.writeStringField("type", "geometry"); - String crs = geometryType.crs(); - if (!crs.isEmpty()) { - generator.writeStringField("crs", geometryType.crs()); - } - generator.writeEndObject(); - } else if (primitive.typeId() == Type.TypeID.GEOGRAPHY) { - Types.GeographyType geographyType = (Types.GeographyType) primitive; - generator.writeStartObject(); - generator.writeStringField("type", "geography"); - String crs = geographyType.crs(); - if (!crs.isEmpty()) { - generator.writeStringField("crs", geographyType.crs()); - } - generator.writeStringField("algorithm", geographyType.algorithm().name()); - generator.writeEndObject(); - } else { - generator.writeString(primitive.toString()); + switch (primitive.typeId()) { + case GEOMETRY: + Types.GeometryType geometryType = (Types.GeometryType) primitive; + generator.writeStartObject(); + generator.writeStringField(TYPE, GEOMETRY); + if (!geometryType.crs().isEmpty()) { + generator.writeStringField(CRS, geometryType.crs()); + } + generator.writeEndObject(); + break; + + case GEOGRAPHY: + Types.GeographyType geographyType = (Types.GeographyType) primitive; + generator.writeStartObject(); + generator.writeStringField(TYPE, GEOGRAPHY); + if (!geographyType.crs().isEmpty()) { + generator.writeStringField(CRS, geographyType.crs()); + } + if (geographyType.algorithm() != null) { + generator.writeStringField(ALGORITHM, geographyType.algorithm().name()); + } + generator.writeEndObject(); + break; + + default: + generator.writeString(primitive.toString()); } } From 7bbfb8be40e6c17450aa422802fc639ecdd05c88 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Wed, 5 Mar 2025 14:55:49 +0800 Subject: [PATCH 05/16] Remove Geometry and Geography objects from iceberg-api and map geometry and geography types to ByteBuffer --- .../java/org/apache/iceberg/Geography.java | 130 ------------------ .../apache/iceberg/expressions/Literal.java | 10 -- .../apache/iceberg/expressions/Literals.java | 65 +-------- .../org/apache/iceberg/types/Conversions.java | 64 --------- .../types/EdgeInterpolationAlgorithm.java | 64 +++++++++ .../java/org/apache/iceberg/types/Type.java | 6 +- .../java/org/apache/iceberg/types/Types.java | 16 +-- .../expressions/TestLiteralSerialization.java | 12 -- .../TestMiscLiteralConversions.java | 13 +- .../apache/iceberg/types/TestConversions.java | 105 -------------- .../iceberg/types/TestReadabilityChecks.java | 3 +- .../iceberg/types/TestSerializableTypes.java | 3 +- .../org/apache/iceberg/types/TestTypes.java | 25 ++-- build.gradle | 2 +- .../org/apache/iceberg/SingleValueParser.java | 15 +- .../apache/iceberg/TestGeospatialTable.java | 5 +- .../org/apache/iceberg/TestSchemaParser.java | 10 +- .../org/apache/iceberg/TestSchemaUpdate.java | 3 +- .../apache/iceberg/TestSingleValueParser.java | 4 +- 19 files changed, 100 insertions(+), 455 deletions(-) delete mode 100644 api/src/main/java/org/apache/iceberg/Geography.java create mode 100644 api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java diff --git a/api/src/main/java/org/apache/iceberg/Geography.java b/api/src/main/java/org/apache/iceberg/Geography.java deleted file mode 100644 index 4e446f4edce3..000000000000 --- a/api/src/main/java/org/apache/iceberg/Geography.java +++ /dev/null @@ -1,130 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.iceberg; - -import java.io.Serializable; -import java.util.Locale; -import java.util.Objects; -import org.locationtech.jts.geom.Geometry; - -/** - * Geospatial features from OGC – Simple feature access. The geometry is on a spherical or - * ellipsoidal surface. An edge-interpolation algorithm is used to evaluate spatial predicates. - */ -public class Geography implements Comparable, Serializable { - - /** The algorithm for interpolating edges. */ - public enum EdgeInterpolationAlgorithm { - /** Edges are interpolated as geodesics on a sphere. */ - SPHERICAL("spherical"), - /** See Vincenty's formulae */ - VINCENTY("vincenty"), - /** - * Thomas, Paul D. Spheroidal geodesics, reference systems, & local geometry. US Naval - * Oceanographic Office, 1970. - */ - THOMAS("thomas"), - /** - * Thomas, Paul D. Mathematical models for navigation systems. US Naval Oceanographic Office, - * 1965. - */ - ANDOYER("andoyer"), - /** - * Karney, Charles - * FF. "Algorithms for geodesics." Journal of Geodesy 87 (2013): 43-55 , and GeographicLib. - */ - KARNEY("karney"); - - private final String value; - - EdgeInterpolationAlgorithm(String value) { - this.value = value; - } - - public String value() { - return value; - } - - public static EdgeInterpolationAlgorithm fromName(String algorithmName) { - try { - return EdgeInterpolationAlgorithm.valueOf(algorithmName.toUpperCase(Locale.ENGLISH)); - } catch (IllegalArgumentException e) { - throw new IllegalArgumentException( - String.format("Invalid edge interpolation algorithm name: %s", algorithmName), e); - } - } - } - - private final Geometry geometry; - - public Geography(Geometry geometry) { - this.geometry = geometry; - } - - public Geometry geometry() { - return geometry; - } - - @Override - public String toString() { - return "Geography(" + geometry + ")"; - } - - @Override - public int compareTo(Geography o) { - return geometry.compareTo(o.geometry); - } - - @Override - public boolean equals(Object o) { - if (!(o instanceof Geography)) { - return false; - } - Geography geography = (Geography) o; - return Objects.equals(geometry, geography.geometry); - } - - @Override - public int hashCode() { - return Objects.hashCode(geometry); - } - - /** - * Returns true if this geography intersects with the other geography. - * - * @param other the other geography - * @param algorithm the edge interpolation algorithm - * @return true if this geography intersects with the other geography - */ - public boolean intersects(Geography other, EdgeInterpolationAlgorithm algorithm) { - throw new UnsupportedOperationException("Geography.intersects is not implemented yet"); - } - - /** - * Returns true if this geography contains the other geography. - * - * @param other the other geography - * @param algorithm the edge interpolation algorithm - * @return true if this geography contains the other geography - */ - public boolean covers(Geography other, EdgeInterpolationAlgorithm algorithm) { - throw new UnsupportedOperationException("Geography.covers is not implemented yet"); - } -} diff --git a/api/src/main/java/org/apache/iceberg/expressions/Literal.java b/api/src/main/java/org/apache/iceberg/expressions/Literal.java index 2de5061c0250..b5d6f72f74d0 100644 --- a/api/src/main/java/org/apache/iceberg/expressions/Literal.java +++ b/api/src/main/java/org/apache/iceberg/expressions/Literal.java @@ -23,9 +23,7 @@ import java.nio.ByteBuffer; import java.util.Comparator; import java.util.UUID; -import org.apache.iceberg.Geography; import org.apache.iceberg.types.Type; -import org.locationtech.jts.geom.Geometry; /** * Represents a literal fixed value in an expression predicate @@ -73,14 +71,6 @@ static Literal of(BigDecimal value) { return new Literals.DecimalLiteral(value); } - static Literal of(Geometry value) { - return new Literals.GeometryLiteral(value); - } - - static Literal of(Geography value) { - return new Literals.GeographyLiteral(value); - } - /** Returns the value wrapped by this literal. */ T value(); diff --git a/api/src/main/java/org/apache/iceberg/expressions/Literals.java b/api/src/main/java/org/apache/iceberg/expressions/Literals.java index b54c4768e10f..ee47035b1e72 100644 --- a/api/src/main/java/org/apache/iceberg/expressions/Literals.java +++ b/api/src/main/java/org/apache/iceberg/expressions/Literals.java @@ -32,7 +32,6 @@ import java.util.Comparator; import java.util.Objects; import java.util.UUID; -import org.apache.iceberg.Geography; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding; import org.apache.iceberg.types.Comparators; @@ -42,7 +41,6 @@ import org.apache.iceberg.util.ByteBuffers; import org.apache.iceberg.util.DateTimeUtil; import org.apache.iceberg.util.NaNUtil; -import org.locationtech.jts.geom.Geometry; class Literals { private Literals() {} @@ -57,7 +55,7 @@ private Literals() {} * @param Java type of value * @return a Literal for the given value */ - @SuppressWarnings({"unchecked", "checkstyle:CyclomaticComplexity"}) + @SuppressWarnings("unchecked") static Literal from(T value) { Preconditions.checkNotNull(value, "Cannot create expression literal from null"); Preconditions.checkArgument(!NaNUtil.isNaN(value), "Cannot create expression literal from NaN"); @@ -82,10 +80,6 @@ static Literal from(T value) { return (Literal) new Literals.BinaryLiteral((ByteBuffer) value); } else if (value instanceof BigDecimal) { return (Literal) new Literals.DecimalLiteral((BigDecimal) value); - } else if (value instanceof Geometry) { - return (Literal) new Literals.GeometryLiteral((Geometry) value); - } else if (value instanceof Geography) { - return (Literal) new Literals.GeographyLiteral((Geography) value); } throw new IllegalArgumentException( @@ -693,61 +687,4 @@ public String toString() { return "X'" + BaseEncoding.base16().encode(bytes) + "'"; } } - - static class GeometryLiteral extends BaseLiteral { - @SuppressWarnings("unchecked") - private static final Comparator CMP = - Comparators.nullsFirst().thenComparing(Comparator.naturalOrder()); - - GeometryLiteral(Geometry value) { - super(value); - } - - @Override - @SuppressWarnings("unchecked") - public Literal to(Type type) { - if (type.typeId() == Type.TypeID.GEOMETRY) { - return (Literal) this; - } - return null; - } - - @Override - public Comparator comparator() { - return CMP; - } - - @Override - protected Type.TypeID typeId() { - return Type.TypeID.GEOMETRY; - } - } - - static class GeographyLiteral extends BaseLiteral { - private static final Comparator CMP = - Comparators.nullsFirst().thenComparing(Comparator.naturalOrder()); - - GeographyLiteral(Geography value) { - super(value); - } - - @Override - @SuppressWarnings("unchecked") - public Literal to(Type type) { - if (type.typeId() == Type.TypeID.GEOGRAPHY) { - return (Literal) this; - } - return null; - } - - @Override - public Comparator comparator() { - return CMP; - } - - @Override - protected Type.TypeID typeId() { - return Type.TypeID.GEOGRAPHY; - } - } } diff --git a/api/src/main/java/org/apache/iceberg/types/Conversions.java b/api/src/main/java/org/apache/iceberg/types/Conversions.java index 4993d04e8eb2..e18c7b4362e6 100644 --- a/api/src/main/java/org/apache/iceberg/types/Conversions.java +++ b/api/src/main/java/org/apache/iceberg/types/Conversions.java @@ -29,17 +29,9 @@ import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.UUID; -import org.apache.iceberg.Geography; import org.apache.iceberg.exceptions.RuntimeIOException; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.util.UUIDUtil; -import org.locationtech.jts.geom.Coordinate; -import org.locationtech.jts.geom.CoordinateXY; -import org.locationtech.jts.geom.CoordinateXYM; -import org.locationtech.jts.geom.CoordinateXYZM; -import org.locationtech.jts.geom.Geometry; -import org.locationtech.jts.geom.GeometryFactory; -import org.locationtech.jts.geom.Point; public class Conversions { @@ -47,8 +39,6 @@ private Conversions() {} private static final String HIVE_NULL = "__HIVE_DEFAULT_PARTITION__"; - private static final GeometryFactory FACTORY = new GeometryFactory(); - public static Object fromPartitionString(Type type, String asString) { if (asString == null || HIVE_NULL.equals(asString)) { return null; @@ -127,10 +117,6 @@ public static ByteBuffer toByteBuffer(Type.TypeID typeId, Object value) { return (ByteBuffer) value; case DECIMAL: return ByteBuffer.wrap(((BigDecimal) value).unscaledValue().toByteArray()); - case GEOMETRY: - return geometryToByteBuffer((Geometry) value); - case GEOGRAPHY: - return geometryToByteBuffer(((Geography) value).geometry()); default: throw new UnsupportedOperationException("Cannot serialize type: " + typeId); } @@ -191,58 +177,8 @@ private static Object internalFromByteBuffer(Type type, ByteBuffer buffer) { byte[] unscaledBytes = new byte[buffer.remaining()]; tmp.get(unscaledBytes); return new BigDecimal(new BigInteger(unscaledBytes), decimal.scale()); - case GEOMETRY: - case GEOGRAPHY: - Coordinate coordinate = coordinateFromByteBuffer(tmp); - Geometry geometry = FACTORY.createPoint(coordinate); - if (type.typeId() == Type.TypeID.GEOMETRY) { - return geometry; - } else { - return new Geography(geometry); - } default: throw new UnsupportedOperationException("Cannot deserialize type: " + type); } } - - private static ByteBuffer geometryToByteBuffer(Geometry value) { - if (value instanceof Point) { - Coordinate coordinate = value.getCoordinate(); - return coordinateToByteBuffer(coordinate); - } else { - throw new IllegalArgumentException("Only point geometry can be converted to byte buffer"); - } - } - - private static ByteBuffer coordinateToByteBuffer(Coordinate coordinate) { - // The getZ() and getM() for a coordinate will return NaN if the value is not set. - // This is conformant with the Bound Serialization spec. - // See https://iceberg.apache.org/spec/#bound-serialization - return ByteBuffer.allocate(32) - .order(ByteOrder.LITTLE_ENDIAN) - .putDouble(0, coordinate.getX()) - .putDouble(8, coordinate.getY()) - .putDouble(16, coordinate.getZ()) - .putDouble(24, coordinate.getM()); - } - - private static Coordinate coordinateFromByteBuffer(ByteBuffer tmp) { - double coordX = tmp.getDouble(0); - double coordY = tmp.getDouble(8); - double coordZ = tmp.getDouble(16); - double coordM = tmp.getDouble(24); - boolean hasZ = !Double.isNaN(coordZ); - boolean hasM = !Double.isNaN(coordM); - Coordinate coordinate; - if (hasZ && hasM) { - coordinate = new CoordinateXYZM(coordX, coordY, coordZ, coordM); - } else if (hasZ) { - coordinate = new Coordinate(coordX, coordY, coordZ); - } else if (hasM) { - coordinate = new CoordinateXYM(coordX, coordY, coordM); - } else { - coordinate = new CoordinateXY(coordX, coordY); - } - return coordinate; - } } diff --git a/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java b/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java new file mode 100644 index 000000000000..b15ffdcdcab5 --- /dev/null +++ b/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java @@ -0,0 +1,64 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.types; + +import java.util.Locale; + +/** The algorithm for interpolating edges. */ +public enum EdgeInterpolationAlgorithm { + /** Edges are interpolated as geodesics on a sphere. */ + SPHERICAL("spherical"), + /** See Vincenty's formulae */ + VINCENTY("vincenty"), + /** + * Thomas, Paul D. Spheroidal geodesics, reference systems, & local geometry. US Naval + * Oceanographic Office, 1970. + */ + THOMAS("thomas"), + /** + * Thomas, Paul D. Mathematical models for navigation systems. US Naval Oceanographic Office, + * 1965. + */ + ANDOYER("andoyer"), + /** + * Karney, Charles + * FF. "Algorithms for geodesics." Journal of Geodesy 87 (2013): 43-55 , and GeographicLib. + */ + KARNEY("karney"); + + private final String value; + + EdgeInterpolationAlgorithm(String value) { + this.value = value; + } + + public String value() { + return value; + } + + public static EdgeInterpolationAlgorithm fromName(String algorithmName) { + try { + return EdgeInterpolationAlgorithm.valueOf(algorithmName.toUpperCase(Locale.ENGLISH)); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException( + String.format("Invalid edge interpolation algorithm name: %s", algorithmName), e); + } + } +} diff --git a/api/src/main/java/org/apache/iceberg/types/Type.java b/api/src/main/java/org/apache/iceberg/types/Type.java index d221a2455f7b..9a2fff9db205 100644 --- a/api/src/main/java/org/apache/iceberg/types/Type.java +++ b/api/src/main/java/org/apache/iceberg/types/Type.java @@ -25,10 +25,8 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import org.apache.iceberg.Geography; import org.apache.iceberg.StructLike; import org.apache.iceberg.variants.Variant; -import org.locationtech.jts.geom.Geometry; public interface Type extends Serializable { enum TypeID { @@ -46,8 +44,8 @@ enum TypeID { FIXED(ByteBuffer.class), BINARY(ByteBuffer.class), DECIMAL(BigDecimal.class), - GEOMETRY(Geometry.class), - GEOGRAPHY(Geography.class), + GEOMETRY(ByteBuffer.class), + GEOGRAPHY(ByteBuffer.class), STRUCT(StructLike.class), LIST(List.class), MAP(Map.class), diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index 52658c06f810..d32fe1ac84f6 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -26,7 +26,6 @@ import java.util.Objects; import java.util.regex.Matcher; import java.util.regex.Pattern; -import org.apache.iceberg.Geography; import org.apache.iceberg.Schema; import org.apache.iceberg.expressions.Expressions; import org.apache.iceberg.expressions.Literal; @@ -612,13 +611,10 @@ public String toString() { public static class GeographyType extends PrimitiveType { - public static final Geography.EdgeInterpolationAlgorithm DEFAULT_ALGORITHM = - Geography.EdgeInterpolationAlgorithm.SPHERICAL; - private final String crs; - private final Geography.EdgeInterpolationAlgorithm algorithm; + private final EdgeInterpolationAlgorithm algorithm; - private GeographyType(String crs, Geography.EdgeInterpolationAlgorithm algorithm) { + private GeographyType(String crs, EdgeInterpolationAlgorithm algorithm) { this.crs = crs; this.algorithm = algorithm; } @@ -631,15 +627,15 @@ public static GeographyType of(String crs) { return new GeographyType(crs, null); } - public static GeographyType of(String crs, Geography.EdgeInterpolationAlgorithm algorithm) { + public static GeographyType of(String crs, EdgeInterpolationAlgorithm algorithm) { return new GeographyType(crs, algorithm); } public static GeographyType of(String crs, String algorithmName) { - Geography.EdgeInterpolationAlgorithm algorithm = + EdgeInterpolationAlgorithm algorithm = ((algorithmName == null || algorithmName.isEmpty()) ? null - : Geography.EdgeInterpolationAlgorithm.fromName(algorithmName)); + : EdgeInterpolationAlgorithm.fromName(algorithmName)); return new GeographyType(crs == null ? "" : crs, algorithm); } @@ -652,7 +648,7 @@ public String crs() { return crs; } - public Geography.EdgeInterpolationAlgorithm algorithm() { + public EdgeInterpolationAlgorithm algorithm() { return algorithm; } diff --git a/api/src/test/java/org/apache/iceberg/expressions/TestLiteralSerialization.java b/api/src/test/java/org/apache/iceberg/expressions/TestLiteralSerialization.java index 18b9ba3abd58..24fc458b37b4 100644 --- a/api/src/test/java/org/apache/iceberg/expressions/TestLiteralSerialization.java +++ b/api/src/test/java/org/apache/iceberg/expressions/TestLiteralSerialization.java @@ -25,16 +25,10 @@ import org.apache.iceberg.TestHelpers; import org.apache.iceberg.types.Types; import org.junit.jupiter.api.Test; -import org.locationtech.jts.geom.Coordinate; -import org.locationtech.jts.geom.CoordinateXY; -import org.locationtech.jts.geom.CoordinateXYM; -import org.locationtech.jts.geom.CoordinateXYZM; -import org.locationtech.jts.geom.GeometryFactory; public class TestLiteralSerialization { @Test public void testLiterals() throws Exception { - GeometryFactory factory = new GeometryFactory(); Literal[] literals = new Literal[] { Literal.of(false), @@ -53,12 +47,6 @@ public void testLiterals() throws Exception { Literal.of(new byte[] {1, 2, 3}).to(Types.FixedType.ofLength(3)), Literal.of(new byte[] {3, 4, 5, 6}).to(Types.BinaryType.get()), Literal.of(new BigDecimal("122.50")), - Literal.of(factory.createPoint()), - Literal.of(factory.createPoint(new CoordinateXY(10, 20))), - Literal.of(factory.createPoint(new Coordinate(10, 20))), - Literal.of(factory.createPoint(new Coordinate(10, 20, 30))), - Literal.of(factory.createPoint(new CoordinateXYM(10, 20, 30))), - Literal.of(factory.createPoint(new CoordinateXYZM(10, 20, 30, 40))) }; for (Literal lit : literals) { diff --git a/api/src/test/java/org/apache/iceberg/expressions/TestMiscLiteralConversions.java b/api/src/test/java/org/apache/iceberg/expressions/TestMiscLiteralConversions.java index 3d2419ffca04..e2611ddb281f 100644 --- a/api/src/test/java/org/apache/iceberg/expressions/TestMiscLiteralConversions.java +++ b/api/src/test/java/org/apache/iceberg/expressions/TestMiscLiteralConversions.java @@ -25,16 +25,11 @@ import java.util.Arrays; import java.util.List; import java.util.UUID; -import org.apache.iceberg.Geography; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.junit.jupiter.api.Test; -import org.locationtech.jts.geom.Envelope; -import org.locationtech.jts.geom.GeometryFactory; public class TestMiscLiteralConversions { - private static final GeometryFactory FACTORY = new GeometryFactory(); - @Test public void testIdentityConversions() { List, Type>> pairs = @@ -53,13 +48,7 @@ public void testIdentityConversions() { Pair.of(Literal.of("abc"), Types.StringType.get()), Pair.of(Literal.of(UUID.randomUUID()), Types.UUIDType.get()), Pair.of(Literal.of(new byte[] {0, 1, 2}), Types.FixedType.ofLength(3)), - Pair.of(Literal.of(ByteBuffer.wrap(new byte[] {0, 1, 2})), Types.BinaryType.get()), - Pair.of( - Literal.of(FACTORY.toGeometry(new Envelope(1, 2, 10, 20))), - Types.GeometryType.get()), - Pair.of( - Literal.of(new Geography(FACTORY.toGeometry(new Envelope(1, 2, 10, 20)))), - Types.GeographyType.get())); + Pair.of(Literal.of(ByteBuffer.wrap(new byte[] {0, 1, 2})), Types.BinaryType.get())); for (Pair, Type> pair : pairs) { Literal lit = pair.first(); diff --git a/api/src/test/java/org/apache/iceberg/types/TestConversions.java b/api/src/test/java/org/apache/iceberg/types/TestConversions.java index 6f74e426b798..e207cfd8d59a 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestConversions.java +++ b/api/src/test/java/org/apache/iceberg/types/TestConversions.java @@ -25,7 +25,6 @@ import java.nio.CharBuffer; import java.nio.charset.StandardCharsets; import java.util.UUID; -import org.apache.iceberg.Geography; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.types.Types.BinaryType; import org.apache.iceberg.types.Types.BooleanType; @@ -42,12 +41,6 @@ import org.apache.iceberg.types.Types.TimestampType; import org.apache.iceberg.types.Types.UUIDType; import org.junit.jupiter.api.Test; -import org.locationtech.jts.geom.Coordinate; -import org.locationtech.jts.geom.CoordinateXY; -import org.locationtech.jts.geom.CoordinateXYM; -import org.locationtech.jts.geom.CoordinateXYZM; -import org.locationtech.jts.geom.Geometry; -import org.locationtech.jts.geom.GeometryFactory; public class TestConversions { @@ -198,104 +191,6 @@ public void testByteBufferConversions() { .isEqualTo(new byte[] {11}); } - @Test - public void testByteBufferConversionsForGeometryType() { - // geometry lower/upper boundaries are stored as 4 8-bytes floating point numbers in little - // endian. - // The 4 components are [x, y, optional z, optional m]. If z and m are not present, NaN is - // filled in. - GeometryFactory factory = new GeometryFactory(); - Geometry pointXY = factory.createPoint(new CoordinateXY(10, 20)); - assertConversion( - pointXY, - Types.GeometryType.get(), - new byte[] { - 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, - 0, 0, -8, 127 - }); - pointXY = factory.createPoint(new Coordinate(10, 20)); - assertConversion( - pointXY, - Types.GeometryType.get(), - new byte[] { - 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, - 0, 0, -8, 127 - }); - Geometry pointXYZ = factory.createPoint(new Coordinate(10, 20, 30)); - assertConversion( - pointXYZ, - Types.GeometryType.get(), - new byte[] { - 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, 62, 64, 0, 0, 0, 0, - 0, 0, -8, 127 - }); - Geometry pointXYM = factory.createPoint(new CoordinateXYM(10, 20, 30)); - assertConversion( - pointXYM, - Types.GeometryType.get(), - new byte[] { - 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, - 0, 0, 62, 64 - }); - Geometry pointXYZM = factory.createPoint(new CoordinateXYZM(10, 20, 30, 40)); - assertConversion( - pointXYZM, - Types.GeometryType.get(), - new byte[] { - 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, 62, 64, 0, 0, 0, 0, - 0, 0, 68, 64 - }); - } - - @Test - public void testByteBufferConversionsForGeographyType() { - // geography lower/upper boundaries are stored as 4 8-bytes floating point numbers in little - // endian. This is the same as geometry lower/upper boundaries. - // The 4 components are [x, y, optional z, optional m]. If z and m are not present, NaN is - // filled in. - GeometryFactory factory = new GeometryFactory(); - Geography pointXY = new Geography(factory.createPoint(new CoordinateXY(10, 20))); - assertConversion( - pointXY, - Types.GeographyType.get(), - new byte[] { - 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, - 0, 0, -8, 127 - }); - pointXY = new Geography(factory.createPoint(new Coordinate(10, 20))); - assertConversion( - pointXY, - Types.GeographyType.get(), - new byte[] { - 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, - 0, 0, -8, 127 - }); - Geography pointXYZ = new Geography(factory.createPoint(new Coordinate(10, 20, 30))); - assertConversion( - pointXYZ, - Types.GeographyType.get(), - new byte[] { - 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, 62, 64, 0, 0, 0, 0, - 0, 0, -8, 127 - }); - Geography pointXYM = new Geography(factory.createPoint(new CoordinateXYM(10, 20, 30))); - assertConversion( - pointXYM, - Types.GeographyType.get(), - new byte[] { - 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, -8, 127, 0, 0, 0, 0, - 0, 0, 62, 64 - }); - Geography pointXYZM = new Geography(factory.createPoint(new CoordinateXYZM(10, 20, 30, 40))); - assertConversion( - pointXYZM, - Types.GeographyType.get(), - new byte[] { - 0, 0, 0, 0, 0, 0, 36, 64, 0, 0, 0, 0, 0, 0, 52, 64, 0, 0, 0, 0, 0, 0, 62, 64, 0, 0, 0, 0, - 0, 0, 68, 64 - }); - } - private void assertConversion(T value, Type type, byte[] expectedBinary) { ByteBuffer byteBuffer = Conversions.toByteBuffer(type, value); assertThat(byteBuffer.array()).isEqualTo(expectedBinary); diff --git a/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java b/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java index e6984b2a20aa..f251bd93264c 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java +++ b/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java @@ -25,7 +25,6 @@ import java.util.Arrays; import java.util.List; import java.util.stream.Stream; -import org.apache.iceberg.Geography; import org.apache.iceberg.Schema; import org.apache.iceberg.types.Type.PrimitiveType; import org.junit.jupiter.api.Test; @@ -59,7 +58,7 @@ public class TestReadabilityChecks { Types.GeometryType.of("srid:3857"), Types.GeographyType.get(), Types.GeographyType.of("srid:4269"), - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY), + Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY), }; @Test diff --git a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java index 281da6025f38..34259c1cdaf0 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java @@ -22,7 +22,6 @@ import static org.apache.iceberg.types.Types.NestedField.required; import static org.assertj.core.api.Assertions.assertThat; -import org.apache.iceberg.Geography; import org.apache.iceberg.Schema; import org.apache.iceberg.TestHelpers; import org.junit.jupiter.api.Test; @@ -71,7 +70,7 @@ public void testEqualTypes() throws Exception { Types.GeographyType.get(), Types.GeographyType.of("srid:4269"), Types.GeographyType.of("projjson:Test_Identifier"), - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY), + Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY), }; for (Type type : equalityPrimitives) { diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypes.java b/api/src/test/java/org/apache/iceberg/types/TestTypes.java index 90e02baa90dc..2c32d6d402d9 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypes.java @@ -23,7 +23,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; -import org.apache.iceberg.Geography; import org.junit.jupiter.api.Test; public class TestTypes { @@ -104,20 +103,15 @@ public void testNestedFieldBuilderIdCheck() { assertThat(Types.fromPrimitiveString("geography(srid:4269)")) .isEqualTo(Types.GeographyType.of("srid:4269")); assertThat(Types.fromPrimitiveString("geography(srid:4269, spherical)")) - .isEqualTo( - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.SPHERICAL)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.SPHERICAL)); assertThat(Types.fromPrimitiveString("geography(srid:4269, vincenty)")) - .isEqualTo( - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.VINCENTY)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.VINCENTY)); assertThat(Types.fromPrimitiveString("geography(srid:4269, thomas)")) - .isEqualTo( - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.THOMAS)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.THOMAS)); assertThat(Types.fromPrimitiveString("geography(srid:4269, andoyer)")) - .isEqualTo( - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.ANDOYER)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.ANDOYER)); assertThat(Types.fromPrimitiveString("geography(srid:4269, karney)")) - .isEqualTo( - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geography(srid:4269, BadAlgorithm)")) @@ -128,13 +122,10 @@ public void testNestedFieldBuilderIdCheck() { assertThat(Types.fromPrimitiveString("geography( srid:4269 )")) .isEqualTo(Types.GeographyType.of("srid:4269")); assertThat(Types.fromPrimitiveString("geography( srid:4269 , spherical )")) - .isEqualTo( - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.SPHERICAL)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.SPHERICAL)); assertThat(Types.fromPrimitiveString("geography(srid:4269,vincenty)")) - .isEqualTo( - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.VINCENTY)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.VINCENTY)); assertThat(Types.fromPrimitiveString("geography( srid:4269 , karney )")) - .isEqualTo( - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); } } diff --git a/build.gradle b/build.gradle index c4944c8f2429..176c5479f207 100644 --- a/build.gradle +++ b/build.gradle @@ -292,7 +292,6 @@ project(':iceberg-api') { dependencies { implementation project(path: ':iceberg-bundled-guava', configuration: 'shadow') - api libs.jts.core compileOnly libs.errorprone.annotations compileOnly libs.findbugs.jsr305 testImplementation libs.avro.avro @@ -349,6 +348,7 @@ project(':iceberg-core') { implementation libs.jackson.databind implementation libs.caffeine implementation libs.roaringbitmap + implementation libs.jts.core compileOnly(libs.hadoop3.client) { exclude group: 'org.apache.avro', module: 'avro' exclude group: 'org.slf4j', module: 'slf4j-log4j12' diff --git a/core/src/main/java/org/apache/iceberg/SingleValueParser.java b/core/src/main/java/org/apache/iceberg/SingleValueParser.java index 1e3998e75a81..21990c88938e 100644 --- a/core/src/main/java/org/apache/iceberg/SingleValueParser.java +++ b/core/src/main/java/org/apache/iceberg/SingleValueParser.java @@ -168,10 +168,8 @@ public static Object fromJson(Type type, JsonNode defaultValue) { defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue); try { Geometry geom = GeometryUtil.fromWKT(defaultValue.textValue()); - if (type.typeId() == Type.TypeID.GEOGRAPHY) { - return new Geography(geom); - } - return geom; + byte[] wkb = GeometryUtil.toWKB(geom); + return ByteBuffer.wrap(wkb); } catch (Exception e) { throw new IllegalArgumentException( String.format("Cannot parse default as a %s value: %s", type, defaultValue), e); @@ -352,14 +350,11 @@ public static void toJson(Type type, Object defaultValue, JsonGenerator generato } break; case GEOMETRY: - Preconditions.checkArgument( - defaultValue instanceof Geometry, "Invalid default %s value: %s", type, defaultValue); - generator.writeString(GeometryUtil.toWKT((Geometry) defaultValue)); - break; case GEOGRAPHY: Preconditions.checkArgument( - defaultValue instanceof Geography, "Invalid default %s value: %s", type, defaultValue); - generator.writeString(GeometryUtil.toWKT(((Geography) defaultValue).geometry())); + defaultValue instanceof ByteBuffer, "Invalid default %s value: %s", type, defaultValue); + byte[] wkb = ByteBuffers.toByteArray((ByteBuffer) defaultValue); + generator.writeString(GeometryUtil.toWKT(GeometryUtil.fromWKB(wkb))); break; case LIST: Preconditions.checkArgument( diff --git a/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java b/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java index 504afc73ad74..003c6109aea4 100644 --- a/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java +++ b/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java @@ -27,6 +27,7 @@ import org.apache.iceberg.hadoop.HadoopCatalog; import org.apache.iceberg.hadoop.HadoopTableTestBase; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.types.EdgeInterpolationAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.junit.jupiter.api.Test; @@ -43,7 +44,7 @@ public void testCreateGeospatialTable() throws IOException { required( 6, "geog", - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY), + Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY), "geography column")); TableIdentifier identifier = TableIdentifier.of("a", "geos_t1"); @@ -61,7 +62,7 @@ public void testCreateGeospatialTable() throws IOException { assertThat(geogField.type().typeId()).isEqualTo(Type.TypeID.GEOGRAPHY); Types.GeographyType geogType = (Types.GeographyType) geogField.type(); assertThat(geogType.crs()).isEqualTo("srid:4269"); - assertThat(geogType.algorithm()).isEqualTo(Geography.EdgeInterpolationAlgorithm.KARNEY); + assertThat(geogType.algorithm()).isEqualTo(EdgeInterpolationAlgorithm.KARNEY); assertThat(catalog.dropTable(identifier)).isTrue(); } } diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaParser.java b/core/src/test/java/org/apache/iceberg/TestSchemaParser.java index 43c114319e4f..dc08681fad18 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaParser.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaParser.java @@ -31,10 +31,10 @@ import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Sets; +import org.apache.iceberg.types.EdgeInterpolationAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.DateTimeUtil; -import org.apache.iceberg.util.GeometryUtil; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -121,11 +121,7 @@ private static Stream primitiveTypesAndDefaults() { Types.FixedType.ofLength(4), Literal.of(ByteBuffer.wrap(new byte[] {0x0a, 0x0b, 0x0c, 0x0d}))), Arguments.of(Types.BinaryType.get(), Literal.of(ByteBuffer.wrap(new byte[] {0x0a, 0x0b}))), - Arguments.of(Types.DecimalType.of(9, 2), Literal.of(new BigDecimal("12.34"))), - Arguments.of(Types.GeometryType.get(), Literal.of(GeometryUtil.fromWKT("POINT (1 2)"))), - Arguments.of( - Types.GeographyType.get(), - Literal.of(new Geography(GeometryUtil.fromWKT("POINT (1 2)"))))); + Arguments.of(Types.DecimalType.of(9, 2), Literal.of(new BigDecimal("12.34")))); } @ParameterizedTest @@ -170,7 +166,7 @@ public void testSpatialType() throws IOException { Types.NestedField.optional( 6, "geog2", - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY))); + Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY))); writeAndValidate(schema); } diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java index 411e6eee1e15..147f0b6d214b 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java @@ -29,6 +29,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Sets; +import org.apache.iceberg.types.EdgeInterpolationAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.TypeUtil; import org.apache.iceberg.types.Types; @@ -368,7 +369,7 @@ public void testUpdateFailure() { Types.GeometryType.get(), Types.GeometryType.of("srid:3857"), Types.GeographyType.get(), - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY)); + Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); for (Type.PrimitiveType fromType : primitives) { for (Type.PrimitiveType toType : primitives) { diff --git a/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java b/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java index 3c5b3a9fc7d8..105089a815f2 100644 --- a/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java +++ b/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java @@ -25,6 +25,7 @@ import java.io.IOException; import java.util.Locale; +import org.apache.iceberg.types.EdgeInterpolationAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.JsonUtil; @@ -55,12 +56,11 @@ public void testValidDefaults() throws IOException { {Types.DecimalType.of(9, -20), "\"2E+20\""}, {Types.GeometryType.get(), "\"POINT (1 2)\""}, {Types.GeometryType.get(), "\"POINT Z(1 2 3)\""}, - {Types.GeometryType.get(), "\"POINT M(1 2 3)\""}, {Types.GeometryType.get(), "\"POINT ZM(1 2 3 4)\""}, {Types.GeometryType.of("srid:3857"), "\"POINT (1 2)\""}, {Types.GeographyType.get(), "\"POINT ZM(1 2 3 4)\""}, { - Types.GeographyType.of("srid:4269", Geography.EdgeInterpolationAlgorithm.KARNEY), + Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY), "\"POINT ZM(1 2 3 4)\"" }, {Types.ListType.ofOptional(1, Types.IntegerType.get()), "[1, 2, 3]"}, From c3b57f7389935d181c60f08bb33463c1de195e23 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Wed, 5 Mar 2025 17:42:14 +0800 Subject: [PATCH 06/16] Comment the estimated size of geometry and geography objects --- api/src/main/java/org/apache/iceberg/types/TypeUtil.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/api/src/main/java/org/apache/iceberg/types/TypeUtil.java b/api/src/main/java/org/apache/iceberg/types/TypeUtil.java index 990aa0834cf0..486b1d695b61 100644 --- a/api/src/main/java/org/apache/iceberg/types/TypeUtil.java +++ b/api/src/main/java/org/apache/iceberg/types/TypeUtil.java @@ -539,8 +539,12 @@ private static int estimateSize(Type type) { return ((Types.FixedType) type).length(); case BINARY: case VARIANT: + return 80; case GEOMETRY: case GEOGRAPHY: + // 80 bytes is an approximate size for a polygon or linestring with 4 to 5 coordinates. + // This is a reasonable estimate for the size of a geometry or geography object without + // additional details. return 80; case UNKNOWN: // Consider Unknown as null From eb1896ab92c2607bce647a2a73489c1a601e91e3 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Fri, 7 Mar 2025 17:29:02 +0800 Subject: [PATCH 07/16] Fixed problems according to review comments --- .../types/EdgeInterpolationAlgorithm.java | 4 +++- .../java/org/apache/iceberg/types/Types.java | 8 +++++-- .../java/org/apache/iceberg/TestSchema.java | 7 +++++- .../apache/iceberg/types/TestTypeUtil.java | 6 ++++- .../org/apache/iceberg/types/TestTypes.java | 23 +++++++++++-------- .../java/org/apache/iceberg/SchemaParser.java | 6 ++--- .../org/apache/iceberg/util/GeometryUtil.java | 4 +++- .../iceberg/TestSchemaUnionByFieldName.java | 9 +++++++- 8 files changed, 47 insertions(+), 20 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java b/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java index b15ffdcdcab5..10d3fd8c25d2 100644 --- a/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java +++ b/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java @@ -19,6 +19,7 @@ package org.apache.iceberg.types; import java.util.Locale; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; /** The algorithm for interpolating edges. */ public enum EdgeInterpolationAlgorithm { @@ -54,11 +55,12 @@ public String value() { } public static EdgeInterpolationAlgorithm fromName(String algorithmName) { + Preconditions.checkNotNull(algorithmName, "Edge interpolation algorithm cannot be null"); try { return EdgeInterpolationAlgorithm.valueOf(algorithmName.toUpperCase(Locale.ENGLISH)); } catch (IllegalArgumentException e) { throw new IllegalArgumentException( - String.format("Invalid edge interpolation algorithm name: %s", algorithmName), e); + String.format("Invalid edge interpolation algorithm: %s", algorithmName), e); } } } diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index d32fe1ac84f6..38ffb7741738 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -58,6 +58,8 @@ private Types() {} .put(BinaryType.get().toString(), BinaryType.get()) .put(UnknownType.get().toString(), UnknownType.get()) .put(VariantType.get().toString(), VariantType.get()) + .put(GeometryType.get().toString(), GeometryType.get()) + .put(GeographyType.get().toString(), GeographyType.get()) .buildOrThrow(); private static final Pattern FIXED = Pattern.compile("fixed\\[\\s*(\\d+)\\s*\\]"); @@ -566,11 +568,12 @@ 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 of(""); + return new GeometryType(""); } public static GeometryType of(String crs) { @@ -615,12 +618,13 @@ public static class GeographyType extends PrimitiveType { 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 of(""); + return new GeographyType("", null); } public static GeographyType of(String crs) { diff --git a/api/src/test/java/org/apache/iceberg/TestSchema.java b/api/src/test/java/org/apache/iceberg/TestSchema.java index 6b41baa4714f..e4f3d25496b1 100644 --- a/api/src/test/java/org/apache/iceberg/TestSchema.java +++ b/api/src/test/java/org/apache/iceberg/TestSchema.java @@ -29,6 +29,7 @@ import java.util.stream.Stream; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.types.EdgeInterpolationAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.junit.jupiter.api.Test; @@ -43,7 +44,11 @@ public class TestSchema { ImmutableList.of( Types.TimestampNanoType.withoutZone(), Types.TimestampNanoType.withZone(), - Types.VariantType.get()); + Types.VariantType.get(), + Types.GeometryType.get(), + Types.GeometryType.of("srid:3857"), + Types.GeographyType.get(), + Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); private static final Schema INITIAL_DEFAULT_SCHEMA = new Schema( diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java b/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java index 078c0180b5e7..5454e71ec1af 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java @@ -658,7 +658,11 @@ private static Stream testTypes() { Arguments.of(Types.UnknownType.get()), Arguments.of(Types.VariantType.get()), Arguments.of(Types.TimestampNanoType.withoutZone()), - Arguments.of(Types.TimestampNanoType.withZone())); + Arguments.of(Types.TimestampNanoType.withZone()), + Arguments.of(Types.GeometryType.get()), + Arguments.of(Types.GeometryType.of("srid:3857")), + Arguments.of(Types.GeographyType.get()), + Arguments.of(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY))); } @ParameterizedTest diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypes.java b/api/src/test/java/org/apache/iceberg/types/TestTypes.java index 2c32d6d402d9..f51a816a7dee 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypes.java @@ -82,15 +82,7 @@ public void fromPrimitiveString() { } @Test - public void testNestedFieldBuilderIdCheck() { - assertThatExceptionOfType(NullPointerException.class) - .isThrownBy(() -> optional("field").ofType(Types.StringType.get()).build()) - .withMessage("Id cannot be null"); - - assertThatExceptionOfType(NullPointerException.class) - .isThrownBy(() -> required("field").ofType(Types.StringType.get()).build()) - .withMessage("Id cannot be null"); - + public void geospatialTypeFromTypeName() { assertThat(Types.fromPrimitiveString("geometry")).isEqualTo(Types.GeometryType.get()); assertThat(Types.fromPrimitiveString("geometry()")).isEqualTo(Types.GeometryType.get()); assertThat(Types.fromPrimitiveString("geometry(srid:3857)")) @@ -115,7 +107,7 @@ public void testNestedFieldBuilderIdCheck() { assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geography(srid:4269, BadAlgorithm)")) - .withMessageContaining("Invalid edge interpolation algorithm name") + .withMessageContaining("Invalid edge interpolation algorithm") .withMessageContaining("BadAlgorithm"); // Test geography type with various spacing @@ -128,4 +120,15 @@ public void testNestedFieldBuilderIdCheck() { assertThat(Types.fromPrimitiveString("geography( srid:4269 , karney )")) .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); } + + @Test + public void testNestedFieldBuilderIdCheck() { + assertThatExceptionOfType(NullPointerException.class) + .isThrownBy(() -> optional("field").ofType(Types.StringType.get()).build()) + .withMessage("Id cannot be null"); + + assertThatExceptionOfType(NullPointerException.class) + .isThrownBy(() -> required("field").ofType(Types.StringType.get()).build()) + .withMessage("Id cannot be null"); + } } diff --git a/core/src/main/java/org/apache/iceberg/SchemaParser.java b/core/src/main/java/org/apache/iceberg/SchemaParser.java index 97fe0944d9dc..9549c77dc284 100644 --- a/core/src/main/java/org/apache/iceberg/SchemaParser.java +++ b/core/src/main/java/org/apache/iceberg/SchemaParser.java @@ -314,13 +314,13 @@ private static Types.MapType mapFromJson(JsonNode json) { } private static Types.GeometryType geometryFromJson(JsonNode json) { - String crs = JsonUtil.getStringOrNull("crs", json); + String crs = JsonUtil.getStringOrNull(CRS, json); return Types.GeometryType.of(crs); } private static Types.GeographyType geographyFromJson(JsonNode json) { - String crs = JsonUtil.getStringOrNull("crs", json); - String algorithm = JsonUtil.getStringOrNull("algorithm", json); + String crs = JsonUtil.getStringOrNull(CRS, json); + String algorithm = JsonUtil.getStringOrNull(ALGORITHM, json); return Types.GeographyType.of(crs, algorithm); } diff --git a/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java b/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java index da195229609b..38138cc20c5a 100644 --- a/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java +++ b/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java @@ -29,6 +29,8 @@ public class GeometryUtil { private GeometryUtil() {} + private static final int DEFAULT_DIMENSION = 2; + public static byte[] toWKB(Geometry geom) { WKBWriter wkbWriter = new WKBWriter(getOutputDimension(geom), false); return wkbWriter.write(geom); @@ -58,7 +60,7 @@ public static Geometry fromWKT(String wkt) { } private static int getOutputDimension(Geometry geom) { - int dimension = 2; + int dimension = DEFAULT_DIMENSION; Coordinate coordinate = geom.getCoordinate(); // We need to set outputDimension = 4 for XYM geometries to make JTS WKTWriter or WKBWriter work diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java b/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java index aa478f85260e..b6a0be9aba4e 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java @@ -27,6 +27,7 @@ import java.util.concurrent.atomic.AtomicInteger; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.collect.Lists; +import org.apache.iceberg.types.EdgeInterpolationAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.types.Types.BinaryType; @@ -36,6 +37,8 @@ import org.apache.iceberg.types.Types.DoubleType; import org.apache.iceberg.types.Types.FixedType; import org.apache.iceberg.types.Types.FloatType; +import org.apache.iceberg.types.Types.GeographyType; +import org.apache.iceberg.types.Types.GeometryType; import org.apache.iceberg.types.Types.IntegerType; import org.apache.iceberg.types.Types.ListType; import org.apache.iceberg.types.Types.LongType; @@ -71,7 +74,11 @@ private static List primitiveTypes() { VariantType.get(), UnknownType.get(), TimestampNanoType.withoutZone(), - TimestampNanoType.withZone()); + TimestampNanoType.withZone(), + GeometryType.get(), + GeometryType.of("srid:3857"), + GeographyType.get(), + GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); } private static NestedField[] primitiveFields( From e78a9c0c1801c457285fdd13a99c924b73803324 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Fri, 14 Mar 2025 22:10:02 +0800 Subject: [PATCH 08/16] Fix review comments --- .../apache/iceberg/transforms/Identity.java | 1 + ...ationAlgorithm.java => EdgeAlgorithm.java} | 26 +++---- .../java/org/apache/iceberg/types/Types.java | 65 +++++++++-------- .../iceberg/TestPartitionSpecValidation.java | 53 +++++--------- .../java/org/apache/iceberg/TestSchema.java | 8 +-- .../iceberg/transforms/TestBucketing.java | 12 ++-- .../iceberg/transforms/TestIdentity.java | 52 +++++--------- .../iceberg/types/TestReadabilityChecks.java | 6 +- .../iceberg/types/TestSerializableTypes.java | 6 +- .../apache/iceberg/types/TestTypeUtil.java | 6 +- .../org/apache/iceberg/types/TestTypes.java | 34 +++++---- .../java/org/apache/iceberg/SchemaParser.java | 14 +++- .../org/apache/iceberg/util/GeometryUtil.java | 7 +- .../apache/iceberg/TestGeospatialTable.java | 69 ------------------- .../org/apache/iceberg/TestSchemaParser.java | 23 ++----- .../iceberg/TestSchemaUnionByFieldName.java | 8 +-- .../org/apache/iceberg/TestSchemaUpdate.java | 8 +-- .../apache/iceberg/TestSingleValueParser.java | 19 +++-- .../org/apache/iceberg/TestSortOrder.java | 4 +- .../org/apache/iceberg/TestTableMetadata.java | 4 +- .../apache/iceberg/catalog/CatalogTests.java | 41 +++++++++++ .../org/apache/iceberg/data/DataTest.java | 45 ++++++++---- 22 files changed, 230 insertions(+), 281 deletions(-) rename api/src/main/java/org/apache/iceberg/types/{EdgeInterpolationAlgorithm.java => EdgeAlgorithm.java} (80%) delete mode 100644 core/src/test/java/org/apache/iceberg/TestGeospatialTable.java diff --git a/api/src/main/java/org/apache/iceberg/transforms/Identity.java b/api/src/main/java/org/apache/iceberg/transforms/Identity.java index c2a6483b4ef2..71b4fa165a45 100644 --- a/api/src/main/java/org/apache/iceberg/transforms/Identity.java +++ b/api/src/main/java/org/apache/iceberg/transforms/Identity.java @@ -101,6 +101,7 @@ public boolean canTransform(Type maybePrimitive) { if (UNSUPPORTED_TYPES.contains(maybePrimitive.typeId())) { return false; } + return maybePrimitive.isPrimitiveType(); } diff --git a/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java b/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java similarity index 80% rename from api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java rename to api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java index 10d3fd8c25d2..08cae564724d 100644 --- a/api/src/main/java/org/apache/iceberg/types/EdgeInterpolationAlgorithm.java +++ b/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java @@ -22,42 +22,32 @@ import org.apache.iceberg.relocated.com.google.common.base.Preconditions; /** The algorithm for interpolating edges. */ -public enum EdgeInterpolationAlgorithm { +public enum EdgeAlgorithm { /** Edges are interpolated as geodesics on a sphere. */ - SPHERICAL("spherical"), + SPHERICAL, /** See Vincenty's formulae */ - VINCENTY("vincenty"), + VINCENTY, /** * Thomas, Paul D. Spheroidal geodesics, reference systems, & local geometry. US Naval * Oceanographic Office, 1970. */ - THOMAS("thomas"), + THOMAS, /** * Thomas, Paul D. Mathematical models for navigation systems. US Naval Oceanographic Office, * 1965. */ - ANDOYER("andoyer"), + ANDOYER, /** * Karney, Charles * FF. "Algorithms for geodesics." Journal of Geodesy 87 (2013): 43-55 , and GeographicLib. */ - KARNEY("karney"); + KARNEY; - private final String value; - - EdgeInterpolationAlgorithm(String value) { - this.value = value; - } - - public String value() { - return value; - } - - public static EdgeInterpolationAlgorithm fromName(String algorithmName) { + public static EdgeAlgorithm fromName(String algorithmName) { Preconditions.checkNotNull(algorithmName, "Edge interpolation algorithm cannot be null"); try { - return EdgeInterpolationAlgorithm.valueOf(algorithmName.toUpperCase(Locale.ENGLISH)); + return EdgeAlgorithm.valueOf(algorithmName.toUpperCase(Locale.ENGLISH)); } catch (IllegalArgumentException e) { throw new IllegalArgumentException( String.format("Invalid edge interpolation algorithm: %s", algorithmName), e); diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index 38ffb7741738..e16f777caca8 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -31,6 +31,7 @@ import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.base.Joiner; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.base.Strings; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.types.Type.NestedType; @@ -58,15 +59,14 @@ private Types() {} .put(BinaryType.get().toString(), BinaryType.get()) .put(UnknownType.get().toString(), UnknownType.get()) .put(VariantType.get().toString(), VariantType.get()) - .put(GeometryType.get().toString(), GeometryType.get()) - .put(GeographyType.get().toString(), GeographyType.get()) + .put(GeometryType.crs84().toString(), GeometryType.crs84()) + .put(GeographyType.crs84().toString(), GeographyType.crs84()) .buildOrThrow(); private static final Pattern FIXED = Pattern.compile("fixed\\[\\s*(\\d+)\\s*\\]"); - private static final Pattern GEOMETRY_PARAMETERS = - Pattern.compile("(?:\\(\\s*([^, ]+)?\\s*\\))?"); + private static final Pattern GEOMETRY_PARAMETERS = Pattern.compile("(?:\\(\\s*([^,]*?)\\s*\\))?"); private static final Pattern GEOGRAPHY_PARAMETERS = - Pattern.compile("(?:\\(\\s*([^, ]+)?\\s*(?:,\\s*(\\w*)\\s*)?\\))?"); + Pattern.compile("(?:\\(\\s*([^,]+?)?\\s*(?:,\\s*(\\w*)\\s*)?\\))?"); private static final Pattern DECIMAL = Pattern.compile("decimal\\(\\s*(\\d+)\\s*,\\s*(\\d+)\\s*\\)"); @@ -86,7 +86,12 @@ public static Type fromTypeName(String typeString) { if (lowerTypeString.startsWith("geography")) { Matcher geography = GEOGRAPHY_PARAMETERS.matcher(typeString.substring(9)); if (geography.matches()) { - return GeographyType.of(geography.group(1), geography.group(2)); + String algorithmName = geography.group(2); + EdgeAlgorithm algorithm = + (algorithmName == null || algorithmName.isEmpty()) + ? null + : EdgeAlgorithm.fromName(algorithmName); + return GeographyType.of(geography.group(1), algorithm); } } @@ -568,16 +573,15 @@ public static class GeometryType extends PrimitiveType { private final String crs; private GeometryType(String crs) { - Preconditions.checkNotNull(crs, "CRS cannot be null"); - this.crs = crs; + this.crs = (crs == null || crs.isEmpty()) ? null : crs; } - public static GeometryType get() { - return new GeometryType(""); + public static GeometryType crs84() { + return new GeometryType(null); } public static GeometryType of(String crs) { - return new GeometryType(crs == null ? "" : crs); + return new GeometryType(crs); } @Override @@ -598,7 +602,7 @@ public boolean equals(Object o) { } GeometryType that = (GeometryType) o; - return crs.equals(that.crs); + return Objects.equals(crs, that.crs); } @Override @@ -608,6 +612,10 @@ public int hashCode() { @Override public String toString() { + if (Strings.isNullOrEmpty(crs)) { + return "geometry"; + } + return String.format("geometry(%s)", crs); } } @@ -615,34 +623,25 @@ public String toString() { public static class GeographyType extends PrimitiveType { private final String crs; - private final EdgeInterpolationAlgorithm algorithm; + private final EdgeAlgorithm algorithm; - private GeographyType(String crs, EdgeInterpolationAlgorithm algorithm) { - Preconditions.checkNotNull(crs, "CRS cannot be null"); - this.crs = crs; + private GeographyType(String crs, EdgeAlgorithm algorithm) { + this.crs = (crs == null || crs.isEmpty()) ? null : crs; this.algorithm = algorithm; } - public static GeographyType get() { - return new GeographyType("", null); + public static GeographyType crs84() { + return new GeographyType(null, null); } public static GeographyType of(String crs) { return new GeographyType(crs, null); } - public static GeographyType of(String crs, EdgeInterpolationAlgorithm algorithm) { + public static GeographyType of(String crs, EdgeAlgorithm algorithm) { return new GeographyType(crs, algorithm); } - public static GeographyType of(String crs, String algorithmName) { - EdgeInterpolationAlgorithm algorithm = - ((algorithmName == null || algorithmName.isEmpty()) - ? null - : EdgeInterpolationAlgorithm.fromName(algorithmName)); - return new GeographyType(crs == null ? "" : crs, algorithm); - } - @Override public TypeID typeId() { return TypeID.GEOGRAPHY; @@ -652,7 +651,7 @@ public String crs() { return crs; } - public EdgeInterpolationAlgorithm algorithm() { + public EdgeAlgorithm algorithm() { return algorithm; } @@ -675,7 +674,15 @@ public int hashCode() { @Override public String toString() { - return String.format("geography(%s, %s)", crs, algorithm != null ? algorithm.value() : ""); + if (algorithm != null) { + return String.format( + "geography(%s, %s)", + crs != null ? crs : "", algorithm.name().toLowerCase(Locale.ENGLISH)); + } else if (!Strings.isNullOrEmpty(crs)) { + return String.format("geography(%s)", crs); + } else { + return "geography"; + } } } diff --git a/api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java b/api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java index 1e2436e648cc..ee71d39bb2db 100644 --- a/api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java +++ b/api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java @@ -26,6 +26,8 @@ import org.apache.iceberg.types.Types; import org.apache.iceberg.types.Types.NestedField; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; public class TestPartitionSpecValidation { private static final Schema SCHEMA = @@ -37,8 +39,8 @@ public class TestPartitionSpecValidation { NestedField.required(5, "another_d", Types.TimestampType.withZone()), NestedField.required(6, "s", Types.StringType.get()), NestedField.required(7, "v", Types.VariantType.get()), - NestedField.required(8, "geom", Types.GeometryType.get()), - NestedField.required(9, "geog", Types.GeographyType.get()), + NestedField.required(8, "geom", Types.GeometryType.crs84()), + NestedField.required(9, "geog", Types.GeographyType.crs84()), NestedField.optional(10, "u", Types.UnknownType.get())); @Test @@ -318,47 +320,24 @@ public void testAddPartitionFieldsWithAndWithoutFieldIds() { assertThat(spec.lastAssignedFieldId()).isEqualTo(1006); } - @Test - public void testVariantUnsupported() { - assertThatThrownBy( - () -> - PartitionSpec.builderFor(SCHEMA) - .add(7, 1005, "variant_partition1", Transforms.bucket(5)) - .build()) - .isInstanceOf(ValidationException.class) - .hasMessage("Cannot partition by non-primitive source field: variant"); - } - - @Test - public void testGeometryUnsupported() { + @ParameterizedTest + @MethodSource("unsupportedFieldsProvider") + public void testUnsupported(int fieldId, String partitionName, String expectedErrorMessage) { assertThatThrownBy( () -> PartitionSpec.builderFor(SCHEMA) - .add(8, 1005, "geom_partition1", Transforms.bucket(5)) + .add(fieldId, 1005, partitionName, Transforms.bucket(5)) .build()) .isInstanceOf(ValidationException.class) - .hasMessageMatching("Invalid source type geometry.* for transform: bucket.*"); + .hasMessage(expectedErrorMessage); } - @Test - public void testGeographyUnsupported() { - assertThatThrownBy( - () -> - PartitionSpec.builderFor(SCHEMA) - .add(9, 1005, "geog_partition1", Transforms.bucket(5)) - .build()) - .isInstanceOf(ValidationException.class) - .hasMessageMatching("Invalid source type geography.* for transform: bucket.*"); - } - - @Test - public void testUnknownUnsupported() { - assertThatThrownBy( - () -> - PartitionSpec.builderFor(SCHEMA) - .add(10, 1005, "unknown_partition1", Transforms.bucket(5)) - .build()) - .isInstanceOf(ValidationException.class) - .hasMessage("Invalid source type unknown for transform: bucket[5]"); + private static Object[][] unsupportedFieldsProvider() { + return new Object[][] { + {7, "variant_partition1", "Cannot partition by non-primitive source field: variant"}, + {8, "geom_partition1", "Invalid source type geometry for transform: bucket[5]"}, + {9, "geog_partition1", "Invalid source type geography for transform: bucket[5]"}, + {10, "unknown_partition1", "Invalid source type unknown for transform: bucket[5]"} + }; } } diff --git a/api/src/test/java/org/apache/iceberg/TestSchema.java b/api/src/test/java/org/apache/iceberg/TestSchema.java index e4f3d25496b1..4b164f963d4a 100644 --- a/api/src/test/java/org/apache/iceberg/TestSchema.java +++ b/api/src/test/java/org/apache/iceberg/TestSchema.java @@ -29,7 +29,7 @@ import java.util.stream.Stream; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; -import org.apache.iceberg.types.EdgeInterpolationAlgorithm; +import org.apache.iceberg.types.EdgeAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.junit.jupiter.api.Test; @@ -45,10 +45,10 @@ public class TestSchema { Types.TimestampNanoType.withoutZone(), Types.TimestampNanoType.withZone(), Types.VariantType.get(), - Types.GeometryType.get(), + Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), - Types.GeographyType.get(), - Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); + Types.GeographyType.crs84(), + Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); private static final Schema INITIAL_DEFAULT_SCHEMA = new Schema( diff --git a/api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java b/api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java index 574b6df513bd..81f4fa6098e2 100644 --- a/api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java +++ b/api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java @@ -433,30 +433,30 @@ public void testVariantUnsupported() { @Test public void testGeometryUnsupported() { - assertThatThrownBy(() -> Transforms.bucket(Types.GeometryType.get(), 3)) + assertThatThrownBy(() -> Transforms.bucket(Types.GeometryType.crs84(), 3)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("Cannot bucket by type: geometry"); Transform bucket = Transforms.bucket(3); - assertThatThrownBy(() -> bucket.bind(Types.GeometryType.get())) + assertThatThrownBy(() -> bucket.bind(Types.GeometryType.crs84())) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("Cannot bucket by type: geometry"); - assertThat(bucket.canTransform(Types.GeometryType.get())).isFalse(); + assertThat(bucket.canTransform(Types.GeometryType.crs84())).isFalse(); } @Test public void testGeographyUnsupported() { - assertThatThrownBy(() -> Transforms.bucket(Types.GeographyType.get(), 3)) + assertThatThrownBy(() -> Transforms.bucket(Types.GeographyType.crs84(), 3)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("Cannot bucket by type: geography"); Transform bucket = Transforms.bucket(3); - assertThatThrownBy(() -> bucket.bind(Types.GeographyType.get())) + assertThatThrownBy(() -> bucket.bind(Types.GeographyType.crs84())) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("Cannot bucket by type: geography"); - assertThat(bucket.canTransform(Types.GeographyType.get())).isFalse(); + assertThat(bucket.canTransform(Types.GeographyType.crs84())).isFalse(); } @Test diff --git a/api/src/test/java/org/apache/iceberg/transforms/TestIdentity.java b/api/src/test/java/org/apache/iceberg/transforms/TestIdentity.java index a226ed5f17ab..62e5418ee204 100644 --- a/api/src/test/java/org/apache/iceberg/transforms/TestIdentity.java +++ b/api/src/test/java/org/apache/iceberg/transforms/TestIdentity.java @@ -24,8 +24,11 @@ import java.math.BigDecimal; import java.nio.ByteBuffer; import org.apache.iceberg.expressions.Literal; +import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; public class TestIdentity { @Test @@ -167,48 +170,27 @@ public void testUnknownToHumanString() { .isEqualTo("null"); } - @Test - public void testVariantUnsupported() { - assertThatThrownBy(() -> Transforms.identity().bind(Types.VariantType.get())) + @ParameterizedTest + @MethodSource("unsupportedTypesProvider") + public void testUnsupported(Type type) { + assertThatThrownBy(() -> Transforms.identity().bind(type)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Cannot bind to unsupported type: variant"); + .hasMessage("Cannot bind to unsupported type: " + type); - assertThatThrownBy(() -> Transforms.fromString(Types.VariantType.get(), "identity")) + assertThatThrownBy(() -> Transforms.fromString(type, "identity")) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Unsupported type for identity: variant"); + .hasMessage("Unsupported type for identity: " + type); - assertThatThrownBy(() -> Transforms.identity(Types.VariantType.get())) + assertThatThrownBy(() -> Transforms.identity(type)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Unsupported type for identity: variant"); + .hasMessage("Unsupported type for identity: " + type); - assertThat(Transforms.identity().canTransform(Types.VariantType.get())).isFalse(); + assertThat(Transforms.identity().canTransform(type)).isFalse(); } - @Test - public void testGeometryUnsupported() { - assertThatThrownBy(() -> Transforms.identity().bind(Types.GeometryType.get())) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Cannot bind to unsupported type: geometry"); - assertThatThrownBy(() -> Transforms.fromString(Types.GeometryType.get(), "identity")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Unsupported type for identity: geometry"); - assertThatThrownBy(() -> Transforms.identity(Types.GeometryType.get())) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Unsupported type for identity: geometry"); - assertThat(Transforms.identity().canTransform(Types.GeometryType.get())).isFalse(); - } - - @Test - public void testGeographyUnsupported() { - assertThatThrownBy(() -> Transforms.identity().bind(Types.GeographyType.get())) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Cannot bind to unsupported type: geography"); - assertThatThrownBy(() -> Transforms.fromString(Types.GeographyType.get(), "identity")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Unsupported type for identity: geography"); - assertThatThrownBy(() -> Transforms.identity(Types.GeographyType.get())) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Unsupported type for identity: geography"); - assertThat(Transforms.identity().canTransform(Types.GeographyType.get())).isFalse(); + private static Type[] unsupportedTypesProvider() { + return new Type[] { + Types.VariantType.get(), Types.GeometryType.crs84(), Types.GeographyType.crs84() + }; } } diff --git a/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java b/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java index f251bd93264c..20299cdafce2 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java +++ b/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java @@ -54,11 +54,11 @@ public class TestReadabilityChecks { Types.DecimalType.of(9, 2), Types.DecimalType.of(11, 2), Types.DecimalType.of(9, 3), - Types.GeometryType.get(), + Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), - Types.GeographyType.get(), + Types.GeographyType.crs84(), Types.GeographyType.of("srid:4269"), - Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY), + Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), }; @Test diff --git a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java index 34259c1cdaf0..5cf58c8e33f6 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java @@ -64,13 +64,13 @@ public void testEqualTypes() throws Exception { Types.DecimalType.of(11, 0), Types.FixedType.ofLength(4), Types.FixedType.ofLength(34), - Types.GeometryType.get(), + Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), Types.GeometryType.of("projjson:Test_Identifier"), - Types.GeographyType.get(), + Types.GeographyType.crs84(), Types.GeographyType.of("srid:4269"), Types.GeographyType.of("projjson:Test_Identifier"), - Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY), + Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), }; for (Type type : equalityPrimitives) { diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java b/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java index 5454e71ec1af..a41ff0d7d2c5 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java @@ -659,10 +659,10 @@ private static Stream testTypes() { Arguments.of(Types.VariantType.get()), Arguments.of(Types.TimestampNanoType.withoutZone()), Arguments.of(Types.TimestampNanoType.withZone()), - Arguments.of(Types.GeometryType.get()), + Arguments.of(Types.GeometryType.crs84()), Arguments.of(Types.GeometryType.of("srid:3857")), - Arguments.of(Types.GeographyType.get()), - Arguments.of(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY))); + Arguments.of(Types.GeographyType.crs84()), + Arguments.of(Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY))); } @ParameterizedTest diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypes.java b/api/src/test/java/org/apache/iceberg/types/TestTypes.java index f51a816a7dee..3ceabe0da1d4 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypes.java @@ -83,27 +83,37 @@ public void fromPrimitiveString() { @Test public void geospatialTypeFromTypeName() { - assertThat(Types.fromPrimitiveString("geometry")).isEqualTo(Types.GeometryType.get()); - assertThat(Types.fromPrimitiveString("geometry()")).isEqualTo(Types.GeometryType.get()); + assertThat(Types.fromPrimitiveString("geometry")).isEqualTo(Types.GeometryType.crs84()); + assertThat(Types.fromPrimitiveString("geometry()")).isEqualTo(Types.GeometryType.crs84()); assertThat(Types.fromPrimitiveString("geometry(srid:3857)")) .isEqualTo(Types.GeometryType.of("srid:3857")); assertThat(Types.fromPrimitiveString("geometry( srid:3857 )")) .isEqualTo(Types.GeometryType.of("srid:3857")); + assertThat(Types.fromPrimitiveString("geometry( srid: 3857 )")) + .isEqualTo(Types.GeometryType.of("srid: 3857")); + assertThat(Types.fromPrimitiveString("geometry( projjson:TestIdentifier )")) + .isEqualTo(Types.GeometryType.of("projjson:TestIdentifier")); - assertThat(Types.fromPrimitiveString("geography")).isEqualTo(Types.GeographyType.get()); - assertThat(Types.fromPrimitiveString("geography()")).isEqualTo(Types.GeographyType.get()); + assertThat(Types.fromPrimitiveString("geography")).isEqualTo(Types.GeographyType.crs84()); + assertThat(Types.fromPrimitiveString("geography()")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromPrimitiveString("geography(srid:4269)")) .isEqualTo(Types.GeographyType.of("srid:4269")); + assertThat(Types.fromPrimitiveString("geography(srid: 4269)")) + .isEqualTo(Types.GeographyType.of("srid: 4269")); assertThat(Types.fromPrimitiveString("geography(srid:4269, spherical)")) - .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.SPHERICAL)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.SPHERICAL)); assertThat(Types.fromPrimitiveString("geography(srid:4269, vincenty)")) - .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.VINCENTY)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.VINCENTY)); assertThat(Types.fromPrimitiveString("geography(srid:4269, thomas)")) - .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.THOMAS)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.THOMAS)); assertThat(Types.fromPrimitiveString("geography(srid:4269, andoyer)")) - .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.ANDOYER)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.ANDOYER)); assertThat(Types.fromPrimitiveString("geography(srid:4269, karney)")) - .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); + assertThat(Types.fromPrimitiveString("geography(srid: 4269, karney)")) + .isEqualTo(Types.GeographyType.of("srid: 4269", EdgeAlgorithm.KARNEY)); + assertThat(Types.fromPrimitiveString("geography(projjson: TestIdentifier, karney)")) + .isEqualTo(Types.GeographyType.of("projjson: TestIdentifier", EdgeAlgorithm.KARNEY)); assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geography(srid:4269, BadAlgorithm)")) @@ -114,11 +124,11 @@ public void geospatialTypeFromTypeName() { assertThat(Types.fromPrimitiveString("geography( srid:4269 )")) .isEqualTo(Types.GeographyType.of("srid:4269")); assertThat(Types.fromPrimitiveString("geography( srid:4269 , spherical )")) - .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.SPHERICAL)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.SPHERICAL)); assertThat(Types.fromPrimitiveString("geography(srid:4269,vincenty)")) - .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.VINCENTY)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.VINCENTY)); assertThat(Types.fromPrimitiveString("geography( srid:4269 , karney )")) - .isEqualTo(Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); } @Test diff --git a/core/src/main/java/org/apache/iceberg/SchemaParser.java b/core/src/main/java/org/apache/iceberg/SchemaParser.java index 9549c77dc284..83e0c7235a15 100644 --- a/core/src/main/java/org/apache/iceberg/SchemaParser.java +++ b/core/src/main/java/org/apache/iceberg/SchemaParser.java @@ -29,7 +29,9 @@ import org.apache.iceberg.expressions.Expressions; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.base.Strings; import org.apache.iceberg.relocated.com.google.common.collect.Lists; +import org.apache.iceberg.types.EdgeAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.JsonUtil; @@ -150,9 +152,10 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws Types.GeometryType geometryType = (Types.GeometryType) primitive; generator.writeStartObject(); generator.writeStringField(TYPE, GEOMETRY); - if (!geometryType.crs().isEmpty()) { + if (!Strings.isNullOrEmpty(geometryType.crs())) { generator.writeStringField(CRS, geometryType.crs()); } + generator.writeEndObject(); break; @@ -160,12 +163,13 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws Types.GeographyType geographyType = (Types.GeographyType) primitive; generator.writeStartObject(); generator.writeStringField(TYPE, GEOGRAPHY); - if (!geographyType.crs().isEmpty()) { + if (!Strings.isNullOrEmpty(geographyType.crs())) { generator.writeStringField(CRS, geographyType.crs()); } if (geographyType.algorithm() != null) { generator.writeStringField(ALGORITHM, geographyType.algorithm().name()); } + generator.writeEndObject(); break; @@ -320,7 +324,11 @@ private static Types.GeometryType geometryFromJson(JsonNode json) { private static Types.GeographyType geographyFromJson(JsonNode json) { String crs = JsonUtil.getStringOrNull(CRS, json); - String algorithm = JsonUtil.getStringOrNull(ALGORITHM, json); + String algorithmName = JsonUtil.getStringOrNull(ALGORITHM, json); + EdgeAlgorithm algorithm = + ((algorithmName == null || algorithmName.isEmpty()) + ? null + : EdgeAlgorithm.fromName(algorithmName)); return Types.GeographyType.of(crs, algorithm); } diff --git a/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java b/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java index 38138cc20c5a..40c691c76b3d 100644 --- a/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java +++ b/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java @@ -32,12 +32,12 @@ private GeometryUtil() {} private static final int DEFAULT_DIMENSION = 2; public static byte[] toWKB(Geometry geom) { - WKBWriter wkbWriter = new WKBWriter(getOutputDimension(geom), false); + WKBWriter wkbWriter = new WKBWriter(outputDimension(geom), false); return wkbWriter.write(geom); } public static String toWKT(Geometry geom) { - WKTWriter wktWriter = new WKTWriter(getOutputDimension(geom)); + WKTWriter wktWriter = new WKTWriter(outputDimension(geom)); return wktWriter.write(geom); } @@ -59,7 +59,7 @@ public static Geometry fromWKT(String wkt) { } } - private static int getOutputDimension(Geometry geom) { + private static int outputDimension(Geometry geom) { int dimension = DEFAULT_DIMENSION; Coordinate coordinate = geom.getCoordinate(); @@ -72,6 +72,7 @@ private static int getOutputDimension(Geometry geom) { if (!Double.isNaN(coordinate.getM())) { dimension = 4; } + return dimension; } } diff --git a/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java b/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java deleted file mode 100644 index 003c6109aea4..000000000000 --- a/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java +++ /dev/null @@ -1,69 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.iceberg; - -import static org.apache.iceberg.types.Types.NestedField.required; -import static org.assertj.core.api.Assertions.assertThat; - -import java.io.IOException; -import java.util.Map; -import org.apache.iceberg.catalog.TableIdentifier; -import org.apache.iceberg.hadoop.HadoopCatalog; -import org.apache.iceberg.hadoop.HadoopTableTestBase; -import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; -import org.apache.iceberg.types.EdgeInterpolationAlgorithm; -import org.apache.iceberg.types.Type; -import org.apache.iceberg.types.Types; -import org.junit.jupiter.api.Test; - -public class TestGeospatialTable extends HadoopTableTestBase { - - @Test - public void testCreateGeospatialTable() throws IOException { - Schema schema = - new Schema( - required(3, "id", Types.IntegerType.get(), "unique ID"), - required(4, "data", Types.StringType.get()), - required(5, "geom", Types.GeometryType.of("srid:3857"), "geometry column"), - required( - 6, - "geog", - Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY), - "geography column")); - - TableIdentifier identifier = TableIdentifier.of("a", "geos_t1"); - try (HadoopCatalog catalog = hadoopCatalog()) { - Map properties = ImmutableMap.of(TableProperties.FORMAT_VERSION, "3"); - catalog.createTable(identifier, schema, PartitionSpec.unpartitioned(), properties); - Table table = catalog.loadTable(identifier); - - Types.NestedField geomField = table.schema().findField("geom"); - assertThat(geomField.type().typeId()).isEqualTo(Type.TypeID.GEOMETRY); - Types.GeometryType geomType = (Types.GeometryType) geomField.type(); - assertThat(geomType.crs()).isEqualTo("srid:3857"); - - Types.NestedField geogField = table.schema().findField("geog"); - assertThat(geogField.type().typeId()).isEqualTo(Type.TypeID.GEOGRAPHY); - Types.GeographyType geogType = (Types.GeographyType) geogField.type(); - assertThat(geogType.crs()).isEqualTo("srid:4269"); - assertThat(geogType.algorithm()).isEqualTo(EdgeInterpolationAlgorithm.KARNEY); - assertThat(catalog.dropTable(identifier)).isTrue(); - } - } -} diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaParser.java b/core/src/test/java/org/apache/iceberg/TestSchemaParser.java index dc08681fad18..1c1f906ab012 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaParser.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaParser.java @@ -31,7 +31,6 @@ import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Sets; -import org.apache.iceberg.types.EdgeInterpolationAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.DateTimeUtil; @@ -56,6 +55,11 @@ protected boolean supportsVariant() { return true; } + @Override + protected boolean supportsGeospatial() { + return true; + } + @Override protected void writeAndValidate(Schema schema) throws IOException { Schema serialized = SchemaParser.fromJson(SchemaParser.toJson(schema)); @@ -153,21 +157,4 @@ public void testVariantType() throws IOException { writeAndValidate(schema); } - - @Test - public void testSpatialType() throws IOException { - Schema schema = - new Schema( - Types.NestedField.required(1, "id", Types.IntegerType.get()), - Types.NestedField.optional(2, "geom0", Types.GeometryType.get()), - Types.NestedField.optional(3, "geom1", Types.GeometryType.of("srid:3857")), - Types.NestedField.optional(4, "geog0", Types.GeographyType.get()), - Types.NestedField.optional(5, "geog1", Types.GeographyType.of("srid:4269")), - Types.NestedField.optional( - 6, - "geog2", - Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY))); - - writeAndValidate(schema); - } } diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java b/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java index b6a0be9aba4e..a9255e4125fb 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java @@ -27,7 +27,7 @@ import java.util.concurrent.atomic.AtomicInteger; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.collect.Lists; -import org.apache.iceberg.types.EdgeInterpolationAlgorithm; +import org.apache.iceberg.types.EdgeAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.types.Types.BinaryType; @@ -75,10 +75,10 @@ private static List primitiveTypes() { UnknownType.get(), TimestampNanoType.withoutZone(), TimestampNanoType.withZone(), - GeometryType.get(), + GeometryType.crs84(), GeometryType.of("srid:3857"), - GeographyType.get(), - GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); + GeographyType.crs84(), + GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); } private static NestedField[] primitiveFields( diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java index 147f0b6d214b..4bd3d89946d0 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaUpdate.java @@ -29,7 +29,7 @@ import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.relocated.com.google.common.collect.Sets; -import org.apache.iceberg.types.EdgeInterpolationAlgorithm; +import org.apache.iceberg.types.EdgeAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.TypeUtil; import org.apache.iceberg.types.Types; @@ -366,10 +366,10 @@ public void testUpdateFailure() { Types.DecimalType.of(9, 2), Types.DecimalType.of(9, 3), Types.DecimalType.of(18, 2), - Types.GeometryType.get(), + Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), - Types.GeographyType.get(), - Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY)); + Types.GeographyType.crs84(), + Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); for (Type.PrimitiveType fromType : primitives) { for (Type.PrimitiveType toType : primitives) { diff --git a/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java b/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java index 105089a815f2..b81badab7282 100644 --- a/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java +++ b/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java @@ -25,7 +25,7 @@ import java.io.IOException; import java.util.Locale; -import org.apache.iceberg.types.EdgeInterpolationAlgorithm; +import org.apache.iceberg.types.EdgeAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.JsonUtil; @@ -54,15 +54,12 @@ public void testValidDefaults() throws IOException { {Types.DecimalType.of(9, 4), "\"123.4500\""}, {Types.DecimalType.of(9, 0), "\"2\""}, {Types.DecimalType.of(9, -20), "\"2E+20\""}, - {Types.GeometryType.get(), "\"POINT (1 2)\""}, - {Types.GeometryType.get(), "\"POINT Z(1 2 3)\""}, - {Types.GeometryType.get(), "\"POINT ZM(1 2 3 4)\""}, + {Types.GeometryType.crs84(), "\"POINT (1 2)\""}, + {Types.GeometryType.crs84(), "\"POINT Z(1 2 3)\""}, + {Types.GeometryType.crs84(), "\"POINT ZM(1 2 3 4)\""}, {Types.GeometryType.of("srid:3857"), "\"POINT (1 2)\""}, - {Types.GeographyType.get(), "\"POINT ZM(1 2 3 4)\""}, - { - Types.GeographyType.of("srid:4269", EdgeInterpolationAlgorithm.KARNEY), - "\"POINT ZM(1 2 3 4)\"" - }, + {Types.GeographyType.crs84(), "\"POINT ZM(1 2 3 4)\""}, + {Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), "\"POINT ZM(1 2 3 4)\""}, {Types.ListType.ofOptional(1, Types.IntegerType.get()), "[1, 2, 3]"}, { Types.MapType.ofOptional(2, 3, Types.IntegerType.get(), Types.StringType.get()), @@ -169,11 +166,11 @@ public void testInvalidTimestamptz() { @Test public void testInvalidGeometry() { - Type expectedType = Types.GeometryType.get(); + Type expectedType = Types.GeometryType.crs84(); String defaultJson = "\"POINT (1 2 3 4 5 6)\""; assertThatThrownBy(() -> defaultValueParseAndUnParseRoundTrip(expectedType, defaultJson)) .isInstanceOf(IllegalArgumentException.class) - .hasMessageMatching("Cannot parse default as a geometry.* value.*"); + .hasMessageStartingWith("Cannot parse default as a geometry value"); } // serialize to json and deserialize back should return the same result diff --git a/core/src/test/java/org/apache/iceberg/TestSortOrder.java b/core/src/test/java/org/apache/iceberg/TestSortOrder.java index b8e570c1c8fc..521320ea23db 100644 --- a/core/src/test/java/org/apache/iceberg/TestSortOrder.java +++ b/core/src/test/java/org/apache/iceberg/TestSortOrder.java @@ -348,8 +348,8 @@ public void testGeospatialUnsupported() { Schema v3Schema = new Schema( Types.NestedField.required(3, "id", Types.LongType.get()), - Types.NestedField.required(4, "geom", Types.GeometryType.get()), - Types.NestedField.required(5, "geog", Types.GeographyType.get())); + Types.NestedField.required(4, "geom", Types.GeometryType.crs84()), + Types.NestedField.required(5, "geog", Types.GeographyType.crs84())); assertThatThrownBy(() -> SortOrder.builderFor(v3Schema).withOrderId(10).asc("geom").build()) .isInstanceOf(IllegalArgumentException.class) diff --git a/core/src/test/java/org/apache/iceberg/TestTableMetadata.java b/core/src/test/java/org/apache/iceberg/TestTableMetadata.java index 59f4aab3d8c2..92c2796cbfa6 100644 --- a/core/src/test/java/org/apache/iceberg/TestTableMetadata.java +++ b/core/src/test/java/org/apache/iceberg/TestTableMetadata.java @@ -1830,11 +1830,11 @@ public void testV3GeometryTypeSupport() { Schema v3SchemaGeom = new Schema( Types.NestedField.required(3, "id", Types.LongType.get()), - Types.NestedField.required(4, "geom", Types.GeometryType.get())); + Types.NestedField.required(4, "geom", Types.GeometryType.crs84())); Schema v3SchemaGeog = new Schema( Types.NestedField.required(3, "id", Types.LongType.get()), - Types.NestedField.required(4, "geog", Types.GeographyType.get())); + Types.NestedField.required(4, "geog", Types.GeographyType.crs84())); for (Schema schema : ImmutableList.of(v3SchemaGeom, v3SchemaGeog)) { for (int unsupportedFormatVersion : ImmutableList.of(1, 2)) { diff --git a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java index b7cfd9b16be4..a0bf2e468b2b 100644 --- a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java +++ b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java @@ -75,6 +75,8 @@ import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.relocated.com.google.common.collect.Streams; +import org.apache.iceberg.types.EdgeAlgorithm; +import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.CharSequenceSet; import org.junit.jupiter.api.Assumptions; @@ -536,6 +538,45 @@ public void testBasicCreateTable() { assertThat(table.properties()).as("Should have table properties").isNotNull(); } + @Test + public void testCreateGeospatialTable() { + Schema schema = + new Schema( + required(3, "id", Types.IntegerType.get(), "unique ID"), + required(4, "data", Types.StringType.get()), + required(5, "geom", Types.GeometryType.of("srid:3857"), "geometry column"), + required( + 6, + "geog", + Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), + "geography column")); + + TableIdentifier identifier = TableIdentifier.of("ns", "geos_table"); + + C catalog = catalog(); + assertThat(catalog.tableExists(identifier)).as("Table should not exist").isFalse(); + + if (requiresNamespaceCreate()) { + catalog.createNamespace(identifier.namespace()); + } + + Map properties = ImmutableMap.of(TableProperties.FORMAT_VERSION, "3"); + catalog.createTable(identifier, schema, PartitionSpec.unpartitioned(), properties); + Table table = catalog.loadTable(identifier); + + Types.NestedField geomField = table.schema().findField("geom"); + assertThat(geomField.type().typeId()).isEqualTo(Type.TypeID.GEOMETRY); + Types.GeometryType geomType = (Types.GeometryType) geomField.type(); + assertThat(geomType.crs()).isEqualTo("srid:3857"); + + Types.NestedField geogField = table.schema().findField("geog"); + assertThat(geogField.type().typeId()).isEqualTo(Type.TypeID.GEOGRAPHY); + Types.GeographyType geogType = (Types.GeographyType) geogField.type(); + assertThat(geogType.crs()).isEqualTo("srid:4269"); + assertThat(geogType.algorithm()).isEqualTo(EdgeAlgorithm.KARNEY); + assertThat(catalog.dropTable(identifier)).isTrue(); + } + @Test public void testTableNameWithSlash() { Assumptions.assumeTrue(supportsNamesWithSlashes()); diff --git a/core/src/test/java/org/apache/iceberg/data/DataTest.java b/core/src/test/java/org/apache/iceberg/data/DataTest.java index 23ea8d879297..dc89f6e0996d 100644 --- a/core/src/test/java/org/apache/iceberg/data/DataTest.java +++ b/core/src/test/java/org/apache/iceberg/data/DataTest.java @@ -34,6 +34,7 @@ import org.apache.iceberg.Schema; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.types.EdgeAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.TypeUtil; import org.apache.iceberg.types.Types; @@ -115,6 +116,11 @@ protected boolean allowsWritingNullValuesForRequiredFields() { Types.DecimalType.of(11, 2), Types.DecimalType.of(38, 10), Types.VariantType.get(), + Types.GeometryType.crs84(), + Types.GeometryType.of("srid:3857"), + Types.GeographyType.crs84(), + Types.GeographyType.of("srid:4269"), + Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), }; protected boolean supportsUnknown() { @@ -129,28 +135,37 @@ protected boolean supportsVariant() { return false; } + protected boolean supportsGeospatial() { + return false; + } + @ParameterizedTest @FieldSource("SIMPLE_TYPES") public void testTypeSchema(Type type) throws IOException { - Assumptions.assumeThat( - supportsUnknown() - || TypeUtil.find(type, t -> t.typeId() == Type.TypeID.UNKNOWN) == null) - .as("unknown is not yet implemented") - .isTrue(); - Assumptions.assumeThat( - supportsTimestampNanos() - || TypeUtil.find(type, t -> t.typeId() == Type.TypeID.TIMESTAMP_NANO) == null) - .as("timestamp_ns is not yet implemented") - .isTrue(); - Assumptions.assumeThat( - supportsVariant() - || TypeUtil.find(type, t -> t.typeId() == Type.TypeID.VARIANT) == null) - .as("variant is not yet implemented") - .isTrue(); + if (!supportsUnknown()) { + assumeNoUnsupportedType(type, Type.TypeID.UNKNOWN, "unknown"); + } + if (!supportsTimestampNanos()) { + assumeNoUnsupportedType(type, Type.TypeID.TIMESTAMP_NANO, "timestamp_ns"); + } + if (!supportsVariant()) { + assumeNoUnsupportedType(type, Type.TypeID.VARIANT, "variant"); + } + if (!supportsGeospatial()) { + assumeNoUnsupportedType(type, Type.TypeID.GEOMETRY, "geometry"); + assumeNoUnsupportedType(type, Type.TypeID.GEOGRAPHY, "geography"); + } writeAndValidate(new Schema(required(1, "id", LongType.get()), optional(2, "test_type", type))); } + private static void assumeNoUnsupportedType( + Type type, Type.TypeID unsupportedTypeId, String unsupportedTypeName) { + Assumptions.assumeThat(TypeUtil.find(type, t -> t.typeId() == unsupportedTypeId) == null) + .as(unsupportedTypeName + " is not yet implemented") + .isTrue(); + } + @Test public void testSimpleStruct() throws IOException { writeAndValidate(new Schema(SUPPORTED_PRIMITIVES.fields())); From 8f677a81eba4271c8e1cdb188a3eeb29a5c68747 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Fri, 14 Mar 2025 22:35:48 +0800 Subject: [PATCH 09/16] Fix geometry/geography type support for Hive catalog --- .../src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java index 20f9eb7f616e..e4ed1f086d8b 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java @@ -166,6 +166,8 @@ private static String convertToTypeString(Type type) { return "timestamp"; case FIXED: case BINARY: + case GEOMETRY: + case GEOGRAPHY: return "binary"; case DECIMAL: final Types.DecimalType decimalType = (Types.DecimalType) type; From 14191747f6ae65169546d9782b72cbdb38656ba9 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Sat, 15 Mar 2025 14:58:41 +0800 Subject: [PATCH 10/16] Fix review comments --- .../apache/iceberg/types/EdgeAlgorithm.java | 4 ++ .../java/org/apache/iceberg/types/Types.java | 30 ++++++++------- .../iceberg/types/TestSerializableTypes.java | 2 - .../org/apache/iceberg/types/TestTypes.java | 20 +++++++++- .../java/org/apache/iceberg/SchemaParser.java | 7 ++-- .../org/apache/iceberg/TestTableMetadata.java | 37 ------------------- .../org/apache/iceberg/data/DataTest.java | 28 ++++++++------ 7 files changed, 57 insertions(+), 71 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java b/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java index 08cae564724d..c3634a9d652c 100644 --- a/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java +++ b/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java @@ -53,4 +53,8 @@ public static EdgeAlgorithm fromName(String algorithmName) { String.format("Invalid edge interpolation algorithm: %s", algorithmName), e); } } + + public String algorithmName() { + return name().toLowerCase(Locale.ENGLISH); + } } diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index e16f777caca8..fbd6bcc21e9a 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -31,7 +31,6 @@ import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.base.Joiner; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; -import org.apache.iceberg.relocated.com.google.common.base.Strings; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.types.Type.NestedType; @@ -64,9 +63,9 @@ private Types() {} .buildOrThrow(); private static final Pattern FIXED = Pattern.compile("fixed\\[\\s*(\\d+)\\s*\\]"); - private static final Pattern GEOMETRY_PARAMETERS = Pattern.compile("(?:\\(\\s*([^,]*?)\\s*\\))?"); + private static final Pattern GEOMETRY_PARAMETERS = Pattern.compile("(?:\\(\\s*([^,]+?)\\s*\\))?"); private static final Pattern GEOGRAPHY_PARAMETERS = - Pattern.compile("(?:\\(\\s*([^,]+?)?\\s*(?:,\\s*(\\w*)\\s*)?\\))?"); + Pattern.compile("(?:\\(\\s*([^,]+)\\s*(?:,\\s*(\\w*)\\s*)?\\))?"); private static final Pattern DECIMAL = Pattern.compile("decimal\\(\\s*(\\d+)\\s*,\\s*(\\d+)\\s*\\)"); @@ -79,19 +78,19 @@ public static Type fromTypeName(String typeString) { if (lowerTypeString.startsWith("geometry")) { Matcher geometry = GEOMETRY_PARAMETERS.matcher(typeString.substring(8)); if (geometry.matches()) { - return GeometryType.of(geometry.group(1)); + String crs = geometry.group(1); + return GeometryType.of(crs != null ? crs.trim() : null); } } if (lowerTypeString.startsWith("geography")) { Matcher geography = GEOGRAPHY_PARAMETERS.matcher(typeString.substring(9)); if (geography.matches()) { + String crs = geography.group(1); String algorithmName = geography.group(2); EdgeAlgorithm algorithm = - (algorithmName == null || algorithmName.isEmpty()) - ? null - : EdgeAlgorithm.fromName(algorithmName); - return GeographyType.of(geography.group(1), algorithm); + algorithmName == null ? null : EdgeAlgorithm.fromName(algorithmName.trim()); + return GeographyType.of(crs != null ? crs.trim() : null, algorithm); } } @@ -570,10 +569,12 @@ public int hashCode() { public static class GeometryType extends PrimitiveType { + public static final String DEFAULT_CRS = "OGC:CRS84"; + private final String crs; private GeometryType(String crs) { - this.crs = (crs == null || crs.isEmpty()) ? null : crs; + this.crs = (crs == null || DEFAULT_CRS.equals(crs)) ? null : crs; } public static GeometryType crs84() { @@ -612,7 +613,7 @@ public int hashCode() { @Override public String toString() { - if (Strings.isNullOrEmpty(crs)) { + if (crs == null) { return "geometry"; } @@ -622,11 +623,13 @@ public String toString() { 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) { - this.crs = (crs == null || crs.isEmpty()) ? null : crs; + this.crs = (crs == null || DEFAULT_CRS.equals(crs)) ? null : crs; this.algorithm = algorithm; } @@ -676,9 +679,8 @@ public int hashCode() { public String toString() { if (algorithm != null) { return String.format( - "geography(%s, %s)", - crs != null ? crs : "", algorithm.name().toLowerCase(Locale.ENGLISH)); - } else if (!Strings.isNullOrEmpty(crs)) { + "geography(%s, %s)", crs != null ? crs : DEFAULT_CRS, algorithm.algorithmName()); + } else if (crs != null) { return String.format("geography(%s)", crs); } else { return "geography"; diff --git a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java index 5cf58c8e33f6..0f719597ac50 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java @@ -66,10 +66,8 @@ public void testEqualTypes() throws Exception { Types.FixedType.ofLength(34), Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), - Types.GeometryType.of("projjson:Test_Identifier"), Types.GeographyType.crs84(), Types.GeographyType.of("srid:4269"), - Types.GeographyType.of("projjson:Test_Identifier"), Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), }; diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypes.java b/api/src/test/java/org/apache/iceberg/types/TestTypes.java index 3ceabe0da1d4..7aedd5f145dc 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypes.java @@ -84,7 +84,6 @@ public void fromPrimitiveString() { @Test public void geospatialTypeFromTypeName() { assertThat(Types.fromPrimitiveString("geometry")).isEqualTo(Types.GeometryType.crs84()); - assertThat(Types.fromPrimitiveString("geometry()")).isEqualTo(Types.GeometryType.crs84()); assertThat(Types.fromPrimitiveString("geometry(srid:3857)")) .isEqualTo(Types.GeometryType.of("srid:3857")); assertThat(Types.fromPrimitiveString("geometry( srid:3857 )")) @@ -94,8 +93,11 @@ public void geospatialTypeFromTypeName() { assertThat(Types.fromPrimitiveString("geometry( projjson:TestIdentifier )")) .isEqualTo(Types.GeometryType.of("projjson:TestIdentifier")); + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> Types.fromPrimitiveString("geometry()")) + .withMessageContaining("Cannot parse type string to primitive"); + assertThat(Types.fromPrimitiveString("geography")).isEqualTo(Types.GeographyType.crs84()); - assertThat(Types.fromPrimitiveString("geography()")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromPrimitiveString("geography(srid:4269)")) .isEqualTo(Types.GeographyType.of("srid:4269")); assertThat(Types.fromPrimitiveString("geography(srid: 4269)")) @@ -115,6 +117,9 @@ public void geospatialTypeFromTypeName() { assertThat(Types.fromPrimitiveString("geography(projjson: TestIdentifier, karney)")) .isEqualTo(Types.GeographyType.of("projjson: TestIdentifier", EdgeAlgorithm.KARNEY)); + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> Types.fromPrimitiveString("geography()")) + .withMessageContaining("Cannot parse type string to primitive"); assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geography(srid:4269, BadAlgorithm)")) .withMessageContaining("Invalid edge interpolation algorithm") @@ -131,6 +136,17 @@ public void geospatialTypeFromTypeName() { .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); } + @Test + public void testGeospatialTypeToString() { + assertThat(Types.GeometryType.crs84().toString()).isEqualTo("geometry"); + assertThat(Types.GeometryType.of("srid:4326").toString()).isEqualTo("geometry(srid:4326)"); + assertThat(Types.GeographyType.crs84().toString()).isEqualTo("geography"); + assertThat(Types.GeographyType.of("srid:4326", EdgeAlgorithm.KARNEY).toString()) + .isEqualTo("geography(srid:4326, karney)"); + assertThat(Types.GeographyType.of(null, EdgeAlgorithm.KARNEY).toString()) + .isEqualTo("geography(OGC:CRS84, karney)"); + } + @Test public void testNestedFieldBuilderIdCheck() { assertThatExceptionOfType(NullPointerException.class) diff --git a/core/src/main/java/org/apache/iceberg/SchemaParser.java b/core/src/main/java/org/apache/iceberg/SchemaParser.java index 83e0c7235a15..e90924455998 100644 --- a/core/src/main/java/org/apache/iceberg/SchemaParser.java +++ b/core/src/main/java/org/apache/iceberg/SchemaParser.java @@ -29,7 +29,6 @@ import org.apache.iceberg.expressions.Expressions; import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; -import org.apache.iceberg.relocated.com.google.common.base.Strings; import org.apache.iceberg.relocated.com.google.common.collect.Lists; import org.apache.iceberg.types.EdgeAlgorithm; import org.apache.iceberg.types.Type; @@ -152,7 +151,7 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws Types.GeometryType geometryType = (Types.GeometryType) primitive; generator.writeStartObject(); generator.writeStringField(TYPE, GEOMETRY); - if (!Strings.isNullOrEmpty(geometryType.crs())) { + if (geometryType.crs() != null) { generator.writeStringField(CRS, geometryType.crs()); } @@ -163,11 +162,11 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws Types.GeographyType geographyType = (Types.GeographyType) primitive; generator.writeStartObject(); generator.writeStringField(TYPE, GEOGRAPHY); - if (!Strings.isNullOrEmpty(geographyType.crs())) { + if (geographyType.crs() != null) { generator.writeStringField(CRS, geographyType.crs()); } if (geographyType.algorithm() != null) { - generator.writeStringField(ALGORITHM, geographyType.algorithm().name()); + generator.writeStringField(ALGORITHM, geographyType.algorithm().algorithmName()); } generator.writeEndObject(); diff --git a/core/src/test/java/org/apache/iceberg/TestTableMetadata.java b/core/src/test/java/org/apache/iceberg/TestTableMetadata.java index 92c2796cbfa6..07b4b0591646 100644 --- a/core/src/test/java/org/apache/iceberg/TestTableMetadata.java +++ b/core/src/test/java/org/apache/iceberg/TestTableMetadata.java @@ -1825,43 +1825,6 @@ public void testConstructV3Metadata() { 3); } - @Test - public void testV3GeometryTypeSupport() { - Schema v3SchemaGeom = - new Schema( - Types.NestedField.required(3, "id", Types.LongType.get()), - Types.NestedField.required(4, "geom", Types.GeometryType.crs84())); - Schema v3SchemaGeog = - new Schema( - Types.NestedField.required(3, "id", Types.LongType.get()), - Types.NestedField.required(4, "geog", Types.GeographyType.crs84())); - - for (Schema schema : ImmutableList.of(v3SchemaGeom, v3SchemaGeog)) { - for (int unsupportedFormatVersion : ImmutableList.of(1, 2)) { - assertThatThrownBy( - () -> - TableMetadata.newTableMetadata( - schema, - PartitionSpec.unpartitioned(), - SortOrder.unsorted(), - TEST_LOCATION, - ImmutableMap.of(), - unsupportedFormatVersion)) - .isInstanceOf(IllegalStateException.class) - .hasMessageContaining("not supported until v3"); - } - - // should be allowed in v3 - TableMetadata.newTableMetadata( - schema, - PartitionSpec.unpartitioned(), - SortOrder.unsorted(), - TEST_LOCATION, - ImmutableMap.of(), - 3); - } - } - @Test public void onlyMetadataLocationIsUpdatedWithoutTimestampAndMetadataLogEntry() { String uuid = "386b9f01-002b-4d8c-b77f-42c3fd3b7c9b"; diff --git a/core/src/test/java/org/apache/iceberg/data/DataTest.java b/core/src/test/java/org/apache/iceberg/data/DataTest.java index dc89f6e0996d..6f5e0d9de67e 100644 --- a/core/src/test/java/org/apache/iceberg/data/DataTest.java +++ b/core/src/test/java/org/apache/iceberg/data/DataTest.java @@ -143,29 +143,33 @@ protected boolean supportsGeospatial() { @FieldSource("SIMPLE_TYPES") public void testTypeSchema(Type type) throws IOException { if (!supportsUnknown()) { - assumeNoUnsupportedType(type, Type.TypeID.UNKNOWN, "unknown"); + Assumptions.assumeThat(TypeUtil.find(type, t -> t.typeId() == Type.TypeID.UNKNOWN) == null) + .as("unknown is not yet implemented") + .isTrue(); } if (!supportsTimestampNanos()) { - assumeNoUnsupportedType(type, Type.TypeID.TIMESTAMP_NANO, "timestamp_ns"); + Assumptions.assumeThat( + TypeUtil.find(type, t -> t.typeId() == Type.TypeID.TIMESTAMP_NANO) == null) + .as("timestamp_ns is not yet implemented") + .isTrue(); } if (!supportsVariant()) { - assumeNoUnsupportedType(type, Type.TypeID.VARIANT, "variant"); + Assumptions.assumeThat(TypeUtil.find(type, t -> t.typeId() == Type.TypeID.VARIANT) == null) + .as("variant is not yet implemented") + .isTrue(); } if (!supportsGeospatial()) { - assumeNoUnsupportedType(type, Type.TypeID.GEOMETRY, "geometry"); - assumeNoUnsupportedType(type, Type.TypeID.GEOGRAPHY, "geography"); + Assumptions.assumeThat(TypeUtil.find(type, t -> t.typeId() == Type.TypeID.GEOMETRY) == null) + .as("geometry is not yet implemented") + .isTrue(); + Assumptions.assumeThat(TypeUtil.find(type, t -> t.typeId() == Type.TypeID.GEOGRAPHY) == null) + .as("geography is not yet implemented") + .isTrue(); } writeAndValidate(new Schema(required(1, "id", LongType.get()), optional(2, "test_type", type))); } - private static void assumeNoUnsupportedType( - Type type, Type.TypeID unsupportedTypeId, String unsupportedTypeName) { - Assumptions.assumeThat(TypeUtil.find(type, t -> t.typeId() == unsupportedTypeId) == null) - .as(unsupportedTypeName + " is not yet implemented") - .isTrue(); - } - @Test public void testSimpleStruct() throws IOException { writeAndValidate(new Schema(SUPPORTED_PRIMITIVES.fields())); From 2bbd4532b7c4e687ee5fc649857af9fd946c8cf5 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Sat, 15 Mar 2025 15:01:02 +0800 Subject: [PATCH 11/16] Rename algorithmName to toString, similar to what NullOrder did --- api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java | 3 ++- api/src/main/java/org/apache/iceberg/types/Types.java | 3 +-- core/src/main/java/org/apache/iceberg/SchemaParser.java | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java b/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java index c3634a9d652c..781c9f636d86 100644 --- a/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java +++ b/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java @@ -54,7 +54,8 @@ public static EdgeAlgorithm fromName(String algorithmName) { } } - public String algorithmName() { + @Override + public String toString() { return name().toLowerCase(Locale.ENGLISH); } } diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index fbd6bcc21e9a..e6f1ed62578c 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -678,8 +678,7 @@ public int hashCode() { @Override public String toString() { if (algorithm != null) { - return String.format( - "geography(%s, %s)", crs != null ? crs : DEFAULT_CRS, algorithm.algorithmName()); + return String.format("geography(%s, %s)", crs != null ? crs : DEFAULT_CRS, algorithm); } else if (crs != null) { return String.format("geography(%s)", crs); } else { diff --git a/core/src/main/java/org/apache/iceberg/SchemaParser.java b/core/src/main/java/org/apache/iceberg/SchemaParser.java index e90924455998..b25369ef0d2a 100644 --- a/core/src/main/java/org/apache/iceberg/SchemaParser.java +++ b/core/src/main/java/org/apache/iceberg/SchemaParser.java @@ -166,7 +166,7 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws generator.writeStringField(CRS, geographyType.crs()); } if (geographyType.algorithm() != null) { - generator.writeStringField(ALGORITHM, geographyType.algorithm().algorithmName()); + generator.writeStringField(ALGORITHM, geographyType.algorithm().toString()); } generator.writeEndObject(); From e157ea9f5cfe7ad4bb9b6515533a0df79fe997fe Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Tue, 18 Mar 2025 16:21:43 +0800 Subject: [PATCH 12/16] Removed jts from iceberg-core, revert implementation of single-value representation for geospatial types --- .../java/org/apache/iceberg/types/Types.java | 48 +++--- .../org/apache/iceberg/types/TestTypes.java | 12 +- build.gradle | 1 - .../java/org/apache/iceberg/SchemaParser.java | 60 +------- .../org/apache/iceberg/SingleValueParser.java | 21 --- .../org/apache/iceberg/util/GeometryUtil.java | 78 ---------- .../org/apache/iceberg/TestSchemaParser.java | 10 -- .../apache/iceberg/TestSingleValueParser.java | 16 -- .../apache/iceberg/util/TestGeometryUtil.java | 145 ------------------ gradle/libs.versions.toml | 2 - 10 files changed, 38 insertions(+), 355 deletions(-) delete mode 100644 core/src/main/java/org/apache/iceberg/util/GeometryUtil.java delete mode 100644 core/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index e6f1ed62578c..a69c6ee71cf4 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -63,9 +63,11 @@ private Types() {} .buildOrThrow(); private static final Pattern FIXED = Pattern.compile("fixed\\[\\s*(\\d+)\\s*\\]"); - private static final Pattern GEOMETRY_PARAMETERS = Pattern.compile("(?:\\(\\s*([^,]+?)\\s*\\))?"); + private static final Pattern GEOMETRY_PARAMETERS = + Pattern.compile("geometry\\s*(?:\\(\\s*([^,]+?)\\s*\\))?", Pattern.CASE_INSENSITIVE); private static final Pattern GEOGRAPHY_PARAMETERS = - Pattern.compile("(?:\\(\\s*([^,]+)\\s*(?:,\\s*(\\w*)\\s*)?\\))?"); + Pattern.compile( + "geography\\s*(?:\\(\\s*([^,]+)\\s*(?:,\\s*(\\w*)\\s*)?\\))?", Pattern.CASE_INSENSITIVE); private static final Pattern DECIMAL = Pattern.compile("decimal\\(\\s*(\\d+)\\s*,\\s*(\\d+)\\s*\\)"); @@ -75,23 +77,19 @@ public static Type fromTypeName(String typeString) { return TYPES.get(lowerTypeString); } - if (lowerTypeString.startsWith("geometry")) { - Matcher geometry = GEOMETRY_PARAMETERS.matcher(typeString.substring(8)); - if (geometry.matches()) { - String crs = geometry.group(1); - return GeometryType.of(crs != null ? crs.trim() : null); - } + Matcher geometry = GEOMETRY_PARAMETERS.matcher(typeString); + if (geometry.matches()) { + String crs = geometry.group(1); + return GeometryType.of(crs != null ? crs.trim() : null); } - if (lowerTypeString.startsWith("geography")) { - Matcher geography = GEOGRAPHY_PARAMETERS.matcher(typeString.substring(9)); - if (geography.matches()) { - String crs = geography.group(1); - String algorithmName = geography.group(2); - EdgeAlgorithm algorithm = - algorithmName == null ? null : EdgeAlgorithm.fromName(algorithmName.trim()); - return GeographyType.of(crs != null ? crs.trim() : null, algorithm); - } + Matcher geography = GEOGRAPHY_PARAMETERS.matcher(typeString); + if (geography.matches()) { + String crs = geography.group(1); + String algorithmName = geography.group(2); + EdgeAlgorithm algorithm = + algorithmName == null ? null : EdgeAlgorithm.fromName(algorithmName.trim()); + return GeographyType.of(crs != null ? crs.trim() : null, algorithm); } Matcher fixed = FIXED.matcher(lowerTypeString); @@ -569,12 +567,15 @@ public int hashCode() { public static class GeometryType extends PrimitiveType { - public static final String DEFAULT_CRS = "OGC:CRS84"; - private final String crs; private GeometryType(String crs) { - this.crs = (crs == null || DEFAULT_CRS.equals(crs)) ? null : 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() { @@ -629,7 +630,12 @@ public static class GeographyType extends PrimitiveType { private final EdgeAlgorithm algorithm; private GeographyType(String crs, EdgeAlgorithm algorithm) { - this.crs = (crs == null || DEFAULT_CRS.equals(crs)) ? null : 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; this.algorithm = algorithm; } diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypes.java b/api/src/test/java/org/apache/iceberg/types/TestTypes.java index 7aedd5f145dc..bf89bd4984a2 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypes.java @@ -84,20 +84,25 @@ public void fromPrimitiveString() { @Test public void geospatialTypeFromTypeName() { assertThat(Types.fromPrimitiveString("geometry")).isEqualTo(Types.GeometryType.crs84()); + assertThat(Types.fromPrimitiveString("Geometry")).isEqualTo(Types.GeometryType.crs84()); assertThat(Types.fromPrimitiveString("geometry(srid:3857)")) .isEqualTo(Types.GeometryType.of("srid:3857")); assertThat(Types.fromPrimitiveString("geometry( srid:3857 )")) .isEqualTo(Types.GeometryType.of("srid:3857")); assertThat(Types.fromPrimitiveString("geometry( srid: 3857 )")) .isEqualTo(Types.GeometryType.of("srid: 3857")); - assertThat(Types.fromPrimitiveString("geometry( projjson:TestIdentifier )")) + assertThat(Types.fromPrimitiveString("Geometry( projjson:TestIdentifier )")) .isEqualTo(Types.GeometryType.of("projjson:TestIdentifier")); assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geometry()")) .withMessageContaining("Cannot parse type string to primitive"); + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> Types.fromPrimitiveString("geometry( )")) + .withMessageContaining("Invalid CRS: (empty string)"); assertThat(Types.fromPrimitiveString("geography")).isEqualTo(Types.GeographyType.crs84()); + assertThat(Types.fromPrimitiveString("Geography")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromPrimitiveString("geography(srid:4269)")) .isEqualTo(Types.GeographyType.of("srid:4269")); assertThat(Types.fromPrimitiveString("geography(srid: 4269)")) @@ -114,12 +119,15 @@ public void geospatialTypeFromTypeName() { .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); assertThat(Types.fromPrimitiveString("geography(srid: 4269, karney)")) .isEqualTo(Types.GeographyType.of("srid: 4269", EdgeAlgorithm.KARNEY)); - assertThat(Types.fromPrimitiveString("geography(projjson: TestIdentifier, karney)")) + assertThat(Types.fromPrimitiveString("Geography(projjson: TestIdentifier, karney)")) .isEqualTo(Types.GeographyType.of("projjson: TestIdentifier", EdgeAlgorithm.KARNEY)); assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geography()")) .withMessageContaining("Cannot parse type string to primitive"); + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> Types.fromPrimitiveString("geography( , spherical)")) + .withMessageContaining("Invalid CRS: (empty string)"); assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geography(srid:4269, BadAlgorithm)")) .withMessageContaining("Invalid edge interpolation algorithm") diff --git a/build.gradle b/build.gradle index 176c5479f207..35723b57df4a 100644 --- a/build.gradle +++ b/build.gradle @@ -348,7 +348,6 @@ project(':iceberg-core') { implementation libs.jackson.databind implementation libs.caffeine implementation libs.roaringbitmap - implementation libs.jts.core compileOnly(libs.hadoop3.client) { exclude group: 'org.apache.avro', module: 'avro' exclude group: 'org.slf4j', module: 'slf4j-log4j12' diff --git a/core/src/main/java/org/apache/iceberg/SchemaParser.java b/core/src/main/java/org/apache/iceberg/SchemaParser.java index b25369ef0d2a..492668ff01f3 100644 --- a/core/src/main/java/org/apache/iceberg/SchemaParser.java +++ b/core/src/main/java/org/apache/iceberg/SchemaParser.java @@ -30,7 +30,6 @@ import org.apache.iceberg.expressions.Literal; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; import org.apache.iceberg.relocated.com.google.common.collect.Lists; -import org.apache.iceberg.types.EdgeAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.JsonUtil; @@ -45,10 +44,6 @@ private SchemaParser() {} private static final String STRUCT = "struct"; private static final String LIST = "list"; private static final String MAP = "map"; - private static final String GEOMETRY = "geometry"; - private static final String GEOGRAPHY = "geography"; - private static final String CRS = "crs"; - private static final String ALGORITHM = "algorithm"; private static final String FIELDS = "fields"; private static final String ELEMENT = "element"; private static final String KEY = "key"; @@ -145,42 +140,8 @@ static void toJson(Types.MapType map, JsonGenerator generator) throws IOExceptio generator.writeEndObject(); } - static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws IOException { - switch (primitive.typeId()) { - case GEOMETRY: - Types.GeometryType geometryType = (Types.GeometryType) primitive; - generator.writeStartObject(); - generator.writeStringField(TYPE, GEOMETRY); - if (geometryType.crs() != null) { - generator.writeStringField(CRS, geometryType.crs()); - } - - generator.writeEndObject(); - break; - - case GEOGRAPHY: - Types.GeographyType geographyType = (Types.GeographyType) primitive; - generator.writeStartObject(); - generator.writeStringField(TYPE, GEOGRAPHY); - if (geographyType.crs() != null) { - generator.writeStringField(CRS, geographyType.crs()); - } - if (geographyType.algorithm() != null) { - generator.writeStringField(ALGORITHM, geographyType.algorithm().toString()); - } - - generator.writeEndObject(); - break; - - default: - generator.writeString(primitive.toString()); - } - } - static void toJson(Type type, JsonGenerator generator) throws IOException { - if (type.isPrimitiveType()) { - toJson(type.asPrimitiveType(), generator); - } else if (type.isVariantType()) { + if (type.isPrimitiveType() || type.isVariantType()) { generator.writeString(type.toString()); } else { Type.NestedType nested = type.asNestedType(); @@ -227,10 +188,6 @@ private static Type typeFromJson(JsonNode json) { return listFromJson(json); } else if (MAP.equals(type)) { return mapFromJson(json); - } else if (GEOMETRY.equals(type)) { - return geometryFromJson(json); - } else if (GEOGRAPHY.equals(type)) { - return geographyFromJson(json); } } } @@ -316,21 +273,6 @@ private static Types.MapType mapFromJson(JsonNode json) { } } - private static Types.GeometryType geometryFromJson(JsonNode json) { - String crs = JsonUtil.getStringOrNull(CRS, json); - return Types.GeometryType.of(crs); - } - - private static Types.GeographyType geographyFromJson(JsonNode json) { - String crs = JsonUtil.getStringOrNull(CRS, json); - String algorithmName = JsonUtil.getStringOrNull(ALGORITHM, json); - EdgeAlgorithm algorithm = - ((algorithmName == null || algorithmName.isEmpty()) - ? null - : EdgeAlgorithm.fromName(algorithmName)); - return Types.GeographyType.of(crs, algorithm); - } - public static Schema fromJson(JsonNode json) { Type type = typeFromJson(json); Preconditions.checkArgument( diff --git a/core/src/main/java/org/apache/iceberg/SingleValueParser.java b/core/src/main/java/org/apache/iceberg/SingleValueParser.java index 21990c88938e..3de6a0bcc663 100644 --- a/core/src/main/java/org/apache/iceberg/SingleValueParser.java +++ b/core/src/main/java/org/apache/iceberg/SingleValueParser.java @@ -38,9 +38,7 @@ import org.apache.iceberg.types.Types; import org.apache.iceberg.util.ByteBuffers; import org.apache.iceberg.util.DateTimeUtil; -import org.apache.iceberg.util.GeometryUtil; import org.apache.iceberg.util.JsonUtil; -import org.locationtech.jts.geom.Geometry; public class SingleValueParser { private SingleValueParser() {} @@ -162,18 +160,6 @@ public static Object fromJson(Type type, JsonNode defaultValue) { byte[] binaryBytes = BaseEncoding.base16().decode(defaultValue.textValue().toUpperCase(Locale.ROOT)); return ByteBuffer.wrap(binaryBytes); - case GEOMETRY: - case GEOGRAPHY: - Preconditions.checkArgument( - defaultValue.isTextual(), "Cannot parse default as a %s value: %s", type, defaultValue); - try { - Geometry geom = GeometryUtil.fromWKT(defaultValue.textValue()); - byte[] wkb = GeometryUtil.toWKB(geom); - return ByteBuffer.wrap(wkb); - } catch (Exception e) { - throw new IllegalArgumentException( - String.format("Cannot parse default as a %s value: %s", type, defaultValue), e); - } case LIST: return listFromJson(type, defaultValue); case MAP: @@ -349,13 +335,6 @@ public static void toJson(Type type, Object defaultValue, JsonGenerator generato generator.writeString(decimalValue.toString()); } break; - case GEOMETRY: - case GEOGRAPHY: - Preconditions.checkArgument( - defaultValue instanceof ByteBuffer, "Invalid default %s value: %s", type, defaultValue); - byte[] wkb = ByteBuffers.toByteArray((ByteBuffer) defaultValue); - generator.writeString(GeometryUtil.toWKT(GeometryUtil.fromWKB(wkb))); - break; case LIST: Preconditions.checkArgument( defaultValue instanceof List, "Invalid default %s value: %s", type, defaultValue); diff --git a/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java b/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java deleted file mode 100644 index 40c691c76b3d..000000000000 --- a/core/src/main/java/org/apache/iceberg/util/GeometryUtil.java +++ /dev/null @@ -1,78 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.iceberg.util; - -import org.locationtech.jts.geom.Coordinate; -import org.locationtech.jts.geom.Geometry; -import org.locationtech.jts.io.WKBReader; -import org.locationtech.jts.io.WKBWriter; -import org.locationtech.jts.io.WKTReader; -import org.locationtech.jts.io.WKTWriter; - -public class GeometryUtil { - - private GeometryUtil() {} - - private static final int DEFAULT_DIMENSION = 2; - - public static byte[] toWKB(Geometry geom) { - WKBWriter wkbWriter = new WKBWriter(outputDimension(geom), false); - return wkbWriter.write(geom); - } - - public static String toWKT(Geometry geom) { - WKTWriter wktWriter = new WKTWriter(outputDimension(geom)); - return wktWriter.write(geom); - } - - public static Geometry fromWKB(byte[] wkb) { - WKBReader reader = new WKBReader(); - try { - return reader.read(wkb); - } catch (Exception e) { - throw new IllegalArgumentException("Failed to parse WKB", e); - } - } - - public static Geometry fromWKT(String wkt) { - WKTReader reader = new WKTReader(); - try { - return reader.read(wkt); - } catch (Exception e) { - throw new IllegalArgumentException("Failed to parse WKT", e); - } - } - - private static int outputDimension(Geometry geom) { - int dimension = DEFAULT_DIMENSION; - Coordinate coordinate = geom.getCoordinate(); - - // We need to set outputDimension = 4 for XYM geometries to make JTS WKTWriter or WKBWriter work - // correctly. - // The WKB/WKT writers will ignore Z ordinate for XYM geometries. - if (!Double.isNaN(coordinate.getZ())) { - dimension = 3; - } - if (!Double.isNaN(coordinate.getM())) { - dimension = 4; - } - - return dimension; - } -} diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaParser.java b/core/src/test/java/org/apache/iceberg/TestSchemaParser.java index 1c1f906ab012..a52e779d9a0f 100644 --- a/core/src/test/java/org/apache/iceberg/TestSchemaParser.java +++ b/core/src/test/java/org/apache/iceberg/TestSchemaParser.java @@ -147,14 +147,4 @@ public void testPrimitiveTypeDefaultValues(Type.PrimitiveType type, Literal d assertThat(serialized.findField("col_with_default").writeDefault()) .isEqualTo(defaultValue.value()); } - - @Test - public void testVariantType() throws IOException { - Schema schema = - new Schema( - Types.NestedField.required(1, "id", Types.IntegerType.get()), - Types.NestedField.optional(2, "data", Types.VariantType.get())); - - writeAndValidate(schema); - } } diff --git a/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java b/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java index b81badab7282..cc1578b0e081 100644 --- a/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java +++ b/core/src/test/java/org/apache/iceberg/TestSingleValueParser.java @@ -25,7 +25,6 @@ import java.io.IOException; import java.util.Locale; -import org.apache.iceberg.types.EdgeAlgorithm; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.JsonUtil; @@ -54,12 +53,6 @@ public void testValidDefaults() throws IOException { {Types.DecimalType.of(9, 4), "\"123.4500\""}, {Types.DecimalType.of(9, 0), "\"2\""}, {Types.DecimalType.of(9, -20), "\"2E+20\""}, - {Types.GeometryType.crs84(), "\"POINT (1 2)\""}, - {Types.GeometryType.crs84(), "\"POINT Z(1 2 3)\""}, - {Types.GeometryType.crs84(), "\"POINT ZM(1 2 3 4)\""}, - {Types.GeometryType.of("srid:3857"), "\"POINT (1 2)\""}, - {Types.GeographyType.crs84(), "\"POINT ZM(1 2 3 4)\""}, - {Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), "\"POINT ZM(1 2 3 4)\""}, {Types.ListType.ofOptional(1, Types.IntegerType.get()), "[1, 2, 3]"}, { Types.MapType.ofOptional(2, 3, Types.IntegerType.get(), Types.StringType.get()), @@ -164,15 +157,6 @@ public void testInvalidTimestamptz() { .hasMessageStartingWith("Cannot parse default as a timestamptz value"); } - @Test - public void testInvalidGeometry() { - Type expectedType = Types.GeometryType.crs84(); - String defaultJson = "\"POINT (1 2 3 4 5 6)\""; - assertThatThrownBy(() -> defaultValueParseAndUnParseRoundTrip(expectedType, defaultJson)) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageStartingWith("Cannot parse default as a geometry value"); - } - // serialize to json and deserialize back should return the same result private static String defaultValueParseAndUnParseRoundTrip(Type type, String defaultValue) { Object javaDefaultValue = SingleValueParser.fromJson(type, defaultValue); diff --git a/core/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java b/core/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java deleted file mode 100644 index 3ab4fcf2525c..000000000000 --- a/core/src/test/java/org/apache/iceberg/util/TestGeometryUtil.java +++ /dev/null @@ -1,145 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.apache.iceberg.util; - -import static org.assertj.core.api.Assertions.assertThat; - -import org.junit.jupiter.api.Disabled; -import org.junit.jupiter.api.Test; -import org.locationtech.jts.geom.Coordinate; -import org.locationtech.jts.geom.CoordinateXY; -import org.locationtech.jts.geom.CoordinateXYM; -import org.locationtech.jts.geom.CoordinateXYZM; -import org.locationtech.jts.geom.Geometry; -import org.locationtech.jts.geom.GeometryFactory; - -public class TestGeometryUtil { - private static final GeometryFactory FACTORY = new GeometryFactory(); - - @Test - public void testToWKB() { - Geometry geometry = FACTORY.createPoint(new Coordinate(1.0, 2.0)); - byte[] wkb = GeometryUtil.toWKB(geometry); - Geometry readGeometry = GeometryUtil.fromWKB(wkb); - assertThat(geometry).isEqualTo(readGeometry); - Coordinate coordinate = readGeometry.getCoordinate(); - assertThat(coordinate.getZ()).isNaN(); - assertThat(coordinate.getM()).isNaN(); - } - - @Test - public void testXYToWKB() { - Geometry geometry = FACTORY.createPoint(new CoordinateXY(1.0, 2.0)); - byte[] wkb = GeometryUtil.toWKB(geometry); - Geometry readGeometry = GeometryUtil.fromWKB(wkb); - assertThat(geometry).isEqualTo(readGeometry); - Coordinate coordinate = readGeometry.getCoordinate(); - assertThat(coordinate.getZ()).isNaN(); - assertThat(coordinate.getM()).isNaN(); - } - - @Test - public void testXYZToWKB() { - Geometry geometry = FACTORY.createPoint(new Coordinate(1.0, 2.0, 3.0)); - byte[] wkb = GeometryUtil.toWKB(geometry); - Geometry readGeometry = GeometryUtil.fromWKB(wkb); - assertThat(geometry).isEqualTo(readGeometry); - Coordinate coordinate = readGeometry.getCoordinate(); - assertThat(coordinate.getZ()).isEqualTo(3.0); - assertThat(coordinate.getM()).isNaN(); - } - - @Test - @Disabled("https://github.com/locationtech/jts/issues/733") - public void testXYMToWKB() { - Geometry geometry = FACTORY.createPoint(new CoordinateXYM(1.0, 2.0, 3.0)); - byte[] wkb = GeometryUtil.toWKB(geometry); - Geometry readGeometry = GeometryUtil.fromWKB(wkb); - assertThat(geometry).isEqualTo(readGeometry); - Coordinate coordinate = readGeometry.getCoordinate(); - assertThat(coordinate.getZ()).isNaN(); - assertThat(coordinate.getM()).isEqualTo(3.0); - } - - @Test - public void testXYZMToWKB() { - Geometry geometry = FACTORY.createPoint(new CoordinateXYZM(1.0, 2.0, 3.0, 4.0)); - byte[] wkb = GeometryUtil.toWKB(geometry); - Geometry readGeometry = GeometryUtil.fromWKB(wkb); - assertThat(geometry).isEqualTo(readGeometry); - Coordinate coordinate = readGeometry.getCoordinate(); - assertThat(coordinate.getZ()).isEqualTo(3.0); - assertThat(coordinate.getM()).isEqualTo(4.0); - } - - @Test - public void testToWKT() { - Geometry geometry = FACTORY.createPoint(new Coordinate(1.0, 2.0)); - String wkt = GeometryUtil.toWKT(geometry); - Geometry readGeometry = GeometryUtil.fromWKT(wkt); - assertThat(geometry).isEqualTo(readGeometry); - Coordinate coordinate = readGeometry.getCoordinate(); - assertThat(coordinate.getZ()).isNaN(); - assertThat(coordinate.getM()).isNaN(); - } - - @Test - public void testXYToWKT() { - Geometry geometry = FACTORY.createPoint(new CoordinateXY(1.0, 2.0)); - String wkt = GeometryUtil.toWKT(geometry); - Geometry readGeometry = GeometryUtil.fromWKT(wkt); - assertThat(geometry).isEqualTo(readGeometry); - Coordinate coordinate = readGeometry.getCoordinate(); - assertThat(coordinate.getZ()).isNaN(); - assertThat(coordinate.getM()).isNaN(); - } - - @Test - public void testXYZToWKT() { - Geometry geometry = FACTORY.createPoint(new Coordinate(1.0, 2.0, 3.0)); - String wkt = GeometryUtil.toWKT(geometry); - Geometry readGeometry = GeometryUtil.fromWKT(wkt); - assertThat(geometry).isEqualTo(readGeometry); - Coordinate coordinate = readGeometry.getCoordinate(); - assertThat(coordinate.getZ()).isEqualTo(3.0); - assertThat(coordinate.getM()).isNaN(); - } - - @Test - public void testXYMToWKT() { - Geometry geometry = FACTORY.createPoint(new CoordinateXYM(1.0, 2.0, 3.0)); - String wkt = GeometryUtil.toWKT(geometry); - Geometry readGeometry = GeometryUtil.fromWKT(wkt); - assertThat(geometry).isEqualTo(readGeometry); - Coordinate coordinate = readGeometry.getCoordinate(); - assertThat(coordinate.getZ()).isNaN(); - assertThat(coordinate.getM()).isEqualTo(3.0); - } - - @Test - public void testXYZMToWKT() { - Geometry geometry = FACTORY.createPoint(new CoordinateXYZM(1.0, 2.0, 3.0, 4.0)); - String wkt = GeometryUtil.toWKT(geometry); - Geometry readGeometry = GeometryUtil.fromWKT(wkt); - assertThat(geometry).isEqualTo(readGeometry); - Coordinate coordinate = readGeometry.getCoordinate(); - assertThat(coordinate.getZ()).isEqualTo(3.0); - assertThat(coordinate.getM()).isEqualTo(4.0); - } -} diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 0d788c830131..4fffa1e14d26 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -62,7 +62,6 @@ jakarta-servlet-api = "6.1.0" jaxb-api = "2.3.1" jaxb-runtime = "2.3.9" jetty = "11.0.24" -jts-core = "1.20.0" junit = "5.11.4" junit-platform = "1.11.4" kafka = "3.9.0" @@ -147,7 +146,6 @@ jackson214-bom = { module = "com.fasterxml.jackson:jackson-bom", version.ref = jackson215-bom = { module = "com.fasterxml.jackson:jackson-bom", version.ref = "jackson215" } jaxb-api = { module = "javax.xml.bind:jaxb-api", version.ref = "jaxb-api" } jaxb-runtime = { module = "org.glassfish.jaxb:jaxb-runtime", version.ref = "jaxb-runtime" } -jts-core = { module = "org.locationtech.jts:jts-core", version.ref = "jts-core" } kafka-clients = { module = "org.apache.kafka:kafka-clients", version.ref = "kafka" } kafka-connect-api = { module = "org.apache.kafka:connect-api", version.ref = "kafka" } kafka-connect-json = { module = "org.apache.kafka:connect-json", version.ref = "kafka" } From 384ef1c4997b376403b5e81fd6e6f24950388f00 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Wed, 19 Mar 2025 09:51:38 +0800 Subject: [PATCH 13/16] Fix error message for null edge interpolation algorithms --- api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java b/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java index 781c9f636d86..5ddc55c64adc 100644 --- a/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java +++ b/api/src/main/java/org/apache/iceberg/types/EdgeAlgorithm.java @@ -45,7 +45,7 @@ public enum EdgeAlgorithm { KARNEY; public static EdgeAlgorithm fromName(String algorithmName) { - Preconditions.checkNotNull(algorithmName, "Edge interpolation algorithm cannot be null"); + Preconditions.checkNotNull(algorithmName, "Invalid edge interpolation algorithm: null"); try { return EdgeAlgorithm.valueOf(algorithmName.toUpperCase(Locale.ENGLISH)); } catch (IllegalArgumentException e) { From 0c451cac53f4098680aa0ae518af70dc45efaeb1 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Wed, 19 Mar 2025 10:00:57 +0800 Subject: [PATCH 14/16] Move geospatial type parsing tests to fromTypeName and fromPrimitiveString --- .../org/apache/iceberg/types/TestTypes.java | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypes.java b/api/src/test/java/org/apache/iceberg/types/TestTypes.java index bf89bd4984a2..b1f588450974 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypes.java @@ -46,6 +46,22 @@ public void fromTypeName() { assertThat(Types.fromTypeName("variant")).isSameAs(Types.VariantType.get()); assertThat(Types.fromTypeName("Variant")).isSameAs(Types.VariantType.get()); + assertThat(Types.fromTypeName("geometry")).isEqualTo(Types.GeometryType.crs84()); + assertThat(Types.fromTypeName("Geometry")).isEqualTo(Types.GeometryType.crs84()); + assertThat(Types.fromTypeName("geometry(srid:3857)")) + .isEqualTo(Types.GeometryType.of("srid:3857")); + assertThat(Types.fromTypeName("geometry ( srid:3857 )")) + .isEqualTo(Types.GeometryType.of("srid:3857")); + + assertThat(Types.fromTypeName("geography")).isEqualTo(Types.GeographyType.crs84()); + assertThat(Types.fromTypeName("Geography")).isEqualTo(Types.GeographyType.crs84()); + assertThat(Types.fromTypeName("geography(srid:4269)")) + .isEqualTo(Types.GeographyType.of("srid:4269")); + assertThat(Types.fromTypeName("geography(srid:4269, karney)")) + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); + assertThat(Types.fromTypeName("geography ( srid:4269 , karney )")) + .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); + assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromTypeName("abcdefghij")) .withMessage("Cannot parse type string to primitive: abcdefghij"); @@ -79,10 +95,7 @@ public void fromPrimitiveString() { assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("abcdefghij")) .withMessage("Cannot parse type string to primitive: abcdefghij"); - } - @Test - public void geospatialTypeFromTypeName() { assertThat(Types.fromPrimitiveString("geometry")).isEqualTo(Types.GeometryType.crs84()); assertThat(Types.fromPrimitiveString("Geometry")).isEqualTo(Types.GeometryType.crs84()); assertThat(Types.fromPrimitiveString("geometry(srid:3857)")) From 7c8fa2b72c80b04e2c97f6661e75184e354d0cd1 Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Thu, 20 Mar 2025 08:44:50 +0800 Subject: [PATCH 15/16] Renamed GeographyType.of(crs) to GeographyType.forCRS, addressed more review comments --- .../java/org/apache/iceberg/types/Types.java | 24 +++++++++++++++---- .../iceberg/types/TestReadabilityChecks.java | 2 +- .../iceberg/types/TestSerializableTypes.java | 2 +- .../org/apache/iceberg/types/TestTypes.java | 8 +++---- .../org/apache/iceberg/data/DataTest.java | 2 +- 5 files changed, 26 insertions(+), 12 deletions(-) diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index a69c6ee71cf4..0d0269496b44 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -574,12 +574,18 @@ private GeometryType(String crs) { 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; + } else { + this.crs = null; } - this.crs = crs; + } + + private GeometryType() { + crs = null; } public static GeometryType crs84() { - return new GeometryType(null); + return new GeometryType(); } public static GeometryType of(String crs) { @@ -634,16 +640,24 @@ private GeographyType(String crs, EdgeAlgorithm algorithm) { 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; + } else { + this.crs = null; } - this.crs = crs; + this.algorithm = algorithm; } + private GeographyType() { + this.crs = null; + this.algorithm = null; + } + public static GeographyType crs84() { - return new GeographyType(null, null); + return new GeographyType(); } - public static GeographyType of(String crs) { + public static GeographyType forCRS(String crs) { return new GeographyType(crs, null); } diff --git a/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java b/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java index 20299cdafce2..3f4b364cef23 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java +++ b/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java @@ -57,7 +57,7 @@ public class TestReadabilityChecks { Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), Types.GeographyType.crs84(), - Types.GeographyType.of("srid:4269"), + Types.GeographyType.forCRS("srid:4269"), Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), }; diff --git a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java index 0f719597ac50..320a6a235d73 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java @@ -67,7 +67,7 @@ public void testEqualTypes() throws Exception { Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), Types.GeographyType.crs84(), - Types.GeographyType.of("srid:4269"), + Types.GeographyType.forCRS("srid:4269"), Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), }; diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypes.java b/api/src/test/java/org/apache/iceberg/types/TestTypes.java index b1f588450974..047fce5bde6b 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypes.java @@ -56,7 +56,7 @@ public void fromTypeName() { assertThat(Types.fromTypeName("geography")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromTypeName("Geography")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromTypeName("geography(srid:4269)")) - .isEqualTo(Types.GeographyType.of("srid:4269")); + .isEqualTo(Types.GeographyType.forCRS("srid:4269")); assertThat(Types.fromTypeName("geography(srid:4269, karney)")) .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); assertThat(Types.fromTypeName("geography ( srid:4269 , karney )")) @@ -117,9 +117,9 @@ public void fromPrimitiveString() { assertThat(Types.fromPrimitiveString("geography")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromPrimitiveString("Geography")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromPrimitiveString("geography(srid:4269)")) - .isEqualTo(Types.GeographyType.of("srid:4269")); + .isEqualTo(Types.GeographyType.forCRS("srid:4269")); assertThat(Types.fromPrimitiveString("geography(srid: 4269)")) - .isEqualTo(Types.GeographyType.of("srid: 4269")); + .isEqualTo(Types.GeographyType.forCRS("srid: 4269")); assertThat(Types.fromPrimitiveString("geography(srid:4269, spherical)")) .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.SPHERICAL)); assertThat(Types.fromPrimitiveString("geography(srid:4269, vincenty)")) @@ -148,7 +148,7 @@ public void fromPrimitiveString() { // Test geography type with various spacing assertThat(Types.fromPrimitiveString("geography( srid:4269 )")) - .isEqualTo(Types.GeographyType.of("srid:4269")); + .isEqualTo(Types.GeographyType.forCRS("srid:4269")); assertThat(Types.fromPrimitiveString("geography( srid:4269 , spherical )")) .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.SPHERICAL)); assertThat(Types.fromPrimitiveString("geography(srid:4269,vincenty)")) diff --git a/core/src/test/java/org/apache/iceberg/data/DataTest.java b/core/src/test/java/org/apache/iceberg/data/DataTest.java index 6f5e0d9de67e..d84d67794e70 100644 --- a/core/src/test/java/org/apache/iceberg/data/DataTest.java +++ b/core/src/test/java/org/apache/iceberg/data/DataTest.java @@ -119,7 +119,7 @@ protected boolean allowsWritingNullValuesForRequiredFields() { Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), Types.GeographyType.crs84(), - Types.GeographyType.of("srid:4269"), + Types.GeographyType.forCRS("srid:4269"), Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), }; From cc773215e141258304e8ca8299926fcb420aceef Mon Sep 17 00:00:00 2001 From: Kristin Cowalcijk Date: Sat, 22 Mar 2025 09:56:41 +0800 Subject: [PATCH 16/16] Fix review comments --- .../java/org/apache/iceberg/types/Types.java | 76 ++++++++---------- .../iceberg/types/TestReadabilityChecks.java | 2 +- .../iceberg/types/TestSerializableTypes.java | 2 +- .../org/apache/iceberg/types/TestTypes.java | 15 ++-- .../apache/iceberg/TestGeospatialTable.java | 77 +++++++++++++++++++ .../apache/iceberg/catalog/CatalogTests.java | 41 ---------- .../org/apache/iceberg/data/DataTest.java | 33 ++++---- .../apache/iceberg/hive/HiveSchemaUtil.java | 2 - 8 files changed, 136 insertions(+), 112 deletions(-) create mode 100644 core/src/test/java/org/apache/iceberg/TestGeospatialTable.java diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java index 0d0269496b44..e091fef10199 100644 --- a/api/src/main/java/org/apache/iceberg/types/Types.java +++ b/api/src/main/java/org/apache/iceberg/types/Types.java @@ -64,10 +64,10 @@ private Types() {} private static final Pattern FIXED = Pattern.compile("fixed\\[\\s*(\\d+)\\s*\\]"); private static final Pattern GEOMETRY_PARAMETERS = - Pattern.compile("geometry\\s*(?:\\(\\s*([^,]+?)\\s*\\))?", Pattern.CASE_INSENSITIVE); + Pattern.compile("geometry\\s*(?:\\(\\s*([^)]*?)\\s*\\))?", Pattern.CASE_INSENSITIVE); private static final Pattern GEOGRAPHY_PARAMETERS = Pattern.compile( - "geography\\s*(?:\\(\\s*([^,]+)\\s*(?:,\\s*(\\w*)\\s*)?\\))?", Pattern.CASE_INSENSITIVE); + "geography\\s*(?:\\(\\s*([^,]*?)\\s*(?:,\\s*(\\w*)\\s*)?\\))?", Pattern.CASE_INSENSITIVE); private static final Pattern DECIMAL = Pattern.compile("decimal\\(\\s*(\\d+)\\s*,\\s*(\\d+)\\s*\\)"); @@ -80,7 +80,8 @@ public static Type fromTypeName(String typeString) { Matcher geometry = GEOMETRY_PARAMETERS.matcher(typeString); if (geometry.matches()) { String crs = geometry.group(1); - return GeometryType.of(crs != null ? crs.trim() : null); + Preconditions.checkArgument(!crs.contains(","), "Invalid CRS: %s", crs); + return GeometryType.of(crs); } Matcher geography = GEOGRAPHY_PARAMETERS.matcher(typeString); @@ -88,8 +89,8 @@ public static Type fromTypeName(String typeString) { String crs = geography.group(1); String algorithmName = geography.group(2); EdgeAlgorithm algorithm = - algorithmName == null ? null : EdgeAlgorithm.fromName(algorithmName.trim()); - return GeographyType.of(crs != null ? crs.trim() : null, algorithm); + algorithmName == null ? null : EdgeAlgorithm.fromName(algorithmName); + return GeographyType.of(crs, algorithm); } Matcher fixed = FIXED.matcher(lowerTypeString); @@ -566,30 +567,25 @@ public int hashCode() { } public static class GeometryType extends PrimitiveType { + public static final String DEFAULT_CRS = "OGC:CRS84"; - private final String crs; + public static GeometryType crs84() { + return new GeometryType(); + } - 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; - } else { - this.crs = null; - } + public static GeometryType of(String crs) { + return new GeometryType(crs); } + private final String crs; + private GeometryType() { crs = null; } - public static GeometryType crs84() { - return new GeometryType(); - } - - public static GeometryType of(String crs) { - return new GeometryType(crs); + private GeometryType(String crs) { + Preconditions.checkArgument(crs == null || !crs.isEmpty(), "Invalid CRS: (empty string)"); + this.crs = DEFAULT_CRS.equalsIgnoreCase(crs) ? null : crs; } @Override @@ -629,35 +625,13 @@ public String toString() { } 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; - } else { - this.crs = null; - } - - this.algorithm = algorithm; - } - - private GeographyType() { - this.crs = null; - this.algorithm = null; - } - public static GeographyType crs84() { return new GeographyType(); } - public static GeographyType forCRS(String crs) { + public static GeographyType of(String crs) { return new GeographyType(crs, null); } @@ -665,6 +639,20 @@ public static GeographyType of(String crs, EdgeAlgorithm algorithm) { return new GeographyType(crs, algorithm); } + private final String crs; + private final EdgeAlgorithm algorithm; + + private GeographyType() { + this.crs = null; + this.algorithm = null; + } + + private GeographyType(String crs, EdgeAlgorithm algorithm) { + Preconditions.checkArgument(crs == null || !crs.isEmpty(), "Invalid CRS: (empty string)"); + this.crs = DEFAULT_CRS.equalsIgnoreCase(crs) ? null : crs; + this.algorithm = algorithm; + } + @Override public TypeID typeId() { return TypeID.GEOGRAPHY; diff --git a/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java b/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java index 3f4b364cef23..20299cdafce2 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java +++ b/api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java @@ -57,7 +57,7 @@ public class TestReadabilityChecks { Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), Types.GeographyType.crs84(), - Types.GeographyType.forCRS("srid:4269"), + Types.GeographyType.of("srid:4269"), Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), }; diff --git a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java index 320a6a235d73..0f719597ac50 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java @@ -67,7 +67,7 @@ public void testEqualTypes() throws Exception { Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), Types.GeographyType.crs84(), - Types.GeographyType.forCRS("srid:4269"), + Types.GeographyType.of("srid:4269"), Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), }; diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypes.java b/api/src/test/java/org/apache/iceberg/types/TestTypes.java index 047fce5bde6b..cc8d3586b862 100644 --- a/api/src/test/java/org/apache/iceberg/types/TestTypes.java +++ b/api/src/test/java/org/apache/iceberg/types/TestTypes.java @@ -56,7 +56,7 @@ public void fromTypeName() { assertThat(Types.fromTypeName("geography")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromTypeName("Geography")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromTypeName("geography(srid:4269)")) - .isEqualTo(Types.GeographyType.forCRS("srid:4269")); + .isEqualTo(Types.GeographyType.of("srid:4269")); assertThat(Types.fromTypeName("geography(srid:4269, karney)")) .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)); assertThat(Types.fromTypeName("geography ( srid:4269 , karney )")) @@ -109,17 +109,20 @@ public void fromPrimitiveString() { assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geometry()")) - .withMessageContaining("Cannot parse type string to primitive"); + .withMessageContaining("Invalid CRS: (empty string)"); assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geometry( )")) .withMessageContaining("Invalid CRS: (empty string)"); + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> Types.fromPrimitiveString("geometry(srid:123,456)")) + .withMessageContaining("Invalid CRS: srid:123,456"); assertThat(Types.fromPrimitiveString("geography")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromPrimitiveString("Geography")).isEqualTo(Types.GeographyType.crs84()); assertThat(Types.fromPrimitiveString("geography(srid:4269)")) - .isEqualTo(Types.GeographyType.forCRS("srid:4269")); + .isEqualTo(Types.GeographyType.of("srid:4269")); assertThat(Types.fromPrimitiveString("geography(srid: 4269)")) - .isEqualTo(Types.GeographyType.forCRS("srid: 4269")); + .isEqualTo(Types.GeographyType.of("srid: 4269")); assertThat(Types.fromPrimitiveString("geography(srid:4269, spherical)")) .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.SPHERICAL)); assertThat(Types.fromPrimitiveString("geography(srid:4269, vincenty)")) @@ -137,7 +140,7 @@ public void fromPrimitiveString() { assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geography()")) - .withMessageContaining("Cannot parse type string to primitive"); + .withMessageContaining("Invalid CRS: (empty string)"); assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> Types.fromPrimitiveString("geography( , spherical)")) .withMessageContaining("Invalid CRS: (empty string)"); @@ -148,7 +151,7 @@ public void fromPrimitiveString() { // Test geography type with various spacing assertThat(Types.fromPrimitiveString("geography( srid:4269 )")) - .isEqualTo(Types.GeographyType.forCRS("srid:4269")); + .isEqualTo(Types.GeographyType.of("srid:4269")); assertThat(Types.fromPrimitiveString("geography( srid:4269 , spherical )")) .isEqualTo(Types.GeographyType.of("srid:4269", EdgeAlgorithm.SPHERICAL)); assertThat(Types.fromPrimitiveString("geography(srid:4269,vincenty)")) diff --git a/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java b/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java new file mode 100644 index 000000000000..cb60e83a0940 --- /dev/null +++ b/core/src/test/java/org/apache/iceberg/TestGeospatialTable.java @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg; + +import static org.apache.iceberg.types.Types.NestedField.required; +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.IOException; +import java.util.Map; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.inmemory.InMemoryCatalog; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.types.EdgeAlgorithm; +import org.apache.iceberg.types.Type; +import org.apache.iceberg.types.Types; +import org.junit.jupiter.api.Test; + +public class TestGeospatialTable { + + @Test + public void testCreateGeospatialTable() throws IOException { + Schema schema = + new Schema( + required(3, "id", Types.IntegerType.get(), "unique ID"), + required(4, "data", Types.StringType.get()), + required(5, "geom", Types.GeometryType.of("srid:3857"), "geometry column"), + required( + 6, + "geog", + Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), + "geography column")); + + try (InMemoryCatalog catalog = initInMemoryCatalog()) { + catalog.createNamespace(Namespace.of("ns")); + + TableIdentifier identifier = TableIdentifier.of("ns", "geos_t1"); + Map properties = ImmutableMap.of(TableProperties.FORMAT_VERSION, "3"); + catalog.createTable(identifier, schema, PartitionSpec.unpartitioned(), properties); + Table table = catalog.loadTable(identifier); + + Types.NestedField geomField = table.schema().findField("geom"); + assertThat(geomField.type().typeId()).isEqualTo(Type.TypeID.GEOMETRY); + Types.GeometryType geomType = (Types.GeometryType) geomField.type(); + assertThat(geomType.crs()).isEqualTo("srid:3857"); + + Types.NestedField geogField = table.schema().findField("geog"); + assertThat(geogField.type().typeId()).isEqualTo(Type.TypeID.GEOGRAPHY); + Types.GeographyType geogType = (Types.GeographyType) geogField.type(); + assertThat(geogType.crs()).isEqualTo("srid:4269"); + assertThat(geogType.algorithm()).isEqualTo(EdgeAlgorithm.KARNEY); + assertThat(catalog.dropTable(identifier)).isTrue(); + } + } + + private InMemoryCatalog initInMemoryCatalog() { + InMemoryCatalog catalog = new InMemoryCatalog(); + catalog.initialize("in-memory-catalog", ImmutableMap.of()); + return catalog; + } +} diff --git a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java index a0bf2e468b2b..b7cfd9b16be4 100644 --- a/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java +++ b/core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java @@ -75,8 +75,6 @@ import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.relocated.com.google.common.collect.Sets; import org.apache.iceberg.relocated.com.google.common.collect.Streams; -import org.apache.iceberg.types.EdgeAlgorithm; -import org.apache.iceberg.types.Type; import org.apache.iceberg.types.Types; import org.apache.iceberg.util.CharSequenceSet; import org.junit.jupiter.api.Assumptions; @@ -538,45 +536,6 @@ public void testBasicCreateTable() { assertThat(table.properties()).as("Should have table properties").isNotNull(); } - @Test - public void testCreateGeospatialTable() { - Schema schema = - new Schema( - required(3, "id", Types.IntegerType.get(), "unique ID"), - required(4, "data", Types.StringType.get()), - required(5, "geom", Types.GeometryType.of("srid:3857"), "geometry column"), - required( - 6, - "geog", - Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), - "geography column")); - - TableIdentifier identifier = TableIdentifier.of("ns", "geos_table"); - - C catalog = catalog(); - assertThat(catalog.tableExists(identifier)).as("Table should not exist").isFalse(); - - if (requiresNamespaceCreate()) { - catalog.createNamespace(identifier.namespace()); - } - - Map properties = ImmutableMap.of(TableProperties.FORMAT_VERSION, "3"); - catalog.createTable(identifier, schema, PartitionSpec.unpartitioned(), properties); - Table table = catalog.loadTable(identifier); - - Types.NestedField geomField = table.schema().findField("geom"); - assertThat(geomField.type().typeId()).isEqualTo(Type.TypeID.GEOMETRY); - Types.GeometryType geomType = (Types.GeometryType) geomField.type(); - assertThat(geomType.crs()).isEqualTo("srid:3857"); - - Types.NestedField geogField = table.schema().findField("geog"); - assertThat(geogField.type().typeId()).isEqualTo(Type.TypeID.GEOGRAPHY); - Types.GeographyType geogType = (Types.GeographyType) geogField.type(); - assertThat(geogType.crs()).isEqualTo("srid:4269"); - assertThat(geogType.algorithm()).isEqualTo(EdgeAlgorithm.KARNEY); - assertThat(catalog.dropTable(identifier)).isTrue(); - } - @Test public void testTableNameWithSlash() { Assumptions.assumeTrue(supportsNamesWithSlashes()); diff --git a/core/src/test/java/org/apache/iceberg/data/DataTest.java b/core/src/test/java/org/apache/iceberg/data/DataTest.java index d84d67794e70..614c07329a1a 100644 --- a/core/src/test/java/org/apache/iceberg/data/DataTest.java +++ b/core/src/test/java/org/apache/iceberg/data/DataTest.java @@ -119,7 +119,7 @@ protected boolean allowsWritingNullValuesForRequiredFields() { Types.GeometryType.crs84(), Types.GeometryType.of("srid:3857"), Types.GeographyType.crs84(), - Types.GeographyType.forCRS("srid:4269"), + Types.GeographyType.of("srid:4269"), Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY), }; @@ -142,22 +142,21 @@ protected boolean supportsGeospatial() { @ParameterizedTest @FieldSource("SIMPLE_TYPES") public void testTypeSchema(Type type) throws IOException { - if (!supportsUnknown()) { - Assumptions.assumeThat(TypeUtil.find(type, t -> t.typeId() == Type.TypeID.UNKNOWN) == null) - .as("unknown is not yet implemented") - .isTrue(); - } - if (!supportsTimestampNanos()) { - Assumptions.assumeThat( - TypeUtil.find(type, t -> t.typeId() == Type.TypeID.TIMESTAMP_NANO) == null) - .as("timestamp_ns is not yet implemented") - .isTrue(); - } - if (!supportsVariant()) { - Assumptions.assumeThat(TypeUtil.find(type, t -> t.typeId() == Type.TypeID.VARIANT) == null) - .as("variant is not yet implemented") - .isTrue(); - } + Assumptions.assumeThat( + supportsUnknown() + || TypeUtil.find(type, t -> t.typeId() == Type.TypeID.UNKNOWN) == null) + .as("unknown is not yet implemented") + .isTrue(); + Assumptions.assumeThat( + supportsTimestampNanos() + || TypeUtil.find(type, t -> t.typeId() == Type.TypeID.TIMESTAMP_NANO) == null) + .as("timestamp_ns is not yet implemented") + .isTrue(); + Assumptions.assumeThat( + supportsVariant() + || TypeUtil.find(type, t -> t.typeId() == Type.TypeID.VARIANT) == null) + .as("variant is not yet implemented") + .isTrue(); if (!supportsGeospatial()) { Assumptions.assumeThat(TypeUtil.find(type, t -> t.typeId() == Type.TypeID.GEOMETRY) == null) .as("geometry is not yet implemented") diff --git a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java index e4ed1f086d8b..20f9eb7f616e 100644 --- a/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java +++ b/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveSchemaUtil.java @@ -166,8 +166,6 @@ private static String convertToTypeString(Type type) { return "timestamp"; case FIXED: case BINARY: - case GEOMETRY: - case GEOGRAPHY: return "binary"; case DECIMAL: final Types.DecimalType decimalType = (Types.DecimalType) type;