feat(csharp): convert geospatial query results

[cathleeny]

What's Changed

• Add the client-side geospatial representation option, defaulting to GeoArrow WKB; disabled mode returns WKT or EWKT strings.
• Convert Reyden's internal SRID plus WKB struct to public geoarrow.wkb binary values.
• Convert legacy WKT or EWKT text results to the same GeoArrow output.
• Preserve fixed column SRIDs in GeoArrow metadata and per-row ANY SRIDs in EWKB.
• Mark GEOGRAPHY fields with spherical edges.
• Apply conversion on SEA/Reyden and Thrift paths without forwarding the option to the server.

Stack: 2 of 3. Depends on #699.

Testing

• Top-level geospatial conversion tests: 19 passed.
• Stack-wide C# unit suite: 981 passed.
• CI is running on the rewritten stack.

[peco-review-bot]

Verdict: 1 Low

Looks good — the geospatial conversion is well-structured and the data flow is consistent across both the Thrift (MaybeWrapComplexTypes) and SEA paths: geo columns are detected via Spark:DataType:SqlName metadata, are correctly excluded from ComplexTypeSerializingStream (which only matches ARRAY/MAP/STRUCT names), and schema/batch types stay in agreement in both native-struct and string modes; sliced-struct offset handling is explicitly tested. One low-confidence coverage gap on WKT formatting (F1). One behavior worth reviewer awareness, not filed as a defect since it's documented and intentional: with the default config (enable_complex_datatype_support=false, enable_geospatial_support=true), GEOMETRY/GEOGRAPHY columns are returned as native Arrow struct<srid,wkb> while all other complex types are JSON-stringified — consumers that can't consume nested Arrow structs will need to set enable_geospatial_support=false.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Solid, well-tested PR — the WKB↔EWKB codec and the geo-transform stream handle byte order, ISO→EWKB flag conversion, nested collections, SRID validation, null children, large/sliced arrays, and both native-struct and legacy-text inputs correctly, with thorough unit coverage. The batch-rewrite/non-dispose pattern matches the existing sibling streams, and the feature flag is correctly kept client-side (not forwarded to SEA). Two non-blocking notes: a medium heads-up that the default-on flag changes GEOMETRY/GEOGRAPHY result columns from WKT strings to binary GeoArrow WKB (a wire-format change for existing consumers), and a low perf note that the stream fully parses every metadata-tagged column's type text — including nested complex types — even for non-geo results.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Solid, well-tested PR. The WKB/EWKB conversion math (ISO dimension offsets → EWKB flags, endianness, nested-collection recursion, trailing-byte checks) and the fixed-vs-ANY SRID handling are careful and match the extensive unit tests; the new stream is correctly composed before ComplexTypeSerializingStream and preserves the Spark:DataType:SqlName metadata so geo columns aren't re-serialized as JSON. Two substantive notes posted inline: a backward-compatibility concern around the true default flipping existing geometry output from WKT strings to binary (F1, medium), and a validation asymmetry on the fixed-SRID native branch (F2, low).

Nits / observations not posted inline: (1) DatabricksStatement.MaybeWrapComplexTypes lost its XML-doc rationale for the PECO-3032/D3 complex-type JSON fix — the comment is now a single generic sentence while the serializer behavior it documented still exists. (2) Geospatial values nested inside complex t

[...truncated to keep verdict scannable]

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Solid, well-tested feature (WKB/EWKB rewriting, SRID validation, GeoArrow metadata, and SEA/Thrift wiring all look correct and have good unit coverage). Two things worth a look: (1) a medium gap — nested geospatial values inside STRUCT/ARRAY/MAP are parsed into tagged transport structs but never converted, so they leak the internal struct<srid,wkb> representation (raw or JSON) rather than WKB/WKT; and (2) a low note that defaulting EnableGeospatialSupport to true changes the existing GEOMETRY/GEOGRAPHY result type from string to binary for current consumers. The client-side flag correctly stays out of SEA session_confs (verified against the ServerSidePropertyPrefix filter and the new SSP test).

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Solid, well-tested addition — the WKB↔EWKB rewriter and WKT rendering carry byte-exact unit coverage that checks out, SRID validation is thorough, and the batch-ownership/schema-wrapping pattern matches the existing ComplexTypeSerializingStream/IntervalSerializingStream. Two things worth a look: nested GEOMETRY/GEOGRAPHY inside complex types is mapped to the internal struct<srid,wkb> transport type but never converted by GeospatialTransformingStream, so that internal shape leaks to callers (medium, possibly deferred to a later stack); and the default-on EnableGeospatialSupport=true changes the output type of existing geospatial columns without opt-in, unlike the default-off EnableComplexDatatypeSupport (low — confirm intent).

