Gabriel39 commented on PR #69032:
URL: https://github.com/apache/doris/pull/69032#issuecomment-6409761653
Review follow-up on `329498a9c2d1ad17562559174ef5d2f83b0ce947`, focused on
spatial correctness, memory usage, and the supported computation domain.
The following two concerns still apply to this head and already have open
inline threads. I independently checked the relevant code and the arithmetic
behind the examples; this comment consolidates those existing findings.
1. **Equivalent non-meridian geodesic subdivisions can produce incorrect
equality and containment results.**
For default Geography values imported as WKB:
```text
A = LINESTRING(45 -45,135 45)
B = LINESTRING(45 -45,90 0,135 45)
```
The added vertex is the midpoint of the same great-circle arc, so
`ST_Equals(A,B)` and mutual `ST_Contains`/`ST_Within` should be true.
Independently converting the coordinates to S2 vectors gives a nonzero exact
determinant (approximately `-3.52e-17`). The strict coplanarity check therefore
misses the subdivision, leaving the Boolean equality/containment paths able to
return false. The conversion-distance fallback for Intersects/Disjoint does not
repair these paths.
Please preserve this subdivision equivalence without merging nearby
distinct curves, and add rotated great-circle subdivision tests with nearby
negative controls.
[Current
code](https://github.com/apache/doris/blob/329498a9c2d1ad17562559174ef5d2f83b0ce947/be/src/exprs/function/geo/functions_geo.cpp#L214-L241)
· [Existing inline
finding](https://github.com/apache/doris/pull/69032#discussion_r4374330645)
2. **Common-meridian noding can expand small inputs into quadratic scratch
memory.**
Let A contain N edges alternating between `(10,0)` and `(10,1)`, and B
contain M distinct intermediate vertices `(10,i/(M+1))`. These lines are
admitted by the current validation. Each A edge copies all M intermediate
vertices, producing `N*(M+1)+1` points in the first expanded vector.
At `N=M=10,000`, the combined WKB size is 320,034 bytes, while that
vector alone contains 100,010,001 S2Points: at least 2,400,240,024 bytes of
point payload, before vector capacity overhead and subsequent S2 indexing. This
creates an OOM risk in predicates and distance calculations using this noding
path.
Please avoid materializing the full cross-product, bound/account scratch
allocation, and add complexity coverage for retraced lines against dense
collinear lines.
[Current
code](https://github.com/apache/doris/blob/329498a9c2d1ad17562559174ef5d2f83b0ce947/be/src/exprs/function/geo/functions_geo.cpp#L254-L306)
· [Existing inline
finding](https://github.com/apache/doris/pull/69032#discussion_r4374330649)
**Computation scope:** Recursive ARRAY elements, ordinary-key MAP values,
and STRUCT fields support read/write, and extracted spatial leaves can be
passed to supported spatial functions. However, the requested relationship and
distance operations are restricted to default `GEOGRAPHY(OGC:CRS84, spherical)`
with supported nonempty XY Point/LineString/Polygon inputs. Typed `GEOMETRY`
read/write and raw accessors are supported, but planar Geometry
predicates/distance are not implemented. Multi*/GeometryCollection, EMPTY, Z/M
calculations, other CRSs and other Geography algorithms remain outside this
computation domain. This is a declared scope limitation; if the original
requirement includes those inputs, it remains a functional gap.
Validation here was static code inspection plus independent exact-arithmetic
and allocation-size calculations. I did not execute Doris SQL reproductions,
benchmarks, or an OOM experiment.
--
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]
