Repository navigation
feat: support geospatial results on the kernel path - #492
Conversation
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
ab7567d to
1d78b88
Compare
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
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[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
| @@ -1 +1 @@ | |||
| 80f2aee7d884994d7b0af9a9ea6078872859a9cd | |||
| b7e9310b27be16a6c42490e58c4b5c4a525320bc | |||
There was a problem hiding this comment.
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.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
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.
aakash-saravanan-db
left a comment
aakash-saravanan-db
left a comment
There was a problem hiding this comment.
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.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — solid, well-tested feature. The EWKB rewriter, config pointer plumbing (preserving explicit false), DeepCopy, DSN extraction (which also keeps the param off the SEA wire), and both unit and e2e coverage are all sound. One low-severity note about an unclear fall-through branch in ColumnTypeInfoForField where a reported geo type name could diverge from the scan path (currently unreachable given the kernel contract).
| if field.Type.ID() == arrow.STRING || field.Type.ID() == arrow.LARGE_STRING { | ||
| return varLen("STRING", scanTypeString) | ||
| } | ||
| info := ColumnTypeInfoFor(field.Type) |
There was a problem hiding this comment.
🔵 Low — When a field carries GEOMETRY/GEOGRAPHY metadata but its physical Arrow type is neither STRUCT nor STRING/LARGE_STRING (e.g. a bare BINARY), ColumnTypeInfoForField reports DatabaseTypeName = GEOMETRY/GEOGRAPHY while IsBinaryGeospatialField returns false (it only matches arrow.STRUCT). The scanner would then return the raw physical value without the SRID-embedding EWKB conversion, so database/sql would advertise a geospatial column type whose scanned value is not the documented EWKB form. This appears unreachable under the current kernel contract (binary mode always emits struct<srid,wkb>), so it's low severity — but the mismatch between the reported type name and the scan path is silent. Consider either returning the plain Arrow mapping (no name override) in this branch, or treating any logical-geo field consistently so the reported type and the scanned representation can't diverge.
2 participants
Summary
WithEnableGeoSpatialSupportand theenableGeoSpatialSupportDSN parameterdatabase/sqlwhile preserving the canonical{srid,wkb}Arrow struct throughGetArrowBatchesDependency
Depends on databricks/databricks-sql-kernel#361.
Validation
go test -tags databricks_kernel ./...against the exact kernel Rustls archive: passed