You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Kontinuation (via GitHub)" <gi...@apache.org> on 2025/09/18 11:32:16 UTC
[PR] [WIP] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Kontinuation opened a new pull request, #14101:
URL: https://github.com/apache/iceberg/pull/14101
This PR is based on https://github.com/apache/iceberg/pull/12667. 2 spatial predicates were added to operate on geospatial bounds:
* `ST_INTERSECTS`: Test if 2 geospatial bounds intersects with each other
* `ST_DISJOINT`: The negation of `ST_INTERSECTS`.
A new literal type `BoundingBoxLiteral` is added for representing a geospatial query window or geospatial bounds in metrics or Parquet stats.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2757902703
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionUtil.java:
##########
@@ -1315,6 +1324,92 @@ private static VariantArray createArrayWithNestedTypes() {
return (VariantArray) VariantValue.from(metadata, variantBB);
}
+ private static Stream<Arguments> geospatialPredicateParameters() {
+ GeospatialBound min = GeospatialBound.createXY(1.0, 2.0);
+ GeospatialBound max = GeospatialBound.createXY(3.0, 4.0);
+ BoundingBox bbox = new BoundingBox(min, max);
+
+ GeospatialBound minXYZ = GeospatialBound.createXYZ(1.0, 2.0, 3.0);
+ GeospatialBound maxXYZ = GeospatialBound.createXYZ(3.0, 4.0, 5.0);
+ BoundingBox bboxXYZ = new BoundingBox(minXYZ, maxXYZ);
+
+ GeospatialBound minXYM = GeospatialBound.createXYM(1.0, 2.0, 3.0);
+ GeospatialBound maxXYM = GeospatialBound.createXYM(3.0, 4.0, 5.0);
+ BoundingBox bboxXYM = new BoundingBox(minXYM, maxXYM);
+
+ GeospatialBound minXYZM = GeospatialBound.createXYZM(1.0, 2.0, 3.0, 4.0);
+ GeospatialBound maxXYZM = GeospatialBound.createXYZM(3.0, 4.0, 5.0, 6.0);
+ BoundingBox bboxXYZM = new BoundingBox(minXYZM, maxXYZM);
+
+ return Stream.of(
+ Arguments.of(
+ Expressions.stIntersects("geometry", bbox),
+ "st_intersects(geometry, (boundingbox-xy))",
+ "(boundingbox-xy)"),
+ Arguments.of(
+ Expressions.stIntersects("geography", bbox),
+ "st_intersects(geography, (boundingbox-xy))",
+ "(boundingbox-xy)"),
+ Arguments.of(
+ Expressions.stDisjoint("geometry", bbox),
+ "st_disjoint(geometry, (boundingbox-xy))",
+ "(boundingbox-xy)"),
+ Arguments.of(
+ Expressions.stDisjoint("geography", bbox),
+ "st_disjoint(geography, (boundingbox-xy))",
+ "(boundingbox-xy)"),
+ Arguments.of(
+ Expressions.stIntersects("geometry", bboxXYZ),
+ "st_intersects(geometry, (boundingbox-xyz))",
+ "(boundingbox-xyz)"),
+ Arguments.of(
+ Expressions.stIntersects("geometry", bboxXYM),
+ "st_intersects(geometry, (boundingbox-xym))",
+ "(boundingbox-xym)"),
+ Arguments.of(
+ Expressions.stIntersects("geometry", bboxXYZM),
+ "st_intersects(geometry, (boundingbox-xyzm))",
+ "(boundingbox-xyzm)"));
+ }
+
+ @ParameterizedTest
+ @MethodSource("geospatialPredicateParameters")
+ public void testSanitizeGeospatialPredicates(
+ UnboundPredicate<ByteBuffer> geoPredicate,
+ String expectedSanitizedString,
+ String expectedLiteral) {
+ Expression.Operation operation = geoPredicate.op();
+ String columnName = geoPredicate.term().ref().name();
+
+ Expression predicateSanitized = Expressions.predicate(operation, columnName, expectedLiteral);
+ assertEquals(predicateSanitized, ExpressionUtil.sanitize(geoPredicate));
+ assertEquals(predicateSanitized, ExpressionUtil.sanitize(STRUCT, geoPredicate, true));
+
+ assertThat(ExpressionUtil.toSanitizedString(geoPredicate))
+ .as("Sanitized string should be identical for geospatial predicates")
+ .isEqualTo(expectedSanitizedString);
+
+ assertThat(ExpressionUtil.toSanitizedString(STRUCT, geoPredicate, true))
+ .as("Sanitized string should be identical for geospatial predicates")
+ .isEqualTo(expectedSanitizedString);
+ }
+
+ @Test
+ public void testBoundingBoxLiteralNormalizesBuffer() {
+ GeospatialBound min = GeospatialBound.createXY(1.0, 2.0);
+ GeospatialBound max = GeospatialBound.createXY(3.0, 4.0);
+ BoundingBox box = new BoundingBox(min, max);
+ ByteBuffer serialized = box.toByteBuffer();
+
+ ByteBuffer padded = ByteBuffer.allocate(serialized.remaining() + 1).order(ByteOrder.BIG_ENDIAN);
+ padded.put((byte) 0x0);
Review Comment:
This is for testing the buffer normalization. There's a comment suggesting normalizing the buffer to handle the non-little-endian or non-zero offset buffers properly: https://github.com/apache/iceberg/pull/14101#discussion_r2706366191
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2831376534
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -166,6 +178,12 @@ public <T> R predicate(BoundPredicate<T> pred) {
return startsWith((BoundReference<T>) pred.term(), literalPred.literal());
case NOT_STARTS_WITH:
return notStartsWith((BoundReference<T>) pred.term(), literalPred.literal());
+ case ST_INTERSECTS:
+ return stIntersects(
+ (BoundReference<T>) pred.term(), (Literal<ByteBuffer>) literalPred.literal());
Review Comment:
I think we still need to do an unchecked cast to cast `literalPred.literal` from `Literal<T>` to `Literal<BoundingBox>`, even if we define `BoundingBoxLiteral implements Literal<BoundingBox>`. Is this cast acceptable and look less problematic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830538431
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +723,71 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ private final ByteBuffer value;
+
+ BoundingBoxLiteral(BoundingBox value) {
+ this.value = value.toByteBuffer();
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
+ Preconditions.checkNotNull(value, "Bounding box buffer cannot be null");
+ this.value = value.slice().order(ByteOrder.LITTLE_ENDIAN);
+ }
+
+ @Override
+ public ByteBuffer value() {
+ return value;
+ }
+
+ @Override
+ public ByteBuffer toByteBuffer() {
+ return value;
+ }
+
+ @Override
+ public <T> Literal<T> to(Type type) {
+ if (type.typeId() != Type.TypeID.GEOMETRY && type.typeId() != Type.TypeID.GEOGRAPHY) {
+ return null;
+ }
+
+ return (Literal<T>) this;
+ }
+
+ @Override
+ public Comparator<ByteBuffer> comparator() {
Review Comment:
This doesn't quite fit since we don't expect the types to align. I think this should throw an exception because there isn't a comparator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2607268142
##########
core/src/test/java/org/apache/iceberg/expressions/TestExpressionParser.java:
##########
@@ -544,4 +563,122 @@ public void testNegativeScaleDecimalLiteral() {
assertThat(ExpressionParser.toJson(ExpressionParser.fromJson(expected), true))
.isEqualTo(expected);
}
+
+ @Test
+ public void testSpatialPredicate() {
+ String expected =
+ "{\n"
+ + " \"type\" : \"st-intersects\",\n"
Review Comment:
Should we add this to `rest-catalog-open-api.yaml`? I didn't see any geospatial expression there.
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -579,6 +593,8 @@ private static String sanitize(Literal<?> literal, long now, int today) {
return sanitizeNumber(((Literals.DoubleLiteral) literal).value(), "float");
} else if (literal instanceof Literals.VariantLiteral) {
return sanitizeVariant(((Literals.VariantLiteral) literal).value(), now, today);
+ } else if (literal instanceof BoundingBoxLiteral) {
+ return "(geospatial)";
Review Comment:
Do we want to use `(boundingbox)`?
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -318,7 +350,6 @@ public <T> R predicate(BoundPredicate<T> pred) {
"Invalid operation for BoundSetPredicate: " + pred.op());
}
}
-
Review Comment:
Revert unnecessary change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2757947778
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -518,44 +527,6 @@ private static List<String> abbreviateValues(List<String> sanitizedValues) {
return sanitizedValues;
}
- private static String sanitize(Type type, Literal<?> lit, long now, int today) {
Review Comment:
Yes. I have submitted https://github.com/apache/iceberg/pull/15224
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#issuecomment-3875570741
@Kontinuation please resolve the merge conflict
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830486715
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -126,6 +127,16 @@ public <T> R notStartsWith(BoundReference<T> ref, Literal<T> lit) {
"notStartsWith expression is not supported by the visitor");
}
+ public <T> R stIntersects(BoundReference<T> ref, Literal<ByteBuffer> lit) {
Review Comment:
I think that the right signature for this is `stIntersects(BoundReference<T>, BoundingBox)` or maybe with `BoundReference<ByteBuffer>`. The use of `T` in both `BoundReference` and `Literal` is because we want comparisons to be able to operate without requiring casting. That way, `ref.comparator()` is a `Comparator<T>` and both `ref.accessor().get(row)` and `lit.value()` are as well.
This is correct that we don't need `T` to match, but it's awkward to get a `BoundReference<T>` and a concrete `Literal`. Since we are going to need to trust binding to geo types, we can update the visitor to pass the `BoundingBox`. I'm not sure that we want to wrap `BoundingBox` in `Literal`, but I'm learning toward doing that so we don't need to add a new type of `BoundPredicate` that doesn't have a `literal`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2635232838
##########
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:
Switched to one liner for each case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#issuecomment-3449055875
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2604371067
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +722,45 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral extends BaseLiteral<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ BoundingBoxLiteral(BoundingBox value) {
+ super(value.toByteBuffer());
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
+ super(value);
+ }
+
+ @Override
+ protected Type.TypeID typeId() {
+ return Type.TypeID.GEOMETRY;
Review Comment:
this seems a bit weird to me. bounding box literal can only be `GEOMETRY` type?
Maybe we should revisit a previous discussion.
https://github.com/apache/iceberg/pull/12667#discussion_r2456577612
Maybe the `BoundingBox` needs to include the geo type?
##########
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:
curious if `BoundingBox` is the only geo literal we are going to have.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2843301905
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -202,6 +204,24 @@ public static UnboundPredicate<String> notStartsWith(UnboundTerm<String> expr, S
return new UnboundPredicate<>(Expression.Operation.NOT_STARTS_WITH, expr, value);
}
+ public static UnboundPredicate<ByteBuffer> stIntersects(String name, BoundingBox value) {
Review Comment:
I think I'm fine either way. We could include an override here that produces a bounding box. I agree with not having 6 and 8 argument versions of the method. And we will need to consider what to name the factory methods in `BoundingBox` because it can be ambiguous whether you're passing z or m through a 6-arg factory method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2843275043
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -126,6 +127,16 @@ public <T> R notStartsWith(BoundReference<T> ref, Literal<T> lit) {
"notStartsWith expression is not supported by the visitor");
}
+ public <T> R stIntersects(BoundReference<T> ref, Literal<ByteBuffer> lit) {
+ throw new UnsupportedOperationException(
+ "stIntersects expression is not supported by the visitor");
Review Comment:
Yes, and I don't think it is necessary to modify those. But we don't need this and try to keep things direct and minimal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2935545789
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +723,71 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ private final ByteBuffer value;
+
+ BoundingBoxLiteral(BoundingBox value) {
+ this.value = value.toByteBuffer();
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
+ Preconditions.checkNotNull(value, "Bounding box buffer cannot be null");
+ this.value = value.slice().order(ByteOrder.LITTLE_ENDIAN);
+ }
+
+ @Override
+ public ByteBuffer value() {
+ return value;
+ }
+
+ @Override
+ public ByteBuffer toByteBuffer() {
+ return value;
+ }
+
+ @Override
+ public <T> Literal<T> to(Type type) {
+ if (type.typeId() != Type.TypeID.GEOMETRY && type.typeId() != Type.TypeID.GEOGRAPHY) {
+ return null;
+ }
+
+ return (Literal<T>) this;
+ }
+
+ @Override
+ public Comparator<ByteBuffer> comparator() {
Review Comment:
Having a comparator makes it integrates more cleanly with the rest of the code base by avoiding special-casing [`BoundLiteralPredicate.test`](https://github.com/apache/iceberg/blob/a50c2d920aadbb79f978bac10d1271bc4aa63a1a/api/src/main/java/org/apache/iceberg/expressions/BoundLiteralPredicate.java#L73) and [`BoundLiteralPredicate.isEquivalentTo`](https://github.com/apache/iceberg/blob/a50c2d920aadbb79f978bac10d1271bc4aa63a1a/api/src/main/java/org/apache/iceberg/expressions/BoundLiteralPredicate.java#L104). I have reimplemented the comparator to have better semantics for BoundingBox values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2935549830
##########
api/src/main/java/org/apache/iceberg/types/Conversions.java:
##########
@@ -120,6 +120,12 @@ public static ByteBuffer toByteBuffer(Type.TypeID typeId, Object value) {
return (ByteBuffer) value;
case DECIMAL:
return ByteBuffer.wrap(((BigDecimal) value).unscaledValue().toByteArray());
+ case GEOMETRY:
+ case GEOGRAPHY:
+ // GEOMETRY and GEOGRAPHY values are represented as byte buffers. They are stored as WKB
+ // (Well-Known Binary) format in Iceberg, and geospatial bounding boxes are stored as
+ // serialized byte buffers. Return the byte buffer as is.
Review Comment:
Fixed comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830404594
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -126,6 +127,16 @@ public <T> R notStartsWith(BoundReference<T> ref, Literal<T> lit) {
"notStartsWith expression is not supported by the visitor");
}
+ public <T> R stIntersects(BoundReference<T> ref, Literal<ByteBuffer> lit) {
Review Comment:
Why does this only work with `ByteBuffer`? Wouldn't it be better to use `BoundingBox`?
My guess is that when the expression is bound, the literal gets coerced and the Java type we used for the geo types is `ByteBuffer`? I think we need to address this so that we don't deserialize this for every evaluation. This visitor is called in a tight loop and this would lose a lot of performance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830531241
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +723,71 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
Review Comment:
I think this should be `Literal<BoundingBox>` since that's what we need in the evaluators.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2831384159
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -126,6 +127,16 @@ public <T> R notStartsWith(BoundReference<T> ref, Literal<T> lit) {
"notStartsWith expression is not supported by the visitor");
}
+ public <T> R stIntersects(BoundReference<T> ref, Literal<ByteBuffer> lit) {
+ throw new UnsupportedOperationException(
+ "stIntersects expression is not supported by the visitor");
Review Comment:
Other methods such as `startsWith`, `in` and `notIn` also includes "by the visitor" in the error message. Would you like to have a separate PR to fix these error messages as well?
https://github.com/apache/iceberg/blob/176c729623fd370f777d78330f404bce1494a631/api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java#L111-L127
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2935534055
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -126,6 +127,16 @@ public <T> R notStartsWith(BoundReference<T> ref, Literal<T> lit) {
"notStartsWith expression is not supported by the visitor");
}
+ public <T> R stIntersects(BoundReference<T> ref, Literal<ByteBuffer> lit) {
Review Comment:
Changed `stIntersects` and `stDisjoint` to take `BoundingBox box` as parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#issuecomment-3419087197
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2610598858
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -318,7 +350,6 @@ public <T> R predicate(BoundPredicate<T> pred) {
"Invalid operation for BoundSetPredicate: " + pred.op());
}
}
-
Review Comment:
Change reverted
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -579,6 +593,8 @@ private static String sanitize(Literal<?> literal, long now, int today) {
return sanitizeNumber(((Literals.DoubleLiteral) literal).value(), "float");
} else if (literal instanceof Literals.VariantLiteral) {
return sanitizeVariant(((Literals.VariantLiteral) literal).value(), now, today);
+ } else if (literal instanceof BoundingBoxLiteral) {
+ return "(geospatial)";
Review Comment:
Change the sanitized string for bounding box literals.
I have also found that the expression sanitization code could be furture simplified as by product of addressing this review comment. I have removed 2 overloaded versions of `ExpressionUtil.sanitize`, the code still seemed to have the correct semantics and passed all related tests.
##########
core/src/test/java/org/apache/iceberg/expressions/TestExpressionParser.java:
##########
@@ -544,4 +563,122 @@ public void testNegativeScaleDecimalLiteral() {
assertThat(ExpressionParser.toJson(ExpressionParser.fromJson(expected), true))
.isEqualTo(expected);
}
+
+ @Test
+ public void testSpatialPredicate() {
+ String expected =
+ "{\n"
+ + " \"type\" : \"st-intersects\",\n"
Review Comment:
I have added this to `rest-catalog-open-api.yaml` and regenerated the Python code. This is a change to the REST Catalog API spec so maybe we want to have broader discussion about this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2935532989
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -202,6 +204,24 @@ public static UnboundPredicate<String> notStartsWith(UnboundTerm<String> expr, S
return new UnboundPredicate<>(Expression.Operation.NOT_STARTS_WITH, expr, value);
}
+ public static UnboundPredicate<ByteBuffer> stIntersects(String name, BoundingBox value) {
+ return geospatialPredicate(Operation.ST_INTERSECTS, name, value);
+ }
+
+ public static UnboundPredicate<ByteBuffer> stIntersects(
+ UnboundTerm<ByteBuffer> expr, BoundingBox value) {
Review Comment:
I have removed the APIs taking UnboundTerm parameters. The ones taking string parameter as field reference should be sufficient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2935547680
##########
api/src/main/java/org/apache/iceberg/expressions/SerializationProxies.java:
##########
@@ -81,4 +82,19 @@ protected byte[] bytes() {
return bytes;
}
}
+
+ static class BoundingBoxLiteralProxy extends FixedLiteralProxy {
+ /** Constructor for Java serialization. */
+ BoundingBoxLiteralProxy() {}
+
+ BoundingBoxLiteralProxy(ByteBuffer buffer) {
+ super(buffer);
+ }
+
+ @Override
+ Object readResolve() throws ObjectStreamException {
+ return new Literals.BoundingBoxLiteral(
+ ByteBuffer.wrap(bytes()).order(ByteOrder.LITTLE_ENDIAN));
Review Comment:
Removed `.order` call and the endianness requirement from `BoundingBox::fromByteBuffer`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#issuecomment-3553783844
@stevenzwu @szehon-ho @huaxingao @wgtmac @hsiang-c Would you please take a look at this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830404594
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -126,6 +127,16 @@ public <T> R notStartsWith(BoundReference<T> ref, Literal<T> lit) {
"notStartsWith expression is not supported by the visitor");
}
+ public <T> R stIntersects(BoundReference<T> ref, Literal<ByteBuffer> lit) {
Review Comment:
Why does this only work with `ByteBuffer`? Wouldn't it be better to use `BoundingBox`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830882385
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -202,6 +204,24 @@ public static UnboundPredicate<String> notStartsWith(UnboundTerm<String> expr, S
return new UnboundPredicate<>(Expression.Operation.NOT_STARTS_WITH, expr, value);
}
+ public static UnboundPredicate<ByteBuffer> stIntersects(String name, BoundingBox value) {
Review Comment:
`GeospatialBound` also have additionally optional dimensions `z` and `m`. Hence, I thought `BoundingBox(GeospatialBound min, GeospatialBound max)` is probably good here.
If we want to provide some convenient constructor, should we do it in `BoundingBox`?
```
public static BoundingBox from(xmin, xmax, ymin, ymax)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#issuecomment-3873522900
Sorry—I didn’t notice this PR is scheduled for discussion in the next community sync. Let’s wait and follow up after the sync.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2757377406
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java:
##########
@@ -509,4 +513,38 @@ public void testIsNullWithNestedStructs(List<Boolean> requiredFields, Expression
TestHelpers.assertAllReferencesBound("NotNull", bound);
assertThat(bound.op()).isEqualTo(expression.negate().op());
}
+
+ @Test
+ public void testStIntersects() {
+ // 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);
+
+ Expression expr = Expressions.stIntersects("point", bbox);
Review Comment:
should we also test geography type for intersects?
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java:
##########
@@ -509,4 +513,38 @@ public void testIsNullWithNestedStructs(List<Boolean> requiredFields, Expression
TestHelpers.assertAllReferencesBound("NotNull", bound);
assertThat(bound.op()).isEqualTo(expression.negate().op());
}
+
+ @Test
+ public void testStIntersects() {
+ // 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);
+
+ Expression expr = Expressions.stIntersects("point", bbox);
+ Expression bound = Binder.bind(STRUCT, expr);
+
+ TestHelpers.assertAllReferencesBound("ST_Intersects", bound);
+ BoundPredicate<?> pred = TestHelpers.assertAndUnwrap(bound);
+ assertThat(pred.op()).isEqualTo(Expression.Operation.ST_INTERSECTS);
+ assertThat(pred.term().ref().fieldId()).as("Should bind point correctly").isEqualTo(7);
+ assertThat(pred.asLiteralPredicate().literal().value()).isEqualTo(bbox.toByteBuffer());
+ }
+
+ @Test
+ public void testStDisjoint() {
+ // 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);
+
+ Expression expr = Expressions.stDisjoint("geography", bbox);
Review Comment:
should we test geometry type for disjoint?
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java:
##########
@@ -62,7 +64,9 @@ public class TestExpressionBinding {
required(3, "data", Types.StringType.get()),
required(4, "var", Types.VariantType.get()),
optional(5, "nullable", Types.IntegerType.get()),
- optional(6, "always_null", Types.UnknownType.get()));
+ optional(6, "always_null", Types.UnknownType.get()),
+ required(7, "point", Types.GeometryType.crs84()),
Review Comment:
nit: name the field `geometry` as did for other test classes?
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionUtil.java:
##########
@@ -1315,6 +1324,92 @@ private static VariantArray createArrayWithNestedTypes() {
return (VariantArray) VariantValue.from(metadata, variantBB);
}
+ private static Stream<Arguments> geospatialPredicateParameters() {
+ GeospatialBound min = GeospatialBound.createXY(1.0, 2.0);
+ GeospatialBound max = GeospatialBound.createXY(3.0, 4.0);
+ BoundingBox bbox = new BoundingBox(min, max);
+
+ GeospatialBound minXYZ = GeospatialBound.createXYZ(1.0, 2.0, 3.0);
+ GeospatialBound maxXYZ = GeospatialBound.createXYZ(3.0, 4.0, 5.0);
+ BoundingBox bboxXYZ = new BoundingBox(minXYZ, maxXYZ);
+
+ GeospatialBound minXYM = GeospatialBound.createXYM(1.0, 2.0, 3.0);
+ GeospatialBound maxXYM = GeospatialBound.createXYM(3.0, 4.0, 5.0);
+ BoundingBox bboxXYM = new BoundingBox(minXYM, maxXYM);
+
+ GeospatialBound minXYZM = GeospatialBound.createXYZM(1.0, 2.0, 3.0, 4.0);
+ GeospatialBound maxXYZM = GeospatialBound.createXYZM(3.0, 4.0, 5.0, 6.0);
+ BoundingBox bboxXYZM = new BoundingBox(minXYZM, maxXYZM);
+
+ return Stream.of(
+ Arguments.of(
+ Expressions.stIntersects("geometry", bbox),
+ "st_intersects(geometry, (boundingbox-xy))",
+ "(boundingbox-xy)"),
+ Arguments.of(
+ Expressions.stIntersects("geography", bbox),
+ "st_intersects(geography, (boundingbox-xy))",
+ "(boundingbox-xy)"),
+ Arguments.of(
+ Expressions.stDisjoint("geometry", bbox),
+ "st_disjoint(geometry, (boundingbox-xy))",
+ "(boundingbox-xy)"),
+ Arguments.of(
+ Expressions.stDisjoint("geography", bbox),
+ "st_disjoint(geography, (boundingbox-xy))",
+ "(boundingbox-xy)"),
+ Arguments.of(
+ Expressions.stIntersects("geometry", bboxXYZ),
+ "st_intersects(geometry, (boundingbox-xyz))",
+ "(boundingbox-xyz)"),
+ Arguments.of(
+ Expressions.stIntersects("geometry", bboxXYM),
+ "st_intersects(geometry, (boundingbox-xym))",
+ "(boundingbox-xym)"),
+ Arguments.of(
+ Expressions.stIntersects("geometry", bboxXYZM),
+ "st_intersects(geometry, (boundingbox-xyzm))",
+ "(boundingbox-xyzm)"));
+ }
+
+ @ParameterizedTest
+ @MethodSource("geospatialPredicateParameters")
+ public void testSanitizeGeospatialPredicates(
+ UnboundPredicate<ByteBuffer> geoPredicate,
+ String expectedSanitizedString,
+ String expectedLiteral) {
+ Expression.Operation operation = geoPredicate.op();
+ String columnName = geoPredicate.term().ref().name();
+
+ Expression predicateSanitized = Expressions.predicate(operation, columnName, expectedLiteral);
+ assertEquals(predicateSanitized, ExpressionUtil.sanitize(geoPredicate));
+ assertEquals(predicateSanitized, ExpressionUtil.sanitize(STRUCT, geoPredicate, true));
+
+ assertThat(ExpressionUtil.toSanitizedString(geoPredicate))
+ .as("Sanitized string should be identical for geospatial predicates")
+ .isEqualTo(expectedSanitizedString);
+
+ assertThat(ExpressionUtil.toSanitizedString(STRUCT, geoPredicate, true))
+ .as("Sanitized string should be identical for geospatial predicates")
+ .isEqualTo(expectedSanitizedString);
+ }
+
+ @Test
+ public void testBoundingBoxLiteralNormalizesBuffer() {
+ GeospatialBound min = GeospatialBound.createXY(1.0, 2.0);
+ GeospatialBound max = GeospatialBound.createXY(3.0, 4.0);
+ BoundingBox box = new BoundingBox(min, max);
+ ByteBuffer serialized = box.toByteBuffer();
+
+ ByteBuffer padded = ByteBuffer.allocate(serialized.remaining() + 1).order(ByteOrder.BIG_ENDIAN);
+ padded.put((byte) 0x0);
Review Comment:
curious about why testing the padded byte buffer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830556733
##########
api/src/main/java/org/apache/iceberg/types/Conversions.java:
##########
@@ -120,6 +120,12 @@ public static ByteBuffer toByteBuffer(Type.TypeID typeId, Object value) {
return (ByteBuffer) value;
case DECIMAL:
return ByteBuffer.wrap(((BigDecimal) value).unscaledValue().toByteArray());
+ case GEOMETRY:
+ case GEOGRAPHY:
+ // GEOMETRY and GEOGRAPHY values are represented as byte buffers. They are stored as WKB
+ // (Well-Known Binary) format in Iceberg, and geospatial bounding boxes are stored as
+ // serialized byte buffers. Return the byte buffer as is.
Review Comment:
This should not be used for `BoundingBox`. This is for values, not for what you may compare values against.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#issuecomment-3869804627
Thanks all. This PR looks in good shape and I’d like to merge it soon. I’ll wait until Wednesday for any additional feedback from @hsiang-c @wgtmac @rdblue; if there are no further comments by then, I’ll go ahead and merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2616507394
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java:
##########
@@ -284,6 +288,15 @@ public String toString() {
return term() + " startsWith \"" + literal() + "\"";
case NOT_STARTS_WITH:
return term() + " notStartsWith \"" + literal() + "\"";
+ case ST_INTERSECTS:
+ case ST_DISJOINT:
+ {
+ ByteBuffer serializedBoundingBox = (ByteBuffer) literal().value();
+ serializedBoundingBox.order(ByteOrder.LITTLE_ENDIAN);
Review Comment:
This changes the ByteBuffer object stored inside the literal. Even though it’s just the byte order, toString() should be side-effect free.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#issuecomment-3769980466
@stevenzwu @huaxingao @wgtmac Hi folks, happy new year! Can you please take a look at this PR again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2843295145
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +723,71 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
Review Comment:
I don't think this requires breaking constraints for `UnboundPredicate`. Those constraints are not violated:
* `T` is a type held by a literal, in this case `BoundingBox`
* `Literal<T>` is a literal that holds that type, which is `Literal<BoundingBox>` implemented by `BoundingBoxLiteral`
* `List<Literal<T>>` is a holder in case the unbound expression has multiple literals. It won't in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2935539899
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +723,71 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
Review Comment:
BoundingBoxLiteral now implements `Literal<BoundingBox>`
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +723,71 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ private final ByteBuffer value;
+
+ BoundingBoxLiteral(BoundingBox value) {
+ this.value = value.toByteBuffer();
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
Review Comment:
Removed this constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2829655724
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -202,6 +204,24 @@ public static UnboundPredicate<String> notStartsWith(UnboundTerm<String> expr, S
return new UnboundPredicate<>(Expression.Operation.NOT_STARTS_WITH, expr, value);
}
+ public static UnboundPredicate<ByteBuffer> stIntersects(String name, BoundingBox value) {
Review Comment:
Why is this a `BoundingBox` rather than passing values directly? Should we have a version that accepts xmin, xmax, ymin, ymax for simple cases?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2831384159
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -126,6 +127,16 @@ public <T> R notStartsWith(BoundReference<T> ref, Literal<T> lit) {
"notStartsWith expression is not supported by the visitor");
}
+ public <T> R stIntersects(BoundReference<T> ref, Literal<ByteBuffer> lit) {
+ throw new UnsupportedOperationException(
+ "stIntersects expression is not supported by the visitor");
Review Comment:
Other methods such as `startsWith`, `in` and `notIn` also includes "by the visitor" in the error message. Would you like to have a separate PR to fix these error messages as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830496506
##########
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:
I don't think the Java type needs to match. I think we just need to handle this in `UnboundPredicate.bind`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830534062
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +723,71 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ private final ByteBuffer value;
+
+ BoundingBoxLiteral(BoundingBox value) {
+ this.value = value.toByteBuffer();
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
Review Comment:
I don't think this is needed. Conversion from `Literal<ByteBuffer>` should be done in `BinaryLiteral` and from `Literal<byte[]>` in `FixedLiteral`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#issuecomment-3844471295
cc @hsiang-c @wgtmac @rdblue for reviews
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
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)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#issuecomment-4240484445
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830882385
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -202,6 +204,24 @@ public static UnboundPredicate<String> notStartsWith(UnboundTerm<String> expr, S
return new UnboundPredicate<>(Expression.Operation.NOT_STARTS_WITH, expr, value);
}
+ public static UnboundPredicate<ByteBuffer> stIntersects(String name, BoundingBox value) {
Review Comment:
`GeospatialBound` also have additionally optional dimensions `z` and `m`. Hence, I thought `BoundingBox(GeospatialBound min, GeospatialBound max)` is probably good here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2706671033
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +722,70 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ private final ByteBuffer value;
+
+ BoundingBoxLiteral(BoundingBox value) {
+ this.value = value.toByteBuffer();
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
+ this.value = value;
Review Comment:
Normalized the ByteBuffer in this constructor. Actually this constructor is only called by `BoundingBoxLiteralProxy`. The ByteBuffer passed into it always has position=0 and byteOrder=LITTLE_ENDIAN. We do this normalization to guard future usages of this constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2610605597
##########
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:
`BoundingBox` is the only geo literal we plan to add in the series of PRs. I think the current design is open to adding more types of geospatial literals.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2609042696
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +722,45 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral extends BaseLiteral<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ BoundingBoxLiteral(BoundingBox value) {
+ super(value.toByteBuffer());
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
+ super(value);
+ }
+
+ @Override
+ protected Type.TypeID typeId() {
+ return Type.TypeID.GEOMETRY;
Review Comment:
Yes, this is indeed strange, although this implementation works in this case since `typeId()` is only used by [`BaseLiteral.toByteBuffer`](https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/expressions/Literals.java#L119-L129) for serializing the value of the literal.
I found 2 ways to fix this weirdness:
1. Making `BoundingBoxLiteral` directly implementing `interface Literal`, but not extends from `BaseLiteral`. The `Literal` interface does not have the `typeId` method we don't have to come up with a confusing implementation.
2. Adding geo type to `BoundingBox`. Check that the arguments passed to spatial predicates must have the same geo type.
I am working towards approach 2 and see if everything fits perfectly after revising the `BoundingBox` definition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2843280585
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -166,6 +178,12 @@ public <T> R predicate(BoundPredicate<T> pred) {
return startsWith((BoundReference<T>) pred.term(), literalPred.literal());
case NOT_STARTS_WITH:
return notStartsWith((BoundReference<T>) pred.term(), literalPred.literal());
+ case ST_INTERSECTS:
+ return stIntersects(
+ (BoundReference<T>) pred.term(), (Literal<ByteBuffer>) literalPred.literal());
Review Comment:
Yes, we may still need to do an unchecked cast. This is an odd situation where the literal and term don't produce the same thing. I would rather have an unchecked cast here and pass `BoundingBox` than try to make everything work as `ByteBuffer`, where the Java type matches but the actual values are completely different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2621614107
##########
core/src/test/java/org/apache/iceberg/expressions/TestExpressionParser.java:
##########
@@ -544,4 +563,122 @@ public void testNegativeScaleDecimalLiteral() {
assertThat(ExpressionParser.toJson(ExpressionParser.fromJson(expected), true))
.isEqualTo(expected);
}
+
+ @Test
+ public void testSpatialPredicate() {
+ String expected =
+ "{\n"
+ + " \"type\" : \"st-intersects\",\n"
Review Comment:
I have filed https://github.com/apache/iceberg/pull/14856 for discussing the REST Catalog OpenAPI spec changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2628488194
##########
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:
nit: add to the end of the list?
##########
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:
nit: make this a static constant?
##########
api/src/test/java/org/apache/iceberg/types/TestConversions.java:
##########
@@ -45,6 +47,7 @@
public class TestConversions {
@Test
+ @SuppressWarnings("MethodLength")
Review Comment:
nit: I don't know why we put all those tests in one giant test method. For the new geo byte buffer conversion, maybe we can put them into a separate test method.
##########
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:
wondering if we should make `Expressions.geospatialPredicate` private or package private. it seems to be only used by test code.
- In some tests, It can be simply replaced by calling the `Expressions.stIntersects` public method.
- Some have parameterized tests (like `TestExpressionUtil`). wondering if the parameters can be replaced as `UnboundPredicate` type.
The goal is to expose minimal scope especially for `api` module.
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java:
##########
@@ -284,6 +287,14 @@ public String toString() {
return term() + " startsWith \"" + literal() + "\"";
case NOT_STARTS_WITH:
return term() + " notStartsWith \"" + literal() + "\"";
+ case ST_INTERSECTS:
+ case ST_DISJOINT:
+ {
+ ByteBuffer serializedBoundingBox = (ByteBuffer) literal().value();
Review Comment:
`BoundingBoxLiteral` should probably implement `toString` method to simplify the code here.
##########
api/src/main/java/org/apache/iceberg/expressions/Evaluator.java:
##########
@@ -156,5 +157,15 @@ public <T> Boolean startsWith(Bound<T> valueExpr, Literal<T> lit) {
public <T> Boolean notStartsWith(Bound<T> valueExpr, Literal<T> lit) {
return !startsWith(valueExpr, lit);
}
+
+ @Override
+ public <T> Boolean stIntersects(Bound<T> valueExpr, Literal<ByteBuffer> literal) {
+ throw new UnsupportedOperationException("Evaluation of stIntersects is not implemented.");
Review Comment:
should this class change be postponed to the next PR?
##########
core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java:
##########
@@ -229,6 +242,44 @@ private void unboundLiteral(Object object) throws IOException {
}
}
+ private void geospatialBoundingBox(BoundingBox value) throws IOException {
Review Comment:
maybe move the bounding box code to a separate package private `BoundingBoxParser` class?
##########
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:
also let's use the full name like `geometry` and `geography`. The current shorter name doesn't seem like commonly recognizable.
##########
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:
since bounding box literal is a byte buffer type, it can be json serialized as as binary types. I guess the special handling here is to make the json serialization more human readable.
##########
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:
the 3rd arg string of `(boundingbox)` is a bit weird to me. shouldn't it be a byte buffer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2635273221
##########
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:
`BoundingBoxLiteral` implements `Literal<ByteBuffer>` to have compatible type with the Java types of iceberg geospatial types, so the return type was defined as `Literal<ByteBuffer>`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #14101: [WIP] API, Core: Add geospatial predicates and bounding box literals
URL: https://github.com/apache/iceberg/pull/14101
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2629460189
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java:
##########
@@ -284,6 +287,14 @@ public String toString() {
return term() + " startsWith \"" + literal() + "\"";
case NOT_STARTS_WITH:
return term() + " notStartsWith \"" + literal() + "\"";
+ case ST_INTERSECTS:
+ case ST_DISJOINT:
+ {
+ ByteBuffer serializedBoundingBox = (ByteBuffer) literal().value();
Review Comment:
Implemented toString for `BoundingBoxLiteral`.
##########
api/src/main/java/org/apache/iceberg/expressions/Evaluator.java:
##########
@@ -156,5 +157,15 @@ public <T> Boolean startsWith(Bound<T> valueExpr, Literal<T> lit) {
public <T> Boolean notStartsWith(Bound<T> valueExpr, Literal<T> lit) {
return !startsWith(valueExpr, lit);
}
+
+ @Override
+ public <T> Boolean stIntersects(Bound<T> valueExpr, Literal<ByteBuffer> literal) {
+ throw new UnsupportedOperationException("Evaluation of stIntersects is not implemented.");
Review Comment:
Removed them from this PR.
##########
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:
The operators after it are aggregation operators. I'd like to group ordinary operators together before the aggregation operators.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2628451932
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +722,45 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral extends BaseLiteral<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ BoundingBoxLiteral(BoundingBox value) {
+ super(value.toByteBuffer());
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
+ super(value);
+ }
+
+ @Override
+ protected Type.TypeID typeId() {
+ return Type.TypeID.GEOMETRY;
Review Comment:
I agree with the reasoning. Let's go with approach 1 for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2706366191
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +722,70 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ private final ByteBuffer value;
+
+ BoundingBoxLiteral(BoundingBox value) {
+ this.value = value.toByteBuffer();
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
+ this.value = value;
Review Comment:
nit: `BoundingBox.fromByteBuffer` requires position=0 and LITTLE_ENDIAN. Do we need to normalize `value` so `toString()`/`ExpressionUtil.sanitize` won’t throw if a caller passes a sliced/big-endian buffer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2829662164
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -202,6 +204,24 @@ public static UnboundPredicate<String> notStartsWith(UnboundTerm<String> expr, S
return new UnboundPredicate<>(Expression.Operation.NOT_STARTS_WITH, expr, value);
}
+ public static UnboundPredicate<ByteBuffer> stIntersects(String name, BoundingBox value) {
+ return geospatialPredicate(Operation.ST_INTERSECTS, name, value);
+ }
+
+ public static UnboundPredicate<ByteBuffer> stIntersects(
+ UnboundTerm<ByteBuffer> expr, BoundingBox value) {
Review Comment:
What unbound term is this intended to accept? I don't think that we have anything to pass here other than refs and if that's the case we should remove this. We don't add anything to the public API that isn't actually needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830400349
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -126,6 +127,16 @@ public <T> R notStartsWith(BoundReference<T> ref, Literal<T> lit) {
"notStartsWith expression is not supported by the visitor");
}
+ public <T> R stIntersects(BoundReference<T> ref, Literal<ByteBuffer> lit) {
+ throw new UnsupportedOperationException(
+ "stIntersects expression is not supported by the visitor");
Review Comment:
No need for "by the visitor" because this is clear from the stack trace.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830551138
##########
api/src/main/java/org/apache/iceberg/expressions/SerializationProxies.java:
##########
@@ -81,4 +82,19 @@ protected byte[] bytes() {
return bytes;
}
}
+
+ static class BoundingBoxLiteralProxy extends FixedLiteralProxy {
+ /** Constructor for Java serialization. */
+ BoundingBoxLiteralProxy() {}
+
+ BoundingBoxLiteralProxy(ByteBuffer buffer) {
+ super(buffer);
+ }
+
+ @Override
+ Object readResolve() throws ObjectStreamException {
+ return new Literals.BoundingBoxLiteral(
+ ByteBuffer.wrap(bytes()).order(ByteOrder.LITTLE_ENDIAN));
Review Comment:
By setting `order`, this changes the buffer that was serialized. I think that the expression code should leave the byte order alone and the `BoundingBox` serialization and deserialization should handle it. That serialization should also be duplicating the `ByteBuffer` anyway because it should not be consuming or changing the incoming buffer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830543378
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +723,71 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ private final ByteBuffer value;
+
+ BoundingBoxLiteral(BoundingBox value) {
+ this.value = value.toByteBuffer();
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
+ Preconditions.checkNotNull(value, "Bounding box buffer cannot be null");
+ this.value = value.slice().order(ByteOrder.LITTLE_ENDIAN);
+ }
+
+ @Override
+ public ByteBuffer value() {
+ return value;
+ }
+
+ @Override
+ public ByteBuffer toByteBuffer() {
+ return value;
+ }
+
+ @Override
+ public <T> Literal<T> to(Type type) {
+ if (type.typeId() != Type.TypeID.GEOMETRY && type.typeId() != Type.TypeID.GEOGRAPHY) {
+ return null;
+ }
+
+ return (Literal<T>) this;
+ }
+
+ @Override
+ public Comparator<ByteBuffer> comparator() {
+ return CMP;
+ }
+
+ Object writeReplace() throws ObjectStreamException {
+ return new SerializationProxies.BoundingBoxLiteralProxy(value());
+ }
+
+ @Override
+ public String toString() {
+ return BoundingBox.fromByteBuffer(value()).toString();
+ }
+
+ @Override
+ public boolean equals(Object other) {
+ if (this == other) {
+ return true;
+ }
+ if (!(other instanceof BoundingBoxLiteral)) {
+ return false;
+ }
+
+ BoundingBoxLiteral that = (BoundingBoxLiteral) other;
+ return comparator().compare(value(), that.value()) == 0;
Review Comment:
If this works, then the comparator is incorrect for values. This is comparing serialized bounding boxes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830432329
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -166,6 +178,12 @@ public <T> R predicate(BoundPredicate<T> pred) {
return startsWith((BoundReference<T>) pred.term(), literalPred.literal());
case NOT_STARTS_WITH:
return notStartsWith((BoundReference<T>) pred.term(), literalPred.literal());
+ case ST_INTERSECTS:
+ return stIntersects(
+ (BoundReference<T>) pred.term(), (Literal<ByteBuffer>) literalPred.literal());
Review Comment:
This unchecked cast is suspicious and should be fixed. The solution is probably to fix the literal type from binding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2830486715
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -126,6 +127,16 @@ public <T> R notStartsWith(BoundReference<T> ref, Literal<T> lit) {
"notStartsWith expression is not supported by the visitor");
}
+ public <T> R stIntersects(BoundReference<T> ref, Literal<ByteBuffer> lit) {
Review Comment:
I think that the right signature for this is `stIntersects(BoundReference<T>, BoundingBox)` or maybe with `BoundReference<ByteBuffer>`. The use of `T` in both `BoundReference` and `Literal` is because we want comparisons to be able to operate without requiring casting. That way, `ref.comparator()` is a `Comparator<T>` and both `ref.accessor().get(row)` and `lit.value()` are as well.
This is correct that we don't need `T` to match, but it's awkward to get a `BoundReference<T>` and a concrete `Literal`. Since we are going to need to trust binding to geo types, we can update the visitor to pass the `BoundingBox`. I'm not sure that we want to wrap `BoundingBox` in `Literal`, but I'm learning against doing it. I think we should just pass the right types here. We can unwrap the literal from the `BoundingBox` in the visitor and the cast will guarantee the type is correct, rather than putting the responsibility on the implementations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2935533381
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java:
##########
@@ -126,6 +127,16 @@ public <T> R notStartsWith(BoundReference<T> ref, Literal<T> lit) {
"notStartsWith expression is not supported by the visitor");
}
+ public <T> R stIntersects(BoundReference<T> ref, Literal<ByteBuffer> lit) {
+ throw new UnsupportedOperationException(
+ "stIntersects expression is not supported by the visitor");
Review Comment:
Removed "by the visitor" from the error messages
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2831361210
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +723,71 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral implements Literal<ByteBuffer> {
Review Comment:
Implementing `Literal<ByteBuffer>` was for not breaking the type constraint of `List<Literal<T>> literals` in `UnboundPredicate<ByteBuffer>` and `Literal<T> literal` of `BoundLiteralPredicate<ByteBuffer>`. Do you suggest that we break the constraint for spatial predicates involving bounding box literals, or do a major refactoring by implementing new variants of predicates?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2831349208
##########
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:
Do you suggest that we hold `Literal<BoundingBox>` in the `literals` field of `UnboundPredicate<ByteBuffer>` by violating the type signature of `List<Literal<T>> literals`? I think we can still make it compile, but I just want to make sure that it is the right way to do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2757217381
##########
api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java:
##########
@@ -518,44 +527,6 @@ private static List<String> abbreviateValues(List<String> sanitizedValues) {
return sanitizedValues;
}
- private static String sanitize(Type type, Literal<?> lit, long now, int today) {
Review Comment:
this looks like refactoring existing code. maybe extract it to a separate small PR to get it in quickly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2757945323
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java:
##########
@@ -509,4 +513,38 @@ public void testIsNullWithNestedStructs(List<Boolean> requiredFields, Expression
TestHelpers.assertAllReferencesBound("NotNull", bound);
assertThat(bound.op()).isEqualTo(expression.negate().op());
}
+
+ @Test
+ public void testStIntersects() {
+ // 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);
+
+ Expression expr = Expressions.stIntersects("point", bbox);
+ Expression bound = Binder.bind(STRUCT, expr);
+
+ TestHelpers.assertAllReferencesBound("ST_Intersects", bound);
+ BoundPredicate<?> pred = TestHelpers.assertAndUnwrap(bound);
+ assertThat(pred.op()).isEqualTo(Expression.Operation.ST_INTERSECTS);
+ assertThat(pred.term().ref().fieldId()).as("Should bind point correctly").isEqualTo(7);
+ assertThat(pred.asLiteralPredicate().literal().value()).isEqualTo(bbox.toByteBuffer());
+ }
+
+ @Test
+ public void testStDisjoint() {
+ // 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);
+
+ Expression expr = Expressions.stDisjoint("geography", bbox);
Review Comment:
I have refactored testStIntersectsBinding and testStDisjointBinding to be parameterized and test both geometry type and geography type.
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java:
##########
@@ -509,4 +513,38 @@ public void testIsNullWithNestedStructs(List<Boolean> requiredFields, Expression
TestHelpers.assertAllReferencesBound("NotNull", bound);
assertThat(bound.op()).isEqualTo(expression.negate().op());
}
+
+ @Test
+ public void testStIntersects() {
+ // 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);
+
+ Expression expr = Expressions.stIntersects("point", bbox);
Review Comment:
I have refactored testStIntersectsBinding and testStDisjointBinding to be parameterized and test both geometry type and geography type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2635235420
##########
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:
Updated the patch to produce sanitized string such as `(boundingbox-xy)`, `(boundingbox-xyz)`, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2621612647
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java:
##########
@@ -284,6 +288,15 @@ public String toString() {
return term() + " startsWith \"" + literal() + "\"";
case NOT_STARTS_WITH:
return term() + " notStartsWith \"" + literal() + "\"";
+ case ST_INTERSECTS:
+ case ST_DISJOINT:
+ {
+ ByteBuffer serializedBoundingBox = (ByteBuffer) literal().value();
+ serializedBoundingBox.order(ByteOrder.LITTLE_ENDIAN);
Review Comment:
I have removed this `.order` call. The reason why I needed it here was because `BoundingBoxLiteralProxy` was not correctly setting the byte order of deserialized byte buffer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "Kontinuation (via GitHub)" <gi...@apache.org>.
Kontinuation commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2622371223
##########
api/src/main/java/org/apache/iceberg/expressions/Literals.java:
##########
@@ -719,4 +722,45 @@ public String toString() {
return "X'" + BaseEncoding.base16().encode(bytes) + "'";
}
}
+
+ static class BoundingBoxLiteral extends BaseLiteral<ByteBuffer> {
+ private static final Comparator<ByteBuffer> CMP =
+ Comparators.<ByteBuffer>nullsFirst().thenComparing(Comparators.unsignedBytes());
+
+ BoundingBoxLiteral(BoundingBox value) {
+ super(value.toByteBuffer());
+ }
+
+ BoundingBoxLiteral(ByteBuffer value) {
+ super(value);
+ }
+
+ @Override
+ protected Type.TypeID typeId() {
+ return Type.TypeID.GEOMETRY;
Review Comment:
I decided to switch to approach 1 and remove the typeId() method from `BoundingBoxLiteral`. Here are the reasons:
1. If `BoundingBox` carries typeId, it should also carry the full fledged type information including CRS and algorithm. Otherwise it would be strange that only the geospatial type ID is explicitly specified while other type attributes inherit from the type of the field.
2. Iceberg needs to check that the type of the field and the bounding box matches with each other when binding and evaluating spatial predicates. The CRS and algorithm also need to match.
3. Once the query engine checked that the geospatial field is supported by the engine and decided to generate a bounding box for pushing down a spatial predicate, it needs extract the geospatial type attributes from the Iceberg schema to construct the `BoundingBox`. The CRS attribute is tightly coupled with the iceberg table since it is the name of the table property defining the full CRS in PROJJSON, so there's a high chance that the engine will extract field type in iceberg table schema directly use it to construct the bounding box. Inferring the type of BoundingBox according to the context would eliminate the need for this extra step.
4. The untyped BoundingBox looks concise in the REST Catalog SPEC: https://github.com/apache/iceberg/pull/14856. Adding type information to the bounding box would make the data model more cluttered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2843284970
##########
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:
I'm saying that `BoundingBoxLiteral` should be `Literal<BoundingBox>` and not `Literal<ByteBuffer>`. We don't need the term and literal types to match later on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API, Core: Add geospatial predicates and bounding box literals [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14101:
URL: https://github.com/apache/iceberg/pull/14101#discussion_r2616507503
##########
core/src/test/java/org/apache/iceberg/expressions/TestExpressionParser.java:
##########
@@ -544,4 +563,122 @@ public void testNegativeScaleDecimalLiteral() {
assertThat(ExpressionParser.toJson(ExpressionParser.fromJson(expected), true))
.isEqualTo(expected);
}
+
+ @Test
+ public void testSpatialPredicate() {
+ String expected =
+ "{\n"
+ + " \"type\" : \"st-intersects\",\n"
Review Comment:
Maybe split the OpenAPI spec/codegen changes into a follow‑up (since spec changes usually require a vote)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org