github-actions[bot] commented on code in PR #69032:
URL: https://github.com/apache/doris/pull/69032#discussion_r4269360799
##########
be/src/format_v2/parquet/native_schema_desc.cpp:
##########
@@ -844,8 +845,19 @@ std::pair<DataTypePtr, bool>
NativeFieldDescriptor::convert_to_doris_type(
tparquet::LogicalType logicalType, bool nullable) {
std::pair<DataTypePtr, bool> ans = {std::make_shared<DataTypeNothing>(),
false};
bool& is_type_compatibility = ans.second;
- if (logicalType.__isset.STRING || logicalType.__isset.ENUM ||
logicalType.__isset.JSON ||
- logicalType.__isset.BSON) {
+ if (logicalType.__isset.GEOMETRY) {
+ const std::string crs =
+ logicalType.GEOMETRY.__isset.crs ? logicalType.GEOMETRY.crs :
"OGC:CRS84";
+ ans.first = std::make_shared<DataTypeSpatial>(TYPE_GEOMETRY, crs);
Review Comment:
[P1] Preserve nullability when constructing spatial file types
For an OPTIONAL annotated GEOMETRY/GEOGRAPHY field, these branches ignore
`nullable` and return a bare `DataTypeSpatial`. `build_native_node_schema`
consequently sets `nullable_definition_level` to 0. The public schema is
wrapped as Nullable, so `COUNT(shape)` pushdown remains admissible, but
`count_loaded_non_null_values` counts NULL definition level 0 as a value. An
annotated Iceberg column containing `[POINT(1 2), NULL]` therefore returns 2
instead of 1 with FileScannerV2 and COUNT pushdown enabled. Honor `nullable`
here and in the parallel v1 branches, and cover COUNT on actual optional
spatial files.
##########
fe/fe-type/src/main/java/org/apache/doris/catalog/PrimitiveType.java:
##########
@@ -83,7 +83,9 @@ public enum PrimitiveType {
VARIANT("VARIANT", 24, TPrimitiveType.VARIANT, false),
TEMPLATE("TEMPLATE", -1, TPrimitiveType.INVALID_TYPE, false),
// Unsupported scalar types.
- BINARY("BINARY", -1, TPrimitiveType.BINARY, false);
+ BINARY("BINARY", -1, TPrimitiveType.BINARY, false),
+ GEOMETRY("GEOMETRY", 16, TPrimitiveType.GEOMETRY, false),
Review Comment:
[P2] Mark spatial MySQL results as binary WKB
These new types inherit the Varbinary SerDe and send raw WKB, but
`toMysqlType` falls through to `MYSQL_TYPE_STRING` and `MysqlSerializer` only
assigns binary charset/flags to VARBINARY. Thus `SELECT
ST_GeometryFromWKBTyped('0101000000000000000000F03F0000000000000040')`
advertises UTF-8 text containing bytes `F0 3F`; default Unicode PyMySQL
decoding raises `UnicodeDecodeError` instead of returning the value. Its
[result
decoder](https://github.com/PyMySQL/PyMySQL/blob/main/pymysql/connections.py)
uses the advertised charset to decide whether to decode bytes. Extend spatial
result metadata to the existing binary contract, including charset 63, with
direct-result coverage for both kinds.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -5391,6 +5382,21 @@ public DataType
visitAggStateDataType(AggStateDataTypeContext ctx) {
});
}
+ @Override
+ public DataType visitSpatialDataType(SpatialDataTypeContext ctx) {
+ return ParserUtils.withOrigin(ctx, () -> {
+ String crs = ctx.crs == null ? "OGC:CRS84" :
stripQuotes(ctx.crs.getText());
Review Comment:
[P2] Unescape spatial parameters as SQL string literals
`stripQuotes` removes only delimiters, whereas `ScalarType.toSql` escapes
apostrophes by doubling them. A CRS such as `projjson:custom'crs` renders as
`GEOMETRY('projjson:custom''crs')` and reparses with two apostrophes, changing
CRS identity and the referenced property; SHOW CREATE/recreate doubles it
again. The [Iceberg CRS contract](https://iceberg.apache.org/spec/#crs) permits
unique strings and table-property references. Use the normal
`SqlLiteralUtils.parseStringLiteral` path, as `visitStringLiteral` does, and
cover an escaped CRS round trip.
##########
be/src/exprs/function/geo/functions_geo.cpp:
##########
@@ -360,7 +527,7 @@ struct StAzimuth {
}
double angle = 0;
- if (!GeoPoint::ComputeAzimuth(&point1, &point2, &angle)) {
+ if (!GeoPoint::ComputeAzimuth(pt1, pt2, &angle)) {
Review Comment:
[P2] Use a correct spherical bearing path for typed Geography
New Geography points reach `ComputeAzimuth -> ComputeAngle`, which calls
`S2LatLng::FromDegrees(x,y)` although x is longitude and [S2 expects latitude
first](https://github.com/google/s2geometry/blob/master/src/s2/s2latlng.h).
This also turns `north(0,90)` into latitude 0/longitude 90. For `POINT(10 45)
-> POINT(10 46)`, the source formula yields about 0.173449 radians instead of
due north's 0. Simply swapping axes still leaves `b-a` opposite the [clockwise
bearing
convention](https://doris.apache.org/docs/4.x/sql-manual/sql-functions/scalar-functions/spatial-functions/st-azimuth/):
east `(0 0)->(1 0)` would yield 3pi/2 instead of pi/2. Use the correct
spherical clockwise bearing/angle computation and test non-equatorial cardinal
directions; `ST_Angle` shares this helper.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/StWithin.java:
##########
@@ -70,6 +70,12 @@ public List<FunctionSignature> getSignatures() {
return SIGNATURES;
}
+ @Override
+ public FunctionSignature searchSignature(List<FunctionSignature>
signatures) {
+ FunctionSignature signature =
SpatialFunctionSignature.binary(child(0), child(1), BooleanType.INSTANCE);
Review Comment:
[P2] Reject unimplemented Geography containment pairs
The new signature admits identical nonempty Geography points, but
`ST_Within(p,p)` reaches `GeoShape::within -> rhs->contains(this)`; `GeoPoint`
and `GeoLine` do not override `contains`, so the base method returns false
without testing containment. Thus `ST_Intersects(p,p)` is true while
`ST_Within`/`ST_Contains(p,p)` are false, and the new golden locks in that
result. [Within is reflexive](https://postgis.net/docs/ST_Within.html), and the
PR requires unsupported combinations to fail explicitly. Implement the
supported typed pairs, or return `NotSupported` for unimplemented combinations,
and correct the self-point golden.
##########
be/src/exprs/function/geo/functions_geo.cpp:
##########
@@ -507,24 +692,32 @@ struct StRelationFunction {
static Status execute(Block& block, const ColumnNumbers& arguments, size_t
result) {
DCHECK_EQ(arguments.size(), 2);
auto return_type = block.get_data_type(result);
- auto left_col =
ColumnView<TYPE_STRING>::create(block.get_by_position(arguments[0]).column);
- auto right_col =
-
ColumnView<TYPE_STRING>::create(block.get_by_position(arguments[1]).column);
+ const auto& left_col = block.get_by_position(arguments[0]).column;
+ const auto& right_col = block.get_by_position(arguments[1]).column;
+ const auto& left_type = block.get_data_type(arguments[0]);
+ const auto& right_type = block.get_data_type(arguments[1]);
+ RETURN_IF_ERROR(validate_geography_semantics(left_type, NAME));
+ RETURN_IF_ERROR(validate_geography_semantics(right_type, NAME));
- const auto size = left_col.size();
+ const auto size = left_col->size();
auto res = ColumnUInt8::create(size, 0);
auto null_map = ColumnUInt8::create(size, 0);
auto& null_map_data = null_map->get_data();
- for (int row = 0; row < size; ++row) {
- auto lhs_value = left_col.value_at(row);
- auto rhs_value = right_col.value_at(row);
-
- std::unique_ptr<GeoShape> shape1(
- GeoShape::from_encoded(lhs_value.data, lhs_value.size));
- std::unique_ptr<GeoShape> shape2(
- GeoShape::from_encoded(rhs_value.data, rhs_value.size));
+ for (size_t row = 0; row < size; ++row) {
+ // Propagate NULL before using nested placeholders; a NULL
geometry has an
+ // empty payload, not a WKB shape.
+ if (any_geo_argument_is_null(block, arguments, row)) {
+ null_map_data[row] = 1;
+ continue;
+ }
+ std::unique_ptr<GeoShape> shape1;
+ RETURN_IF_ERROR(
+ decode_calculation_shape(left_col->get_data_at(row),
left_type, &shape1, NAME));
Review Comment:
[P2] Use spherical predicates throughout typed Geography dispatch
This newly accepts `GEOGRAPHY(OGC:CRS84, spherical)`, but
`GeoLine::intersects` projects latitude/longitude onto a Cartesian segment
using only vertices 0/1. Accepted `LINESTRING(170 0,-170 0)` and `POINT(180 0)`
give distance 10 and `ST_Intersects=false`/`ST_Disjoint=true`, although the
point lies on the spherical date-line edge. Later line segments are also
ignored. The parallel polygon/point helper uses planar ray crossing: a polygon
spanning longitudes +/-170 and latitudes +/-10 falsely intersects `POINT(0 0)`
instead of respecting its small spherical interior at the date line. The
[Geography
contract](https://iceberg.apache.org/spec/#edge-interpolation-algorithm)
requires geodesic edges. Use complete spherical operations across admitted
shape pairs, or explicitly reject unsupported pairs; fixing only the line
helper leaves the polygon path wrong.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/StIntersects.java:
##########
@@ -70,6 +70,12 @@ public List<FunctionSignature> getSignatures() {
return SIGNATURES;
}
+ @Override
+ public FunctionSignature searchSignature(List<FunctionSignature>
signatures) {
+ FunctionSignature signature =
SpatialFunctionSignature.binary(child(0), child(1), BooleanType.INSTANCE);
Review Comment:
[P2] Keep an open Geography line open in polygon intersection
The new typed signature admits a default Geography polygon covering the
square `[-0.5,0.5]^2` and the disjoint open `LINESTRING(-2 0,-2 2,2 2,2 0)`.
`GeoPolygon::intersects` gets an empty S2 polyline intersection, then its
fallback loops through all vertices using `(i+1)%num_vertices`. That invents a
last-to-first edge `(2 0)->(-2 0)` across the square and returns true.
Reversing operands uses `S2Polygon::Intersects` directly and returns false.
This is separate from the planar-helper problem: the nonexistent closing edge
is wrong even with a spherical edge test. Iterate only real consecutive line
edges, reserve wraparound for polygon rings, and cover both operand orders.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/StDisjoint.java:
##########
@@ -70,6 +70,12 @@ public List<FunctionSignature> getSignatures() {
return SIGNATURES;
}
+ @Override
+ public FunctionSignature searchSignature(List<FunctionSignature>
signatures) {
+ FunctionSignature signature =
SpatialFunctionSignature.binary(child(0), child(1), BooleanType.INSTANCE);
Review Comment:
[P2] Canonicalize equivalent Geography points for relation comparisons
The new typed overload accepts default Geography `POINT(180 0)` and
`POINT(-180 0)`, which denote the same date-line point. Both pass the inclusive
coordinate bounds, but `to_s2point` converts them without canonicalization and
`GeoPoint::intersects` compares their S2 vectors exactly. The pinned [S2
ToPoint
formula](https://github.com/google/s2geometry/blob/v0.10.0/src/s2/s2latlng.cc)
produces opposite nonzero sine roundoff at +pi/-pi, so `ST_Intersects` returns
false and `ST_Disjoint` true for these equivalent locations. This is separate
from the line/polygon planar-helper issue. Preserve raw WKB, but canonicalize
equivalent longitude/pole representations for calculations or compare positions
under spherical semantics, and cover date-line/pole aliases.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/StAsBinary.java:
##########
@@ -68,6 +68,12 @@ public List<FunctionSignature> getSignatures() {
return SIGNATURES;
}
+ @Override
+ public FunctionSignature searchSignature(List<FunctionSignature>
signatures) {
+ FunctionSignature signature = SpatialFunctionSignature.unary(child(0),
VarcharType.SYSTEM_DEFAULT);
Review Comment:
[P2] Return a binary carrier for typed ST_AsBinary
This typed overload returns VARCHAR, while its BE branch copies raw WKB into
`ColumnString`. The recommended Flight projection `SELECT
ST_AsBinary(ST_GeometryFromWKBTyped('0101000000000000000000F03F0000000000000040'))`
therefore maps to Arrow UTF8 and appends bytes `F0 3F` unchanged. The
inspected builder path does not validate UTF8, and [PyArrow
StringScalar](https://github.com/apache/arrow/blob/apache-arrow-24.0.0/python/pyarrow/scalar.pxi)
decodes those bytes in `as_py`/`to_pylist`, raising `UnicodeDecodeError`
instead of returning WKB bytes. Spatial MySQL metadata changes cannot fix this
accessor's VARCHAR result. Give the typed overload a binary return type with
matching BE column/result handling, and cover its Arrow schema directly;
`hex(ST_AsBinary(...))` masks the defect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
