stevenzwu commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2629668432
##########
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:
oh. didn't realize it was for sanitized expression. I guess it would be
better to include the coordinate encoding, like `(boundingbox-x:y)` or
``(boundingbox-xy)`.
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java:
##########
@@ -284,6 +285,12 @@ public String toString() {
return term() + " startsWith \"" + literal() + "\"";
case NOT_STARTS_WITH:
return term() + " notStartsWith \"" + literal() + "\"";
+ case ST_INTERSECTS:
+ case ST_DISJOINT:
+ {
+ String functionName = op().name().toLowerCase(Locale.ROOT);
Review Comment:
Since both will be one line now, there is no benefit to combine the two
operations. we can hardcode the function name in the string construction, which
is also consistent with other cases.
##########
api/src/main/java/org/apache/iceberg/expressions/Literal.java:
##########
@@ -71,6 +72,10 @@ static Literal<BigDecimal> of(BigDecimal value) {
return new Literals.DecimalLiteral(value);
}
+ static Literal<ByteBuffer> of(BoundingBox value) {
Review Comment:
This is probably the first case in Iceberg where the literal value type
(`BoundingBox`) is different with the field value (`ByteBuffer`). I have been
wondering if the return type should be `Literal<BoundingBox>`.
##########
api/src/main/java/org/apache/iceberg/expressions/Expression.java:
##########
@@ -44,6 +44,8 @@ enum Operation {
OR,
STARTS_WITH,
NOT_STARTS_WITH,
+ ST_INTERSECTS,
Review Comment:
I see. thx for explaining
--
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]
