Repository navigation
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>
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
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.
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