Skip to content

feat: support geospatial results on the kernel path - #492

Open
cathleeny wants to merge 10 commits into
mainfrom
cathleeny/geospatial-results
Open

cathleeny wants to merge 10 commits into
mainfrom
cathleeny/geospatial-results

Conversation

@cathleeny

@cathleeny cathleeny commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • add WithEnableGeoSpatialSupport and the enableGeoSpatialSupport DSN parameter
  • default to enabled: return EWKB through database/sql while preserving the canonical {srid,wkb} Arrow struct through GetArrowBatches
  • return WKT / EWKT strings when disabled
  • pass the value through the typed kernel C ABI only; it is never forwarded to SEA

Dependency

Depends on databricks/databricks-sql-kernel#361.

Validation

  • uncached full go test -tags databricks_kernel ./... against the exact kernel Rustls archive: passed
  • generated C header exactly matches kernel
  • live Reyden E2E against the exact archive: default-enabled, explicit-enabled, and disabled-string modes passed, including logical type metadata, WKB equality, and nulls

Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
@cathleeny
cathleeny force-pushed the cathleeny/geospatial-results branch from ab7567d to 1d78b88 Compare September 30, 2026 20:37
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
@cathleeny
cathleeny marked this pull request as ready for review October 6, 2026 23:47
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks solid — the EWKB conversion, struct-shape validation, pointer-based default/explicit config, and representation-aware column metadata are all correct and well-covered by tests. One low-severity note: the enableGeoSpatialSupport DSN carrier allocates KernelExperimental for both true and false, so an enableGeoSpatialSupport=... DSN on a Thrift connection is rejected at connect — diverging from the deliberate tokenCache=false no-op precedent in the same function, and untested.

Comment thread connector.go
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

Comment thread internal/arrowscan/ewkb.go Outdated
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Comment thread KERNEL_REV
@@ -1 +1 @@
80f2aee7d884994d7b0af9a9ea6078872859a9cd
b7e9310b27be16a6c42490e58c4b5c4a525320bc

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of adopting the declared kernel #361 dependency, could we update both KERNEL_REV and the published kernel-bindings module versions together?

The current Linux AMD64 v1.1.0 archive lacks the new geospatial setter. Please also validate a consumer build without local archive overrides, since the source-built smoke test does not exercise that distribution path.

Comment thread internal/backend/kernel/kernel_test.go
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — a clean, well-tested feature with solid EWKB conversion, consistent config plumbing, and thorough unit + e2e coverage. One low-severity defensive note on a panic in the per-row scan path; nothing blocking.

Comment thread internal/arrowscan/ewkb.go
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — clean, well-tested geospatial feature. The EWKB rewriter (byte-order, SRID placement, dimension flags, child-type validation, bounds checks), config/DSN plumbing (pointer preserves explicit false through DeepCopy and the kernel build), and the type-reporting/value-scanning lockstep are all correct. One low-severity note on an unreachable fallback branch in ColumnTypeInfoForField; no blocking issues.

Comment thread internal/arrowscan/coltype.go

@aakash-saravanan-db aakash-saravanan-db left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. No further functional concerns from the review. As discussed, merge kernel #361 first, then update both KERNEL_REV and the published kernel-bindings module versions and rerun verification against those artifacts before merging this PR, including a consumer build without local archive overrides.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants