OIiveirra commented on code in PR #69032:
URL: https://github.com/apache/doris/pull/69032#discussion_r4291163121
##########
be/src/exprs/function/geo/functions_geo.cpp:
##########
@@ -845,122 +1363,46 @@ struct StDistance {
auto null_map = ColumnUInt8::create(size, 0);
auto& null_map_data = null_map->get_data();
- if (left_const) {
- const_vector(left_column, right_column, res, null_map_data, size);
- } else if (right_const) {
- vector_const(left_column, right_column, res, null_map_data, size);
- } else {
- vector_vector(left_column, right_column, res, null_map_data, size);
- }
- block.replace_by_position(result,
- ColumnNullable::create(std::move(res),
std::move(null_map)));
- return Status::OK();
- }
-
-private:
- static bool decode_shape(const StringRef& value,
std::unique_ptr<GeoShape>& shape) {
- shape = GeoShape::from_encoded(value.data, value.size);
- return static_cast<bool>(shape);
- }
-
- static void loop_do(StringRef& lhs_value, StringRef& rhs_value,
- std::vector<std::unique_ptr<GeoShape>>& shapes,
- ColumnFloat64::MutablePtr& res, NullMap& null_map, int
row) {
- StringRef* strs[2] = {&lhs_value, &rhs_value};
- for (int i = 0; i < 2; ++i) {
- if (!decode_shape(*strs[i], shapes[i])) {
- null_map[row] = 1;
- res->insert_default();
- return;
- }
- }
- double distance = shapes[0]->Distance(shapes[1].get());
- if (UNLIKELY(distance < 0)) {
- null_map[row] = 1;
- res->insert_default();
- return;
- }
- res->insert_value(distance);
- }
-
- static void const_vector(const ColumnPtr& left_column, const ColumnPtr&
right_column,
- ColumnFloat64::MutablePtr& res, NullMap&
null_map, const size_t size) {
- const auto* left_string = assert_cast<const
ColumnString*>(left_column.get());
- const auto* right_string = assert_cast<const
ColumnString*>(right_column.get());
-
- auto lhs_value = left_string->get_data_at(0);
- std::unique_ptr<GeoShape> lhs_shape;
- if (!decode_shape(lhs_value, lhs_shape)) {
- for (int row = 0; row < size; ++row) {
- null_map[row] = 1;
- res->insert_default();
- }
- return;
- }
-
- std::unique_ptr<GeoShape> rhs_shape;
- for (int row = 0; row < size; ++row) {
- auto rhs_value = right_string->get_data_at(row);
- if (!decode_shape(rhs_value, rhs_shape)) {
- null_map[row] = 1;
+ std::unique_ptr<GeoShape> left_shape;
+ std::unique_ptr<GeoShape> right_shape;
+ bool left_decoded = false;
+ bool right_decoded = false;
+ 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;
res->insert_default();
continue;
}
- double distance = lhs_shape->Distance(rhs_shape.get());
- if (UNLIKELY(distance < 0)) {
- null_map[row] = 1;
- res->insert_default();
- continue;
+ if (!left_const || !left_decoded) {
+ RETURN_IF_ERROR(
+
decode_calculation_shape(left_column->get_data_at(left_const ? 0 : row),
+ left_type, &left_shape,
NAME));
+ left_decoded = true;
}
- res->insert_value(distance);
- }
- }
-
- static void vector_const(const ColumnPtr& left_column, const ColumnPtr&
right_column,
- ColumnFloat64::MutablePtr& res, NullMap&
null_map, const size_t size) {
- const auto* left_string = assert_cast<const
ColumnString*>(left_column.get());
- const auto* right_string = assert_cast<const
ColumnString*>(right_column.get());
-
- auto rhs_value = right_string->get_data_at(0);
- std::unique_ptr<GeoShape> rhs_shape;
- if (!decode_shape(rhs_value, rhs_shape)) {
- for (int row = 0; row < size; ++row) {
- null_map[row] = 1;
- res->insert_default();
+ if (!right_const || !right_decoded) {
+ RETURN_IF_ERROR(
+
decode_calculation_shape(right_column->get_data_at(right_const ? 0 : row),
+ right_type, &right_shape,
NAME));
+ right_decoded = true;
}
- return;
- }
-
- std::unique_ptr<GeoShape> lhs_shape;
- for (int row = 0; row < size; ++row) {
- auto lhs_value = left_string->get_data_at(row);
- if (!decode_shape(lhs_value, lhs_shape)) {
- null_map[row] = 1;
+ if (!left_shape || !right_shape) {
+ null_map_data[row] = 1;
res->insert_default();
continue;
}
- double distance = lhs_shape->Distance(rhs_shape.get());
+ const double distance = left_shape->Distance(right_shape.get());
Review Comment:
Fixed in
[b2106a70a1fd](https://github.com/apache/doris/commit/b2106a70a1fd5564f70aa9a0445392040abe8e00).
Typed calculation decoding now rejects zero-loop Geography polygons with
NotSupported instead of exposing the distance helpers' infinity/max sentinels.
Added distance tests for EMPTY/Point, Point/EMPTY and EMPTY/EMPTY; raw EMPTY
construction and WKB accessors retain their existing path. The targeted BE ASAN
suite passed.
##########
be/src/exprs/function/geo/functions_geo.cpp:
##########
@@ -32,18 +44,288 @@
#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 == 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);
+ if (is_spatial_type(type) && *shape == nullptr) {
+ 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 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 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() == GEO_SHAPE_POINT ? lhs : rhs;
+ const auto& other = lhs.type() == GEO_SHAPE_POINT ? rhs : lhs;
+ const auto point =
+ canonical_geography_point(*assert_cast<const
GeoPoint&>(point_shape).point());
+ bool interior = false;
+ bool boundary = false;
+ if (other.type() == GEO_SHAPE_POINT) {
+ interior = point == canonical_geography_point(*assert_cast<const
GeoPoint&>(other).point());
+ } else if (other.type() == 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)),
+ S1ChordAngle::Zero()) <= 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_EQ(other.type(), 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)),
+ S1ChordAngle::Zero()) <= 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() !=
GEO_SHAPE_POINT) ||
+ (name == "st_touches" && lhs.type() != GEO_SHAPE_POINT && rhs.type()
!= GEO_SHAPE_POINT)) {
+ return Status::NotSupported("Function {} does not support this
Geography shape pair", name);
+ }
+ if (lhs.type() == GEO_SHAPE_POINT || rhs.type() == 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);
+ DCHECK(name == "st_intersects" || name == "st_disjoint");
+ *result = name == "st_intersects" ? intersects : !intersects;
+ return Status::OK();
+}
+
+static S2LatLng geography_point_coordinate(StringRef wkb) {
+ double longitude;
+ double latitude;
+ const auto status = WkbParse::point_coordinates(wkb.data, wkb.size,
&longitude, &latitude);
+ DCHECK_EQ(status, GEO_PARSE_OK);
+ // Use original coordinates: reconstructing a shared longitude from two S2
vectors
+ // can turn due north into a tiny negative bearing that wraps to almost
2*pi.
+ if (std::abs(latitude) == 90) {
+ longitude = 0;
+ } else if (longitude == 180) {
+ longitude = -180;
+ }
+ return S2LatLng::FromDegrees(latitude, longitude);
+}
+
+static bool geography_bearing(StringRef from, StringRef to, double* bearing) {
+ const auto start = geography_point_coordinate(from);
+ const auto end = geography_point_coordinate(to);
+ // Coincident or antipodal positions have no unique initial bearing.
+ if (start == end || start.GetDistance(end).radians() == M_PI) {
Review Comment:
Fixed in
[b2106a70a1fd](https://github.com/apache/doris/commit/b2106a70a1fd5564f70aa9a0445392040abe8e00).
Antipodality now uses the original canonical coordinate relationship
(opposite latitude plus a 180-degree longitude difference, with pole
equivalence), without comparing haversine distance to pi or adding a distance
tolerance. Tests cover exact antipodes at several latitudes and poles, both
azimuth directions and both angle legs, plus nearby non-antipodes including
longitude 179.999999. The targeted BE ASAN suite passed.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/connector/converter/ConnectorSpatialWriteValidator.java:
##########
@@ -0,0 +1,119 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.datasource.connector.converter;
+
+import org.apache.doris.catalog.Column;
+import org.apache.doris.catalog.ScalarType;
+import org.apache.doris.catalog.Type;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.nereids.trees.expressions.Alias;
+import org.apache.doris.nereids.trees.expressions.CaseWhen;
+import org.apache.doris.nereids.trees.expressions.Cast;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.NamedExpression;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.Coalesce;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.If;
+import org.apache.doris.nereids.types.DataType;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * Preserves the kind and parameters of spatial connector columns before sink
coercion.
+ * A spatial value is an opaque WKB payload at execution time, so a generic
cast cannot
+ * safely convert between CRS, edge algorithms, or Geometry and Geography.
+ */
+public final class ConnectorSpatialWriteValidator {
+ private ConnectorSpatialWriteValidator() {
+ }
+
+ public static void validate(List<Column> targetColumns,
+ List<? extends NamedExpression> sourceColumns) {
+ if (targetColumns.size() != sourceColumns.size()) {
+ throw new AnalysisException("Spatial connector write target and
source columns are not aligned");
+ }
+ for (int i = 0; i < targetColumns.size(); i++) {
+ validate(targetColumns.get(i), sourceColumns.get(i));
+ }
+ }
+
+ public static void validate(Column targetColumn, Expression source) {
+ Type targetType = targetColumn.getType();
+ if (targetType instanceof ScalarType && ((ScalarType)
targetType).isSpatialType()) {
+ validateExpression(source, (ScalarType) targetType,
targetColumn.getName());
+ } else if (targetType.typeContainsSpatialType()) {
+ throw new AnalysisException("Nested spatial columns are not
supported for connector writes: "
+ + targetColumn.getName());
+ }
+ }
+
+ private static void validateExpression(Expression expression, ScalarType
targetType, String columnName) {
+ if (expression instanceof Alias) {
+ validateExpression(expression.child(0), targetType, columnName);
+ return;
+ }
+ DataType targetDataType = DataType.fromCatalogType(targetType);
+ if (expression instanceof If &&
expression.getDataType().equals(targetDataType)) {
+ If branch = (If) expression;
+ validateExpression(branch.getTrueValue(), targetType, columnName);
+ validateExpression(branch.getFalseValue(), targetType, columnName);
+ return;
+ }
+ if (expression instanceof Cast &&
expression.getDataType().equals(targetDataType)) {
+ validateExpression(expression.child(0), targetType, columnName);
+ return;
+ }
+ if (expression instanceof CaseWhen &&
expression.getDataType().equals(targetDataType)) {
+ CaseWhen caseWhen = (CaseWhen) expression;
+ caseWhen.getWhenClauses().forEach(clause ->
+ validateExpression(clause.getResult(), targetType,
columnName));
+ caseWhen.getDefaultValue().ifPresent(value ->
validateExpression(value, targetType, columnName));
+ return;
+ }
+ if (expression instanceof Coalesce &&
expression.getDataType().equals(targetDataType)) {
+ expression.children().forEach(child -> validateExpression(child,
targetType, columnName));
+ return;
+ }
+ validateType(expression.getDataType().toCatalogDataType(), targetType,
columnName);
Review Comment:
Fixed in
[b2106a70a1fd](https://github.com/apache/doris/commit/b2106a70a1fd5564f70aa9a0445392040abe8e00).
After unwrapping Alias, the validator now accepts an actual NullLiteral
before checking spatial identity. The new integration test goes through
InsertUtils.normalizePlan with CAST(NULL AS INT), a different-CRS Geometry and
Geography, and verifies a target-typed NULL for a nullable Geometry sink.
Existing non-NULL type/CRS/algorithm rejection tests also passed.
--
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]
