Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2629460771
##########
api/src/test/java/org/apache/iceberg/types/TestConversions.java:
##########
@@ -45,6 +47,7 @@
public class TestConversions {
@Test
+ @SuppressWarnings("MethodLength")
Review Comment:
Moved geo type buffer conversion to a new test method
`testGeospatialByteBufferConversions`.
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -809,4 +816,35 @@ public void testNotInExceptions() {
.isInstanceOf(ValidationException.class)
.hasMessageContaining("Invalid value for conversion to type int");
}
+
+ private static Stream<Arguments> geospatialPredicateParameters() {
+ return Stream.of(
+ Arguments.of(Expression.Operation.ST_INTERSECTS, "geom"),
Review Comment:
Renamed column names in tests for clarity.
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionUtil.java:
##########
@@ -1315,6 +1322,48 @@ private static VariantArray createArrayWithNestedTypes()
{
return (VariantArray) VariantValue.from(metadata, variantBB);
}
+ private static Stream<Arguments> geospatialPredicateParameters() {
+ return Stream.of(
+ Arguments.of(Expression.Operation.ST_INTERSECTS, "geom"),
+ Arguments.of(Expression.Operation.ST_INTERSECTS, "geog"),
+ Arguments.of(Expression.Operation.ST_DISJOINT, "geom"),
+ Arguments.of(Expression.Operation.ST_DISJOINT, "geog"));
+ }
+
+ @ParameterizedTest
+ @MethodSource("geospatialPredicateParameters")
+ public void testSanitizeGeospatialPredicates(Expression.Operation operation,
String columnName) {
+ // Create a schema with geometry and geography fields
+ Schema geoSchema =
Review Comment:
Moved fields into the static `SCHEMA` at the top of the file and removed
this locally constructed schema.
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionUtil.java:
##########
@@ -1315,6 +1322,48 @@ private static VariantArray createArrayWithNestedTypes()
{
return (VariantArray) VariantValue.from(metadata, variantBB);
}
+ private static Stream<Arguments> geospatialPredicateParameters() {
+ return Stream.of(
+ Arguments.of(Expression.Operation.ST_INTERSECTS, "geom"),
+ Arguments.of(Expression.Operation.ST_INTERSECTS, "geog"),
+ Arguments.of(Expression.Operation.ST_DISJOINT, "geom"),
+ Arguments.of(Expression.Operation.ST_DISJOINT, "geog"));
+ }
+
+ @ParameterizedTest
+ @MethodSource("geospatialPredicateParameters")
+ public void testSanitizeGeospatialPredicates(Expression.Operation operation,
String columnName) {
+ // Create a schema with geometry and geography fields
+ Schema geoSchema =
+ new Schema(
+ Types.NestedField.required(1, "geom", Types.GeometryType.crs84()),
+ Types.NestedField.required(2, "geog",
Types.GeographyType.crs84()));
+ Types.StructType geoStruct = geoSchema.asStruct();
+
+ // Create a bounding box for testing
+ GeospatialBound min = GeospatialBound.createXY(1.0, 2.0);
+ GeospatialBound max = GeospatialBound.createXY(3.0, 4.0);
+ BoundingBox bbox = new BoundingBox(min, max);
+
+ UnboundPredicate<ByteBuffer> geoPredicate =
+ Expressions.geospatialPredicate(operation, columnName, bbox);
+ Expression predicateSanitized = Expressions.predicate(operation,
columnName, "(boundingbox)");
Review Comment:
For ordinary expressions it should be a byte buffer. However, expression
sanitization makes the literals of such expressions as string literals
regardless of the original types of the literals. Is there a recommended way to
construct expected sanitization results?
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -648,4 +651,53 @@ public void
testNotInPredicateBindingConversionToExpression() {
.as("Should change NOT_IN to alwaysTrue expression")
.isEqualTo(Expressions.alwaysTrue());
}
+
+ @Test
+ public void testGeospatialPredicateBinding() {
+ StructType struct =
+ StructType.of(
+ required(20, "geom", Types.GeometryType.crs84()),
+ required(21, "geog", Types.GeographyType.crs84()));
+
+ // Create a bounding box for testing
+ GeospatialBound min = GeospatialBound.createXY(1.0, 2.0);
+ GeospatialBound max = GeospatialBound.createXY(3.0, 4.0);
+ BoundingBox bbox = new BoundingBox(min, max);
+
+ // Test ST_INTERSECTS with geometry
+ UnboundPredicate<ByteBuffer> stIntersectsGeom =
+ Expressions.geospatialPredicate(Expression.Operation.ST_INTERSECTS,
"geom", bbox);
Review Comment:
Made `Expressions.geospatialPredicate` package private and refactored the
tests.
##########
core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java:
##########
@@ -159,8 +162,13 @@ public <T> Void predicate(BoundPredicate<T> pred) {
if (pred.isLiteralPredicate()) {
gen.writeFieldName(VALUE);
- SingleValueParser.toJson(
- pred.term().type(),
pred.asLiteralPredicate().literal().value(), gen);
+ if (pred.op() == Operation.ST_INTERSECTS || pred.op() ==
Operation.ST_DISJOINT) {
Review Comment:
Yes. It is also related to the REST Catalog OpenAPI. I removed the changes
and will defer them to https://github.com/apache/iceberg/pull/14856.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
