jiayuasu commented on PR #1142: URL: https://github.com/apache/sedona-db/pull/1142#issuecomment-5262845763
Thanks — every one of these reproduced, and all eight are now addressed across five commits. Two of the findings needed a small refinement, noted below, and one turned out to be an engine bug rather than something this layer could fix. ## P1 **`mutate` availability.** Raised the floors to `sedonadb>=0.5.0a0` / `sedonadb-expr>=0.5.0a0`, confirming that tagged 0.4.0 contains no `def mutate` and that the prerelease form still admits the `0.5.0aN` nightlies this is developed against (a plain `>=0.5.0` would reject them). Worth flagging that the exposure is older and wider than the finding says: `to_crs()` has used `mutate` since the package was first added in #1052, so the package has been broken on install against the only released `sedonadb` all along. This fixes that too. **Geometry-general union.** Replaced `ST_Union_Agg` with collect-then-`ST_UnaryUnion`. The union has to be a separate projection rather than part of the aggregate, since a scalar function wrapped around an aggregate is not a valid aggregate expression. Dissolving points and linestrings now returns geometry that is *equal to*, and of the same type as, what GeoPandas returns, and the polygon results are unchanged. **Expression provenance.** Withdrew `Series.expr` and made assignment reject a bare expression. Confirmed the reported behaviour first: `left["copied"] = right["v"].expr` produced `[1, 2]` where the right frame held `[10, 20]`. There is no way to validate the origin of a bare expression after the fact, so rather than make it origin-aware I removed the unguarded surface: assignment now takes a `Series` (whose frame is checked) or a literal, which holds a value rather than a column reference. ## P2 **`dwithin` and crossing lines — this is an engine bug**, now filed as #1156. `ST_Distance` between two properly crossing LineStrings returns `sqrt(2)`, the gap between the nearest endpoints, rather than `0`, while `ST_Intersects` on the same pair correctly returns `true`. I scoped it: only the LineString/LineString crossing case is wrong; touching-at-endpoint, parallel lines, point-vs-line, overlapping polygons, and a line crossing a polygon are all correct. Since anything that crosses also intersects, and intersecting implies a distance of zero, `sjoin` now uses the union of `ST_DWithin` and `ST_Intersects`, which recovers the missing pair without changing any other case. Added the row-pair assertion you asked for, plus a parametrized check that the non-crossing distances are unchanged. **Dissolve missing-value semantics — one part of this needs correcting.** `first_value` does skip true SQL nulls; it returns `2` for a column of `[NULL, 2]`, matching GeoPandas. The mismatch you saw comes from pandas: a missing value in a numeric column arrives as NaN, which the engine treats as an ordinary floating-point value where GeoPandas' `first` skips it. I have documented that precisely rather than "fixing" something that behaves correctly for nulls. The null-key case is now fixed properly with `dropna=True`, matching GeoPandas' default. The empty-input case is documented: a grouping-free SQL aggregate returns one row, and detecting emptiness would mean executing the query first, which defeats the laziness the rest of the API depends on. **Scalar classification.** Replaced the `__array__` test, which was wrong in both directions as you say. Sequences are now rejected explicitly and array-likes are judged by dimensionality, so a NumPy scalar is accepted (0-d) while an array, list, tuple, set, or pandas Series is rejected. Shapely geometries are accepted as scalars, which also gives a clean way to set a constant geometry. **Geometry revalidation.** The active geometry column is now recomputed after assignment, so replacing it with a number clears it: `.geometry` raises and `.crs` returns `None` instead of `.area` failing later with a kernel error. On the CRS half of that note — a scalar Shapely geometry carries no CRS, so assigning one does drop it. I have left that as-is for now rather than silently re-stamping the previous column's CRS onto different data; a `set_crs()` is the honest way to express that intent and belongs with the accessor work. **Suffix collisions — the asymmetry matters.** Collisions are now computed over the columns actually emitted, after deciding which side's geometry is discarded. Checking GeoPandas' behaviour for the reported case, it produces `['x', 'geom', 'index_right', 'geom_right']`: the retained geometry keeps its own name and only the opposite side's ordinary column is suffixed, unlike ordinary collisions which suffix both sides. The fix matches that rather than suffixing both. ## Tests 63 tests, up from 46, with a regression test for each finding — dissolve across all three geometry types, `dropna`, bare-expression rejection, the absence of the escape hatch, sequence and NumPy classification, geometry-state revalidation, the crossing-line `dwithin` pair, and the collision shape. Everything is asserted against GeoPandas where a comparison exists. The suite passes both locally and in a clean environment installing `sedonadb` from the nightly index, and `ruff format` / `ruff check` are clean. -- 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]
