github-actions[bot] commented on code in PR #69032:
URL: https://github.com/apache/doris/pull/69032#discussion_r4320808223
##########
be/src/exprs/function/geo/functions_geo.cpp:
##########
@@ -32,18 +44,302 @@
#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;
+ 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)),
+ S1ChordAngle::Zero()) <= 0;
Review Comment:
[P2] Account for conversion roundoff in point-on-edge tests
For default Geography WKB representing `LINESTRING(10 45,10 46)` and
`POINT(10 45.5)`, the point lies inside the meridian segment, so
Intersects/Contains/Within should be true and Disjoint false. `to_s2point`
rounds each coordinate conversion independently; their resulting vectors have a
nonzero exact scalar triple product (about -1.41467e-19). [Pinned S2's exact
distance
fallback](https://github.com/google/s2geometry/blob/v0.10.0/src/s2/s2predicates.cc#L660-L713)
therefore returns +1 against `Zero()`, making this path produce the opposite
booleans. This is a numerical defect in the replacement spherical predicate,
distinct from the earlier planar/first-edge dispatch issue. Account for
coordinate-conversion error when testing spherical membership, apply the same
rule to the polygon boundary check below, and cover nonzero-longitude meridian
points in both operand orders; the equatorial cases do not exercise this.
##########
be/src/core/data_type/data_type_spatial.cpp:
##########
@@ -0,0 +1,219 @@
+// 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.
+
+#include "core/data_type/data_type_spatial.h"
+
+#include <algorithm>
+#include <cctype>
+#include <cstring>
+#include <string_view>
+
+#include "agent/be_exec_version_manager.h"
+#include "common/exception.h"
+#include "core/assert_cast.h"
+#include "core/column/column_const.h"
+#include "core/data_type/data_type.h"
+#include "core/data_type_serde/data_type_varbinary_serde.h"
+#include "core/field.h"
+#include "core/string_view.h"
+#include "exprs/function/geo/wkb_parse.h"
+#include "gen_cpp/data.pb.h"
+
+namespace doris {
+
+namespace {
+
+std::string normalize_spatial_crs(std::string crs) {
+ // Imported ogc:crs84 has default semantics; leave custom CRS/property
references unchanged.
+ constexpr std::string_view default_crs = "OGC:CRS84";
+ if (std::ranges::equal(crs, default_crs, [](unsigned char left, unsigned
char right) {
+ return std::tolower(left) == std::tolower(right);
+ })) {
+ return std::string(default_crs);
+ }
+ return crs;
+}
+
+std::string normalize_spatial_algorithm(PrimitiveType type, std::string
algorithm) {
+ if (type == TYPE_GEOGRAPHY && algorithm.empty()) {
+ return "spherical";
+ }
+ std::ranges::transform(algorithm, algorithm.begin(),
+ [](unsigned char c) { return
static_cast<char>(std::tolower(c)); });
+ return algorithm;
+}
+
+class SpatialParquetSerDe final : public DataTypeVarbinarySerDe {
+public:
+ SpatialParquetSerDe(int nesting_level, bool geographic)
+ : DataTypeVarbinarySerDe(nesting_level), _geographic(geographic) {}
+
+ Status read_column_from_parquet(IColumn& column, ParquetDecodeSource&
source,
+ const ParquetDecodeContext& context,
size_t num_values,
+ ParquetMaterializationState& state) const
override {
+ const auto start = column.size();
+
RETURN_IF_ERROR(DataTypeVarbinarySerDe::read_column_from_parquet(column,
source, context,
+
num_values, state));
+ // Validate materialized values, not unselected dictionary entries.
NULL runs
+ // are appended by the reader outside the leaf SerDe.
+ for (size_t row = start; row < column.size(); ++row) {
+ const auto value = column.get_data_at(row);
+ RETURN_IF_ERROR(
+ WkbParse::validate_spatial_wkb(value.data, value.size,
_geographic, row));
+ }
+ return Status::OK();
+ }
+
+private:
+ const bool _geographic;
+};
+
+} // namespace
+
+DataTypeSpatial::DataTypeSpatial(PrimitiveType primitive_type, std::string crs,
+ std::string algorithm)
+ : _primitive_type(primitive_type),
+ _crs(normalize_spatial_crs(std::move(crs))),
+ _algorithm(normalize_spatial_algorithm(primitive_type,
std::move(algorithm))) {
+ DCHECK(primitive_type == TYPE_GEOMETRY || primitive_type ==
TYPE_GEOGRAPHY);
+ DCHECK(!_crs.empty());
+ DCHECK(primitive_type == TYPE_GEOMETRY || !_algorithm.empty());
+ DCHECK(primitive_type == TYPE_GEOGRAPHY || _algorithm.empty());
+}
+
+void DataTypeSpatial::to_pb_column_meta(PColumnMeta* col_meta) const {
+ IDataType::to_pb_column_meta(col_meta);
+ col_meta->set_spatial_crs(_crs);
+ if (_primitive_type == TYPE_GEOGRAPHY) {
+ col_meta->set_spatial_algorithm(_algorithm);
+ }
+}
+
+DataTypeSerDeSPtr DataTypeSpatial::get_serde(int nesting_level) const {
+ return std::make_shared<SpatialParquetSerDe>(nesting_level,
requires_geographic_coordinates());
Review Comment:
[P2] Preserve nested spatial values across aggregate-state serialization
The new spatial SerDe inherits raw WKB JSON output. For admitted
`COLLECT_LIST(shapes)` over imported `ARRAY<GEOMETRY>` or `ARRAY<GEOGRAPHY>`,
`agg_phase=2` sends the local state's cells through
`AggregateFunctionCollectListData::write/read` and `DataTypeArraySerDe`'s text
parser. A one-element array containing valid `POINT(14 2)` emits raw WKB
`01010000000000000000002c400000000000000040` between brackets; its `0x2c` byte
is parsed as a comma, producing two nonnull fragments of 11 and 9 bytes. The
inherited spatial JSON reader accepts both, so `size(collect_list(shapes)[1])`
becomes 2 instead of 1, and spatial accessors later reject the damaged values.
Provide a reversible nested spatial encoding/decoding or a binary
aggregate-state path; otherwise reject the affected aggregate input explicitly.
Cover actual partial-state serialization/merge with delimiter-bearing WKB for
both spatial kinds.
--
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]