Nit (not inlined): MaybeWrapComplexTypes in DatabricksStatement.cs lost its doc comment explaining the PECO-3032 / malformed-MAP-JSON rationale for ComplexTypeSerializingStream; the terser replacement no longer records why that wrapper exists.

[peco-review-bot]

Verdict: 1 Low

Solid, well-tested PR — the WKB/EWKB codec and GeospatialTransformingStream have thorough round-trip and edge-case coverage, the flag is wired consistently across Thrift/SEA with a true default, and the setting is correctly kept client-side (verified via the SSP test and telemetry mapping). One low-severity scoping note: nested GEOMETRY/GEOGRAPHY columns (inside ARRAY/MAP/STRUCT) are not converted and leak the internal struct<srid,wkb> transport type when EnableComplexDatatypeSupport=true. Also note (no action needed) that defaulting enable_geospatial_support=true changes the default geo output representation to binary GeoArrow WKB — intended per the PR description.

[eric-wang-1990]

Two related questions on how geo columns are detected:

1. Which physical types can the server send? The code handles a tagged struct<srid,wkb> (Reyden) and String/LargeString EWKT (legacy; main's GeospatialDataColumnReportsStringEwkt shows strings on both Thrift and SEA), and throws on anything else. Could you document which protocol/server versions/configs produce which representation, and whether it can vary within one result (inline vs CloudFetch, chunk to chunk)? If it can, a test mixing string and struct batches would be good, especially for feat(csharp): support nested geospatial results #701, which plans from the first batch only. Also, can the server ever send plain Binary WKB or an untagged struct? Today that would fail the whole query.

2. Detection convention. Since a legacy string column can only be recognized via Spark:DataType:SqlName, that key is the real source of truth here, as in ComplexTypeSerializingStream / IntervalSerializingStream / NullColumnSerializingStream. But a geo-tagged physical struct also counts as geo on its own, even when Spark:DataType:SqlName says STRUCT or STRING. Unless there's a real case for that, could the physical tag only validate (throw InvalidData when Spark:DataType:SqlName says non-geo but the payload is tagged, as with the SRID mismatch), and be used alone only when Spark:DataType:SqlName is missing? Same for the nested ResolveTag(source.DataType) fallback in feat(csharp): support nested geospatial results #701's RewriteData.

Related: the SEA legacy case, where the manifest-derived schema declares the tagged struct<srid,wkb> but the IPC batches contain EWKT strings, works because RewriteGeo dispatches on the batch's array type. But I don't see a unit test for it; the existing tests declare StringType in the schema. Could you add one (declared tagged struct + StringArray batch, both modes)? Also, in string mode legacy text is passed through verbatim while native values are rendered locally. Is the formatting guaranteed to match (spacing, coordinate precision)?

[eric-wang-1990]

Perf question: this stream now wraps every result (including non-geo, which is fine since it's a pass-through), but each geo column is rebuilt row by row. A few suggestions:

1. Fixed SRID (default path): the output is byte-identical to the wkb child, so it could reuse the child's offsets/data buffers and only combine the struct validity, which is roughly zero-copy.
2. ANY → EWKB: WkbToEwkbRewriter copies through a List<byte> one byte at a time and allocates a byte[4] per uint32. Writing into a pre-sized byte[] (input length + 4) with Span copies would cut this to about one copy.
3. String mode: NTS parsing into a full Geometry per row is the heaviest path. Since you already have a custom WKT writer, could it read WKB directly without building a Geometry?

Could you add a quick benchmark (e.g. 1M rows of points and of 1K-vertex polygons, per mode) so we know the overhead before this ships with the default flipped to true?

[eric-wang-1990]

Echoing the bot's comment with a human +1: before this stack, geo columns came back as WKT/EWKT strings on both Thrift and SEA (GeospatialDataColumnReportsStringEwkt on main asserts this). Defaulting enable_geospatial_support to true silently changes the column type from String to Binary for existing users. Could we default to false (opt-in, like enable_complex_datatype_support), or if true is intentional, call it out as a behavior change in the CHANGELOG (#701 currently lists it under "Added")?

Also, have we tested this behavior change in Power BI already? Power BI is one of the main consumers of this driver, so it would be good to confirm how it handles a geo column that arrives as geoarrow.wkb binary instead of a string, both for existing reports/models and for new ones.

[peco-review-bot]

Verdict: 1 Low

Looks solid — the WKB↔WKT/EWKB codec and the SEA/Thrift stream wiring are carefully implemented and backed by thorough round-trip tests (dimensional markers, empty members, byte-order, sliced-struct offset, fixed-vs-ANY SRID validation, GeoArrow metadata). No correctness defects found in the top-level conversion path; the flag is correctly kept client-side (not forwarded to SEA) and defaults are consistent across both connection classes.

Two things for the human reviewer to confirm rather than treat as bugs: (1) EnableGeospatialSupport defaults to true, so merging this flips the on-the-wire representation of existing top-level GEOMETRY/GEOGRAPHY results from server WKT strings to GeoArrow WKB binary — this is the PR's stated intent but is a behavioral break for current consumers; (2) one inline Low on nested-geo handling.

[peco-review-bot]

Verdict: 1 Low

Solid, well-tested PR. The WKB↔WKT/EWKB codec (GeospatialWkb) and the per-column plan/convert logic in GeospatialTransformingStream are carefully implemented and backed by extensive unit tests (dimensional markers, empty members, byte order, sliced-offset structs, fixed/ANY SRID, legacy text). Struct-child access is guarded by GeospatialArrowType.IsStructShape (exact 2-field shape), and the undisposed-original-batch rewrite pattern matches the existing ComplexTypeSerializingStream. One low-severity asymmetry noted inline. Two things to confirm (not code bugs): (1) EnableGeospatialSupport defaults to true on both paths, so GEOMETRY/GEOGRAPHY results that previously surfaced as StringType/WKT now become geoarrow.wkb binary by default — ensure this default-on output-format change is intended and called out in release notes; (2) geospatial columns nested inside complex ARRAY/MAP/STRUCT values are not converted to GeoArrow (only to

[...truncated to keep verdict scannable]

[peco-review-bot]

🔵 Low — Fixed-SRID validation is asymmetric between the two disabled-mode (string) input paths.

When outputType.TypeId == String and the source is a native struct, RewriteGeo routes to ConvertToEwkt, which calls ValidateFixedSrid for every row — so a GEOMETRY(4326) column carrying a row whose SRID is 3857 is rejected with InvalidData (covered by FixedSrid_StringModeRejectsMismatchedRowSrid).

But when the source is already text (legacy WKT/EWKT), this early branch returns the string verbatim without ever parsing/validating the embedded SRID=...:

if (outputType.TypeId == ArrowTypeId.String)
{
    return source.DataType.TypeId == ArrowTypeId.String
        ? source
        : ConvertLargeStringToString(source).Data;
}

So for a fixed-SRID column, a legacy-text row like SRID=3857;POINT(1 2) on a GEOMETRY(4326) column passes through silently in disabled mode, whereas the identical mismatch is thrown in native mode (ConvertTextToGeoArrow → ValidateFixedSrid) and in disabled-struct mode. This may be intentional (render legacy text as-is), but the inconsistency means the same bad data is accepted or rejected depending only on the server's physical encoding and the flag. Worth a comment at minimum so the divergence is deliberate rather than accidental.