github-actions[bot] commented on code in PR #69032:
URL: https://github.com/apache/doris/pull/69032#discussion_r4335064374
##########
be/src/exprs/function/geo/functions_geo.cpp:
##########
@@ -32,18 +45,308 @@
#include "core/column/column_array.h"
#include "core/column/column_execute_util.h"
#include "core/column/column_nullable.h"
+#include "core/column/column_varbinary.h"
#include "core/data_type/data_type_array.h"
#include "core/data_type/data_type_nullable.h"
#include "core/data_type/data_type_number.h"
+#include "core/data_type/data_type_spatial.h"
#include "core/data_type/data_type_string.h"
+#include "core/data_type/data_type_varbinary.h"
#include "core/data_type/define_primitive_type.h"
#include "core/string_ref.h"
#include "exprs/function/geo/geo_common.h"
#include "exprs/function/geo/geo_types.h"
+#include "exprs/function/geo/wkb_parse.h"
#include "exprs/function/simple_function_factory.h"
+#include "exprs/function/string_hex_util.h"
namespace doris {
+static bool is_spatial_type(const DataTypePtr& type) {
+ const auto primitive_type = remove_nullable(type)->get_primitive_type();
+ return primitive_type == TYPE_GEOMETRY || primitive_type == TYPE_GEOGRAPHY;
+}
+
+static Status validate_geography_semantics(const DataTypePtr& type, const
char* function_name) {
+ const auto& nested_type = remove_nullable(type);
+ if (!is_spatial_type(nested_type)) {
+ return Status::OK();
+ }
+
+ const auto* spatial_type = dynamic_cast<const
DataTypeSpatial*>(nested_type.get());
+ DCHECK(spatial_type != nullptr);
+ if (spatial_type->get_primitive_type() == TYPE_GEOGRAPHY &&
+ spatial_type->crs() == "OGC:CRS84" && spatial_type->algorithm() ==
"spherical") {
+ return Status::OK();
+ }
+ return Status::NotSupported(
+ "Function {} requires GEOGRAPHY(OGC:CRS84, spherical) for spatial
inputs",
+ function_name);
+}
+
+static std::unique_ptr<GeoShape> decode_geo_shape(StringRef value, const
DataTypePtr& type) {
+ if (!is_spatial_type(type)) {
+ return GeoShape::from_encoded(value.data, value.size);
+ }
+
+ GeoParseStatus status;
+ auto shape = GeoShape::from_wkb_bytes(value.data, value.size, status);
+ return status == GeoParseStatus::GEO_PARSE_OK ? std::move(shape) : nullptr;
+}
+
+static bool any_geo_argument_is_null(const Block& block, const ColumnNumbers&
arguments,
+ size_t row) {
+ return std::ranges::any_of(arguments, [&](auto argument) {
+ return block.get_by_position(argument).column->is_null_at(row);
+ });
+}
+
+static Status decode_calculation_shape(StringRef value, const DataTypePtr&
type,
+ std::unique_ptr<GeoShape>* shape,
+ const char* function_name) {
+ *shape = decode_geo_shape(value, type);
+ // POLYGON EMPTY decodes successfully, but distance helpers retain
infinity/max sentinels.
+ // Reject it for typed calculations just like empty points/lines, leaving
raw WKB untouched.
+ if (is_spatial_type(type) &&
+ (*shape == nullptr ||
+ ((*shape)->type() == GeoShapeType::GEO_SHAPE_POLYGON &&
+ assert_cast<const GeoPolygon&>(**shape).polygon()->num_loops() ==
0))) {
+ return Status::NotSupported("Function {} does not support this
Geography WKB shape",
+ function_name);
+ }
+ return Status::OK();
+}
+
+// Calculations use canonical positions; raw spatial accessors still return
the original WKB.
+static S2Point canonical_geography_point(const S2Point& point) {
+ const auto coordinate = S2LatLng(point);
+ const double latitude = coordinate.lat().radians();
+ // All longitudes at a pole and +/-180 at the date line denote the same
position.
+ if (std::abs(latitude) == M_PI_2) {
+ return {0, 0, latitude > 0 ? 1.0 : -1.0};
+ }
+ if (coordinate.lng().radians() == M_PI) {
+ // Flip only sin(longitude): rebuilding latitude can change its last
bit.
+ return {point.x(), -point.y(), point.z()};
+ }
+ return point;
+}
+
+static void geography_shape_index(const GeoShape& shape, MutableS2ShapeIndex*
index) {
+ switch (shape.type()) {
+ case GeoShapeType::GEO_SHAPE_LINE_STRING: {
+ const auto* line = assert_cast<const GeoLine&>(shape).polyline();
+ std::vector<S2Point> vertices;
+ for (int i = 0; i < line->num_vertices(); ++i) {
+ vertices.push_back(canonical_geography_point(line->vertex(i)));
+ }
+ index->Add(std::make_unique<S2LaxPolylineShape>(vertices));
+ break;
+ }
+ case GeoShapeType::GEO_SHAPE_POLYGON: {
+ const auto* polygon = assert_cast<const GeoPolygon&>(shape).polygon();
+ std::vector<S2LaxPolygonShape::Loop> loops;
+ for (int i = 0; i < polygon->num_loops(); ++i) {
+ auto& vertices = loops.emplace_back();
+ const auto* loop = polygon->loop(i);
+ for (int j = 0; j < loop->num_vertices(); ++j) {
+
vertices.push_back(canonical_geography_point(loop->oriented_vertex(j)));
+ }
+ }
+ index->Add(std::make_unique<S2LaxPolygonShape>(loops));
+ break;
+ }
+ default:
+ // decode_calculation_shape admits only nonempty
Point/LineString/Polygon WKB.
+ throw Exception(ErrorCode::INTERNAL_ERROR, "Unexpected Geography
calculation shape");
+ }
+}
+
+static bool geography_point_relation(std::string_view name, const GeoShape&
lhs,
+ const GeoShape& rhs) {
+ const auto& point_shape = lhs.type() == GeoShapeType::GEO_SHAPE_POINT ?
lhs : rhs;
+ const auto& other = lhs.type() == GeoShapeType::GEO_SHAPE_POINT ? rhs :
lhs;
+ const auto point =
+ canonical_geography_point(*assert_cast<const
GeoPoint&>(point_shape).point());
+ bool interior = false;
+ bool boundary = false;
+ // S2LatLng::ToPoint has up to 1.5*epsilon error, plus degrees-to-radians
+ // rounding. Allow only conversion-scale error for the point and edge
endpoints:
+ // POINT(10 45.5) is on LINESTRING(10 45,10 46), but their rounded vectors
+ // are not exactly coplanar. This is about 1e-8 metres, not a user snap
radius.
+ const auto conversion_error =
+ S1ChordAngle(S1Angle::Radians(8 *
std::numeric_limits<double>::epsilon()));
+ if (other.type() == GeoShapeType::GEO_SHAPE_POINT) {
+ interior = point == canonical_geography_point(*assert_cast<const
GeoPoint&>(other).point());
+ } else if (other.type() == GeoShapeType::GEO_SHAPE_LINE_STRING) {
+ const auto* line = assert_cast<const GeoLine&>(other).polyline();
+ bool member = false;
+ // S2 Boolean operations symbolically move points off edge interiors.
Use its
+ // unperturbed distance predicate on every real geodesic edge instead.
+ for (int i = 0; i + 1 < line->num_vertices(); ++i) {
+ member = member ||
+ s2pred::CompareEdgeDistance(point,
canonical_geography_point(line->vertex(i)),
+
canonical_geography_point(line->vertex(i + 1)),
+ conversion_error) <= 0;
+ }
+ const auto first = canonical_geography_point(line->vertex(0));
+ const auto last =
canonical_geography_point(line->vertex(line->num_vertices() - 1));
+ boundary = first != last && (point == first || point == last);
+ interior = member && !boundary;
+ } else {
+ DCHECK(other.type() == GeoShapeType::GEO_SHAPE_POLYGON);
+ const auto* polygon = assert_cast<const GeoPolygon&>(other).polygon();
+ // Polygon rings have closing edges; open polylines above do not.
+ for (int i = 0; i < polygon->num_loops(); ++i) {
+ const auto* loop = polygon->loop(i);
+ for (int j = 0; j < loop->num_vertices(); ++j) {
+ boundary = boundary || s2pred::CompareEdgeDistance(
+ point,
canonical_geography_point(loop->vertex(j)),
+
canonical_geography_point(loop->vertex(j + 1)),
+ conversion_error) <= 0;
+ }
+ }
+ interior = !boundary && polygon->Contains(point);
+ }
+ if (name == "st_intersects") {
+ return interior || boundary;
+ }
+ if (name == "st_disjoint") {
+ return !interior && !boundary;
+ }
+ if (name == "st_touches") {
+ return boundary;
+ }
+ return interior;
+}
+
+static Status geography_relation(std::string_view name, const GeoShape& lhs,
const GeoShape& rhs,
+ bool* result) {
+ const auto& container = name == "st_within" ? rhs : lhs;
+ const auto& value = name == "st_within" ? lhs : rhs;
+ // Legacy base contains() returns false even for p within itself. Complex
containment
+ // and touches pairs need interior/boundary semantics that are not
implemented here.
+ if (((name == "st_contains" || name == "st_within") &&
+ value.type() != GeoShapeType::GEO_SHAPE_POINT) ||
+ (name == "st_touches" && lhs.type() != GeoShapeType::GEO_SHAPE_POINT &&
+ rhs.type() != GeoShapeType::GEO_SHAPE_POINT)) {
+ return Status::NotSupported("Function {} does not support this
Geography shape pair", name);
+ }
+ if (lhs.type() == GeoShapeType::GEO_SHAPE_POINT ||
+ rhs.type() == GeoShapeType::GEO_SHAPE_POINT) {
+ *result = geography_point_relation(name, lhs, rhs);
+ return Status::OK();
+ }
+ MutableS2ShapeIndex left;
+ MutableS2ShapeIndex right;
+ geography_shape_index(container, &left);
+ geography_shape_index(value, &right);
+ S2BooleanOperation::Options options;
+ options.set_polygon_model(S2BooleanOperation::PolygonModel::CLOSED);
+ options.set_polyline_model(S2BooleanOperation::PolylineModel::CLOSED);
+ options.set_polyline_loops_have_boundaries(false);
+ const bool intersects = S2BooleanOperation::Intersects(left, right,
options);
Review Comment:
[P2] Handle conversion roundoff in nonpoint Geography intersections
The conversion-error allowance above only runs when an operand is a Point.
Default Geography `LINESTRING(10 45,10 46)` and `LINESTRING(10 45.3,10 45.9)`
instead reach this branch. They overlap on the same meridian, but converting
their endpoints independently to S2 vectors puts both inner endpoints strictly
on the same side of the outer edge's great-circle plane (exact determinants
approximately -1.987e-19 and -2.592e-19). With [S2's default exact-input
predicates](https://github.com/google/s2geometry/blob/v0.10.0/src/s2/s2boolean_operation.h#L239-L259),
the represented arcs do not intersect: `ST_Intersects` yields false and
`ST_Disjoint` true in both operand orders. This is a distinct nonpoint path
from the repaired point-on-edge thread; it bypasses `geography_point_relation`.
Apply a consistent conversion-scale error policy to this path too, and cover
overlapping meridian lines with different endpoints plus nearby disjoint
controls.
##########
be/src/core/data_type/define_primitive_type.h:
##########
@@ -75,7 +75,9 @@ enum PrimitiveType : PrimitiveNative {
TYPE_VARBINARY, /* 41, varbinary */
TYPE_TIMESTAMPTZ, /* 42, timestamptz */
TYPE_TIMESTAMP_NS, /* 43, signed Int64 epoch nanoseconds
*/
- TYPE_UUID /* 44, uuid */
+ TYPE_UUID, /* 44, uuid */
+ TYPE_GEOMETRY, /* 45, Iceberg geometry */
Review Comment:
[P2] Guard spatial arguments to LEAST and GREATEST
The new types are accepted by `GreatestLeast.customSignature` when both
arguments have the same spatial identity, but their BE consumer still has no
spatial implementation. For distinct imported Iceberg columns `g1` and `g2` of
the same GEOMETRY type, the admitted plan is:
```text
Project[ST_AsText(greatest(g1, g2))]
IcebergScan[g1:GEOMETRY, g2:GEOMETRY]
```
`FunctionMultiSameArgs` reaches `CompareMultiImpl::execute` in
`least_greast.cpp`: `ColumnSpatial` takes the non-STRING branch, then
`dispatch_switch_scalar` returns false and throws `INTERNAL_ERROR: not support
type Geometry`. `LEAST` and GEOGRAPHY fail likewise. These scalar functions are
separate from the already-repaired aggregate factories. Add a spatial column
comparison/materialization path or reject these inputs during FE analysis, with
coverage using two distinct spatial slots and nullable inputs.
--
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]
