[cathleeny]

What's Changed

• Recursively transform GEOMETRY and GEOGRAPHY inside structs, arrays, and maps.
• Return nested geoarrow.wkb fields in enabled mode and WKT or EWKT strings in disabled mode.
• Preserve the configured representation through complex-type serialization.
• Add Reyden and legacy-path E2E scenarios for GeoArrow and string modes.
• Document the option and supported output representations.

Stack: 3 of 3. Depends on #700.

Testing

• Focused geospatial suite: 44 passed; 8 live E2E cases skipped because DATABRICKS_TEST_CONFIG_FILE is unavailable.
• Complete C# unit namespace: 981 passed, 0 failed.
• CI is running on the rewritten stack.

[peco-review-bot]

Verdict: 1 Low

Looks good — solid, well-tested implementation of recursive geospatial rewriting through structs/arrays/maps, with correct offset preservation (children are rewritten 1:1 and parent offset/length retained) and good error paths. One low-severity note: in the serialize-outer-complex path the stream declares a column as StringType but emits a struct/list/map array, relying on the always-chained ComplexTypeSerializingStream and on RecordBatch not validating column types — a latent fragility rather than a current bug.

[peco-review-bot]

Verdict: 1 Low

Looks good — a thorough, well-tested feature (recursive GEOMETRY/GEOGRAPHY rewrite inside struct/array/map, GeoArrow-WKB vs WKT/EWKT modes, offset-aware validity masking). I verified the recursion, sliced-offset handling, and sibling conversions against the unit/E2E tests and found the logic internally consistent; the documented default (true) matches the code. One low-severity note about leaf type reconciliation being TypeId-only is posted inline.

[peco-review-bot]

Verdict: 1 Low

Looks good — a solid, well-tested feature with no correctness concerns found. The recursive transform, validity-masking (parent-null propagation through struct/list/map/fixed-size-list), sliced-offset handling, and both GeoArrow-WKB and WKT/EWKT string modes are carefully reconciled against the manifest-declared vs. physical Arrow shapes, and coverage (unit + E2E, nested + null + sliced cases) is strong. Only one low-severity diagnostic nit: error messages for geo values nested inside arrays/maps reference the container path and flattened child-row indices rather than the element location.

[peco-review-bot]

Verdict: 1 Low

Looks good overall — a careful, well-tested implementation; one low-severity note about execute-time semantics. The recursive geo rewrite (struct/list/largelist/fixedsizelist/map), null-masking across sliced parents, physical-vs-logical type reconciliation of non-geo siblings, and the async schema-prefetch lifecycle (dispose-on-throw, first-batch preservation, EOF/empty handling) are each backed by dedicated unit tests, and the GeoArrow-vs-string modes have E2E coverage. Docs/CHANGELOG and the true default are consistent with the code. Worth noting for reviewers (not anchored to this diff): with enable_geospatial_support defaulting to true, GEOMETRY/GEOGRAPHY columns now return GeoArrow WKB byte[] by default instead of EWKT strings — a visible output-type change for existing consumers of geo columns, presumably intended as part of this feature stack.

[peco-review-bot]

Verdict: 1 Low

Looks solid — the recursive GEOMETRY/GEOGRAPHY rewrite through structs/arrays/maps, the parent-validity masking, offset/slice handling, and the SEA manifest-vs-IPC schema reconciliation in CreateAsync are carefully implemented and well covered by the new unit tests (sliced containers, null-outer masking, parameterized sibling preservation, EOF/prefetch accounting). One low-severity telemetry-ordering note inline.

Nit (summary-only, not posted inline): the synchronous ExecuteQuery path now bridges via MaybeWrapComplexTypesAsync(...).GetAwaiter().GetResult() (DatabricksStatement.cs:280,293), which blocks a thread on the newly-eager first-batch read; this is the standard sync/async bridge but now carries real I/O, so it is marginally more exposed to thread-pool starvation under load (and to deadlock only if ever invoked under a captured SynchronizationContext whose inner reader doesn't fully ConfigureAwait(false)). No action required if ADB

[...truncated to keep verdict scannable]

[peco-review-bot]

Verdict: 2 Low

Solid, well-structured feature. The recursive geo rewrite across struct/list/largelist/fixedsizelist/map, the native↔GeoArrow↔EWKT conversions, null/validity masking, and the SEA manifest-vs-IPC schema reconciliation (prefetch one batch in CreateAsync) are all backed by an unusually thorough unit suite — sliced parents, out-of-slice masking, parameterized non-geo siblings (timestamp/decimal), EOF preservation, and the no-prefetch fast path for non-geo schemas. No correctness or security issues found in the transform logic.

Two low-severity notes posted inline (a resource-cleanup gap and a Decimal256 validation asymmetry). One behavioral note not worth an inline anchor: ExecuteQuery/ExecuteQueryAsync now eagerly fetch the first record batch for geospatial result sets (needed for manifest reconciliation), so data-level errors in that first batch (invalid WKB/WKT, shape mismatch) now surface from the execute call itself rather than during s

[...truncated to keep verdict scannable]

[eric-wang-1990]

Adding to the bot's note: for any result with a geo column, ExecuteQuery now blocks on the first batch, which on CloudFetch is a full file download. Errors from that fetch also surface from ExecuteQuery instead of the first ReadNextRecordBatchAsync. Is reading the batch eagerly avoidable, e.g. by making the plan lazily on the first read when SEA's manifest schema and the IPC schema differ? If not, please document that only geo results behave this way.

[cathleeny]

Kept eager fetching. Documented why it is needed + CloudFetch latency + behavior in both geospatial output modes.

[peco-review-bot]

Verdict: 1 Medium

Solid, well-tested change — the nested geospatial rewrite (struct/list/map recursion, validity masking, sliced-offset handling), schema reconciliation prefetch, and resource disposal are internally consistent and backed by thorough unit/E2E coverage. Telemetry ordering (EmitTelemetry before nulling _lastQueryResult, chunk-metrics caching in CloudFetch/composite Dispose) is correct. One medium concern: the new synchronous prefetch in ExecuteQuery() blocks on a CloudFetch read path that lacks ConfigureAwait(false), a potential sync-over-async deadlock under a captured SynchronizationContext.

[peco-review-bot]

🟡 Medium — The synchronous ExecuteQuery() now blocks on the new geospatial prefetch via MaybeWrapComplexTypesAsync(result).GetAwaiter().GetResult() (and the ctx == null branch above does the same). CreateAsync reads the first batch, which for a CloudFetch result set routes through CloudFetchReader.ReadNextRecordBatchAsync → downloadManager.GetNextDownloadedFileAsync(...) / await this.currentDownloadResult.DownloadCompletedTask — none of which use ConfigureAwait(false). GeospatialTransformingStream.CreateAsync only applies ConfigureAwait(false) to its own await; the inner reader's awaits still capture the calling SynchronizationContext.

Before this PR the synchronous execute path did not read any result batch (MaybeWrapComplexTypes did no I/O), so this introduces a new blocking wait over context-capturing continuations. If ExecuteQuery() is invoked from a thread with a non-default SynchronizationContext (UI / legacy ASP.NET) against a geospatial CloudFetch result, the download continuation will be posted back to the context that is blocked inside GetResult(), producing a classic sync-over-async deadlock. The async ExecuteQueryAsync() path is unaffected (it awaits with ConfigureAwait(false)).

Consider making the inner CloudFetch read path use ConfigureAwait(false), or documenting that the synchronous API must not be called from a thread with a captured context when geospatial prefetch is active.