github-actions[bot] commented on code in PR #69032: URL: https://github.com/apache/doris/pull/69032#discussion_r4380521044
########## fe/fe-core/src/main/java/org/apache/doris/datasource/connector/converter/ConnectorSpatialWriteNormalizer.java: ########## @@ -0,0 +1,267 @@ +// 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.nereids.CTEContext; +import org.apache.doris.nereids.trees.expressions.Alias; +import org.apache.doris.nereids.trees.expressions.CTEId; +import org.apache.doris.nereids.trees.expressions.Cast; +import org.apache.doris.nereids.trees.expressions.ExprId; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.NamedExpression; +import org.apache.doris.nereids.trees.expressions.Slot; +import org.apache.doris.nereids.trees.expressions.functions.scalar.Array; +import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateMap; +import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateNamedStruct; +import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateStruct; +import org.apache.doris.nereids.trees.expressions.literal.Literal; +import org.apache.doris.nereids.trees.expressions.literal.NullLiteral; +import org.apache.doris.nereids.trees.plans.Plan; +import org.apache.doris.nereids.trees.plans.logical.LogicalCTE; +import org.apache.doris.nereids.trees.plans.logical.LogicalCTEAnchor; +import org.apache.doris.nereids.trees.plans.logical.LogicalCTEConsumer; +import org.apache.doris.nereids.trees.plans.logical.LogicalCTEProducer; +import org.apache.doris.nereids.trees.plans.logical.LogicalFilter; +import org.apache.doris.nereids.trees.plans.logical.LogicalLimit; +import org.apache.doris.nereids.trees.plans.logical.LogicalOneRowRelation; +import org.apache.doris.nereids.trees.plans.logical.LogicalPlan; +import org.apache.doris.nereids.trees.plans.logical.LogicalProject; +import org.apache.doris.nereids.trees.plans.logical.LogicalSort; +import org.apache.doris.nereids.trees.plans.logical.LogicalSubQueryAlias; +import org.apache.doris.nereids.types.ArrayType; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.GeographyType; +import org.apache.doris.nereids.types.GeometryType; +import org.apache.doris.nereids.types.MapType; +import org.apache.doris.nereids.types.StructType; + +import com.google.common.collect.ImmutableList; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.stream.IntStream; + +/** Restores proven NULL producer values before spatial sink coercion. */ +public final class ConnectorSpatialWriteNormalizer { + private ConnectorSpatialWriteNormalizer() { + } + + public static LogicalPlan normalize(List<Column> targets, LogicalPlan source, CTEContext cteContext) { + if (targets.stream().noneMatch(column -> column.getType().typeContainsSpatialType())) { + return source; + } + Resolver resolver = new Resolver(source, cteContext); + List<NamedExpression> outputs = new ArrayList<>(source.getOutput()); + boolean changed = false; + for (int i = 0; i < targets.size(); i++) { + Column target = targets.get(i); + if (!target.getType().typeContainsSpatialType()) { + continue; + } + Slot slot = source.getOutput().get(i); + Expression value = resolver.resolve(slot, source); + ConnectorSpatialWriteValidator.validate(target, value); + // Never lift arbitrary producer expressions above their input scope, or + // replace outer-join NULL padding with a non-NULL container constant. + if (isLiteralConstruction(value)) { + Expression normalized = normalizeSpatialNulls(value, DataType.fromCatalogType(target.getType())); + if (!normalized.getDataType().equals(slot.getDataType())) { + outputs.set(i, new Alias(slot.getExprId(), normalized, slot.getName())); Review Comment: [P2] Give retagged output positions distinct ExprIds For a nullable Iceberg target `(n BIGINT, geom GEOMETRY)`, this valid insert selects the same slot twice: ```sql INSERT INTO t SELECT v, v FROM (SELECT CAST(NULL AS INT) AS v) q; ``` `bindProject` keeps both references as `v#x:INT`. Retagging only the second position here produces this reduced plan: ```text Project(v#x:INT, NULL:GEOMETRY AS v#x) Project(v#x:INT, v#x:INT) OneRow(NULL:INT AS v#x) ``` `BindSink.getColumnToOutput` then builds `CAST(v#x:INT AS BIGINT)` for `n`. The early `MergeProjectable` pass calls `generateReplaceMap`, which keys slots by ExprId and maps `#x` to the GEOMETRY NULL alias. It therefore rewrites the ordinary column to `CAST(NULL:GEOMETRY AS BIGINT)`. `FoldConstantRuleOnFE` preserves this unsupported pair and `CheckCast` rejects it, so the two-NULL insert fails. Give independently retagged output positions fresh identities and propagate those slots to sink binding. Cover repeated slots/star expansion with an ordinary sibling requiring coercion. This is a new ExprId collision introduced by normalization, distinct from the missing NULL provenance in 4378237190; this conclusion is based on the source rewrite path, not an executed query. ########## be/src/exprs/function/geo/functions_geo.cpp: ########## @@ -18,32 +18,1267 @@ #include "exprs/function/geo/functions_geo.h" #include <glog/logging.h> +#include <s2/mutable_s2shape_index.h> +#include <s2/s2boolean_operation.h> +#include <s2/s2builderutil_s2polyline_vector_layer.h> +#include <s2/s2closest_edge_query.h> +#include <s2/s2closest_point_query.h> +#include <s2/s2contains_point_query.h> +#include <s2/s2earth.h> +#include <s2/s2lax_polygon_shape.h> +#include <s2/s2loop.h> +#include <s2/s2loop_measures.h> #include <s2/s2point.h> +#include <s2/s2point_index.h> +#include <s2/s2point_vector_shape.h> +#include <s2/s2pointutil.h> +#include <s2/s2polygon.h> +#include <s2/s2polyline.h> +#include <s2/s2predicates.h> +#include <s2/s2shape_measures.h> #include <algorithm> +#include <array> +#include <bit> #include <boost/iterator/iterator_facade.hpp> +#include <boost/multiprecision/cpp_int.hpp> +#include <cmath> +#include <cstdint> +#include <limits> +#include <map> +#include <numbers> +#include <optional> +#include <set> #include <utility> +#include "common/cast_set.h" #include "common/compiler_util.h" #include "core/assert_cast.h" #include "core/block/block.h" #include "core/block/column_with_type_and_name.h" #include "core/column/column.h" #include "core/column/column_array.h" #include "core/column/column_execute_util.h" +#include "core/column/column_map.h" #include "core/column/column_nullable.h" +#include "core/column/column_struct.h" +#include "core/column/column_varbinary.h" +#include "core/custom_allocator.h" #include "core/data_type/data_type_array.h" +#include "core/data_type/data_type_map.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_struct.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 std::unique_ptr<GeoShape> decode_geography_calculation_shape(StringRef value); + +static Status decode_calculation_shape(StringRef value, const DataTypePtr& type, + std::unique_ptr<GeoShape>* shape, + const char* function_name) { + *shape = is_spatial_type(type) ? decode_geography_calculation_shape(value) + : decode_geo_shape(value, type); + // Typed admission rejects unsupported and EMPTY calculation shapes before distance + // helpers can expose infinity/max sentinels. Raw WKB storage/accessors remain unchanged. + 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; +} + +// Error-free TwoSum preserves tiny differences from a half-turn (e.g. -1e-15 + 180). +static std::pair<double, double> geography_degree_sum(double a, double b) { + const double high = a + b; + const double offset = high - a; + return {high, (a - (high - offset)) + (b - offset)}; +} + +// Reduce the exact degree sum before converting to radians. In particular, +// 180 + (-1e-300) must retain its small sine instead of becoming sin(pi). +static std::pair<long double, long double> geography_degree_sincos(double a, double b = 0, + bool half = false) { + const auto [high, low] = geography_degree_sum(a, b); + int quadrant; + const long double reduced = + std::remquo(static_cast<long double>(high), half ? 180.0L : 90.0L, &quadrant) + + static_cast<long double>(low); + const auto radians = reduced * (std::numbers::pi_v<long double> / 180) / (half ? 2 : 1); + const auto sine = std::sin(radians); + const auto cosine = std::cos(radians); + switch ((quadrant % 4 + 4) % 4) { + case 0: + return {sine, cosine}; + case 1: + return {cosine, -sine}; + case 2: + return {-sine, -cosine}; + default: + return {-cosine, sine}; + } +} + +// Unlike cross(ToPoint(a), ToPoint(b)), these product identities do not subtract +// rounded nearly opposite vectors. Longitude-midpoint coordinates give +// (-sin(dLon/2)*sin(latA+latB), cos(dLon/2)*sin(latA-latB), +// cos(latA)*cos(latB)*sin(dLon)) for the oriented normal. +struct GeographyRawArc { + S2Point normal; + S2Point midpoint; + + GeographyRawArc(const GeoCoordinate& a, const GeoCoordinate& b) { + // Wide exponents retain products of the smallest binary64 degree residuals. + static_assert(std::numeric_limits<long double>::digits >= 64 && + std::numeric_limits<long double>::min_exponent <= -4096); + const auto [sd, cd] = geography_degree_sincos(b.x, -a.x, true); + const auto [sm, cm] = geography_degree_sincos(a.x, b.x, true); + const auto rotate_unit = [&](long double x, long double y, long double z) { + const auto norm = std::sqrt(x * x + y * y + z * z); + DCHECK_GT(norm, 0); + return S2Point(static_cast<double>((cm * x - sm * y) / norm), + static_cast<double>((sm * x + cm * y) / norm), + static_cast<double>(z / norm)); + }; + normal = rotate_unit(-sd * geography_degree_sincos(a.y, b.y).first, + cd * geography_degree_sincos(a.y, -b.y).first, + geography_degree_sincos(a.y).second * + geography_degree_sincos(b.y).second * + geography_degree_sincos(b.x, -a.x).first); + const auto [sl, cl] = geography_degree_sincos(a.y, b.y, true); + const auto [st, ct] = geography_degree_sincos(b.y, -a.y, true); + midpoint = rotate_unit(cd * cl * ct, -sd * sl * st, sl * ct); + } +}; + +// Raw identity must survive ToPoint rounding: 58 and 58.00000000000001 degrees +// produce the same S2Point, but only one is an endpoint of the original edge. +struct GeographyVertex { + GeoCoordinate coordinate; + S2Point point; + + GeographyVertex() : GeographyVertex({.x = 0, .y = 0}) {} + explicit GeographyVertex(GeoCoordinate value) : coordinate(value) { + if (std::abs(coordinate.y) == 90) { + coordinate.x = 0; + point = {0, 0, coordinate.y > 0 ? 1.0 : -1.0}; + } else { + if (coordinate.x == 180) { + coordinate.x = -180; + } + point = S2LatLng::FromDegrees(coordinate.y, coordinate.x).ToPoint(); + } + } + bool operator==(const GeographyVertex& other) const { + return coordinate.x == other.coordinate.x && coordinate.y == other.coordinate.y; + } + bool operator<(const GeographyVertex& other) const { + return std::pair {coordinate.x, coordinate.y} < + std::pair {other.coordinate.x, other.coordinate.y}; + } +}; + +static bool geography_antipodal(const GeographyVertex& a, const GeographyVertex& b) { + return a.coordinate.y == -b.coordinate.y && + (std::abs(a.coordinate.y) == 90 || + geography_degree_sum(std::max(a.coordinate.x, b.coordinate.x), + -std::min(a.coordinate.x, b.coordinate.x)) == + std::pair {180.0, 0.0}); +} + +struct GeographyCoordinates { + GeoShapeType type = GeoShapeType::GEO_SHAPE_ANY; + DorisVector<DorisVector<GeographyVertex>> sequences; + GeoParseStatus status; + + explicit GeographyCoordinates(StringRef wkb) { + GeoCoordinateListList raw; + status = WkbParse::calculation_coordinates(wkb.data, wkb.size, &raw, &type); + if (status != GeoParseStatus::GEO_PARSE_OK) { + return; + } + for (const auto& sequence : raw.list) { + auto& vertices = sequences.emplace_back(); + for (const auto& coordinate : sequence->list) { + vertices.emplace_back(coordinate); + } + } + } +}; + +static std::unique_ptr<GeoShape> decode_geography_calculation_shape(StringRef value) { + GeographyCoordinates raw(value); + if (raw.status != GeoParseStatus::GEO_PARSE_OK) { + return nullptr; + } + if (raw.type != GeoShapeType::GEO_SHAPE_POLYGON) { + GeoParseStatus status; + return GeoShape::from_wkb_bytes(value.data, value.size, status); Review Comment: [P2] Stabilize LineStrings before legacy S2 admission For default GEOGRAPHY, `LINESTRING(30 58,-150 -58.00000000000001)` has non-antipodal raw endpoints, but the pinned [degree conversion](https://github.com/google/s2geometry/blob/v0.10.0/src/s2/s1angle.h#L290-L292) and [ToPoint](https://github.com/google/s2geometry/blob/v0.10.0/src/s2/s2latlng.cc#L32-L38) produce exactly opposite binary64 vectors. This branch calls `GeoShape::from_wkb_bytes -> readLine -> to_s2polyline`, whose [S2 validation](https://github.com/google/s2geometry/blob/v0.10.0/src/s2/s2polyline.cc#L148-L169) rejects those vectors before `GeographyEdgeNodes` can stabilize the raw arc. Thus this supported calculation fails with an unsupported-shape error: ```sql SELECT ST_Distance( ST_GeogFromWKB('0102000000020000000000000000003e400000000000004d400000000000c062c00100000000004dc0'), ST_GeogFromWKB('0101000000000000000000000000000000008056c0')); ``` The same raw arc subdivided as `LINESTRING(30 58,30 -90,-150 -58.00000000000001)` passes admission and contains the queried south pole. Apply raw-coordinate antipode validation and stable subdivision to typed LineStrings before S2Polyline validation, and cover equivalent subdivision/reversal. This is source/arithmetic evidence, not a Doris runtime test. Unlike 4379422808's downstream identity collision and 4379422839's Polygon hole admission, this fails in the remaining LineString admission branch before noding. -- 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]
