kernel: distribute the kernel via per-platform bindings modules (go get, no build step, all 5 platforms) - #440
kernel: distribute the kernel via per-platform bindings modules (go get, no build step, all 5 platforms)#440msrathore-db wants to merge 1 commit into
Conversation
![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 Medium
Distribution-model change (nested per-platform kernel module) looks sound for the in-tree build: build constraints on cgo_darwin.go and link.go match, the default Thrift build stays CGO-free, and .gitignore/header handling is consistent. One Medium concern: the require v0.0.0 + local replace won't resolve for external go get consumers once a release is cut, since replace isn't transitive and module-graph resolution ignores build tags — the release must be gated on publishing/tagging the nested module.
| // only the archive for the platform it targets (and nothing at all for a | ||
| // pure-Go Thrift build). The replace pins them to the in-tree directories; when | ||
| // published, the require versions are what a `go get` consumer resolves. | ||
| require github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/darwin_arm64 v0.0.0 |
There was a problem hiding this comment.
🟡 Medium — The nested module is wired with require .../kernellib/darwin_arm64 v0.0.0 + a local replace. This works for in-tree builds, but note two things that undercut the PR's "works straight from go get" goal for external consumers:
-
replaceis not transitive. A downstream project that doesgo get github.com/databricks/databricks-sql-goignores this repo'sreplacedirective entirely (replace is honored only in the main module). It sees only the barerequire .../darwin_arm64 v0.0.0. -
Module-graph resolution is build-tag-independent. MVS must load the
go.modof everyrequired module to build the graph, even for a pure-Thrift (CGO_ENABLED=0, no tag) build that never compiles a file from the nested module. Becausev0.0.0is not a published/tagged version of the nested module, that resolution would fail for all consumers — not just kernel builds — with an "unknown revision" error, once a release of this repo is cut carrying thisgo.mod.
The PR description acknowledges this ("when published, the require versions are what a go get consumer resolves"), so this is a known follow-up rather than a defect in the in-tree workflow. Flagging so the release that publishes this is gated on: tagging the nested module at a real version and updating the require to match. Until then, external go get of a tagged release would break even for Thrift-only users.
There was a problem hiding this comment.
Verdict: 1 High · 1 Low
Solid, well-documented distribution change; the nested-module wiring is correct for in-repo builds. Main concern (High): the unconditional require ... v0.0.0 + local replace in go.mod resolves in-repo but not for external go get consumers (replace is ignored downstream), which would break the default build until the nested module is actually published/tagged — contradicting the README's "works straight from go get" claim. One Low note on the sync workflow swallowing go mod verify failures. Nit: the verify step's comment says it "type-checks the darwin build" but the command builds linux/amd64.
| // only the archive for the platform it targets (and nothing at all for a | ||
| // pure-Go Thrift build). The replace pins them to the in-tree directories; when | ||
| // published, the require versions are what a `go get` consumer resolves. | ||
| require github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/darwin_arm64 v0.0.0 |
There was a problem hiding this comment.
🟠 High — The nested module is pulled in with an unconditional require ... v0.0.0 plus a replace => ./internal/backend/kernel/kernellib/darwin_arm64. This resolves correctly for in-repo builds (make test, go build ./..., CI), because replace short-circuits the placeholder v0.0.0.
But replace directives are ignored in any module other than the main module (Go modules reference). So for an external consumer that does go get github.com/databricks/databricks-sql-go, the replace does not apply and Go must resolve .../kernellib/darwin_arm64@v0.0.0 from the proxy/VCS. Since the top-level require is unconditional, this module's go.mod is loaded for every consumer build graph — including a pure-Thrift, CGO_ENABLED=0 build that never compiles a kernellib file. Unless the tag internal/backend/kernel/kernellib/darwin_arm64/v0.0.0 actually exists in the published repo, go get/go build fails at module resolution (unknown revision v0.0.0) for all consumers, not just kernel users.
This directly contradicts the README section added in this PR ("works straight from go get with no build step") and the PR's headline claim. The PR text acknowledges "when published, the require versions are what a go get consumer resolves" — so the gap is known, but as it stands the merged+tagged state would break the default build for downstream consumers. Please confirm the publish/tagging plan makes v0.0.0 (or the eventual pinned version) resolvable before the README advertises frictionless go get, or gate the require so a Thrift build doesn't force resolution.
| branch="sync/kernel-${KERNEL_TAG}" | ||
| git config user.name "databricks-ci" | ||
| git config user.email "ci@databricks.com" | ||
| git checkout -b "$branch" |
There was a problem hiding this comment.
🔵 Low — go mod verify || true swallows any failure, so a genuinely corrupt/tampered module cache would not fail this "verify module graph resolves" step — only the subsequent go build gates the job. The comment frames this step as catching a broken go.mod/missing module; the || true defeats the go mod verify half of that intent. Consider dropping || true (or logging explicitly why a failure is tolerated) so the checksum verification of the freshly-committed archive can actually fail the sync PR.
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-documented build/distribution change; the build constraints on cgo_darwin.go and the nested link.go match, and the Thrift-build exclusion reasoning is sound. One medium concern: the require v0.0.0 + local replace only makes the kernel backend build in-tree — replace is ignored downstream and v0.0.0 isn't resolvable, so the "works from go get" claim isn't met for external consumers until the nested module is published at a real version.
| // only the archive for the platform it targets (and nothing at all for a | ||
| // pure-Go Thrift build). The replace pins them to the in-tree directories; when | ||
| // published, the require versions are what a `go get` consumer resolves. | ||
| require github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/darwin_arm64 v0.0.0 |
There was a problem hiding this comment.
🟡 Medium — The nested-module wiring here works only for in-tree builds — which is all the PR's verification actually exercised (go build ./... run from within this repo, where the replace applies). It does not deliver the headline "works straight from go get, no build step" for external consumers:
replacedirectives are ignored for dependencies. When someone addsdatabricks-sql-goas a dependency, only their main module'sreplacedirectives take effect; thereplaceon line 62 of this repo'sgo.modis dropped. So a downstreamgo build -tags databricks_kernelfor darwin/arm64 will try to resolve.../kernellib/darwin_arm64 v0.0.0from the module proxy, not from./internal/....v0.0.0is not a resolvable version. There is nointernal/backend/kernel/kernellib/darwin_arm64/v0.0.0tag, so that resolution fails for a consumer. go-duckdb's model (cited in the PR) requires the nested modules to be published as real tagged versions that the parentrequires directly — not av0.0.0placeholder held together by an in-treereplace.
Net effect: the go get path the PR is built around is currently only proven for builds run inside this checkout. Recommend either (a) tempering the README/PR claim to "builds from a repo checkout" until the nested modules are published and required at real versions, or (b) documenting the publish+version-bump step as a hard prerequisite before the kernel backend is advertised as go-get-installable. The Thrift-build verification is unaffected (the import is build-tag-excluded), so this only concerns the kernel-tag darwin/arm64 consumer path.
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — clean, well-documented distribution change with one low-severity note. The nested-module wiring is correct: build tags on cgo_darwin.go and link.go match exactly (cgo && databricks_kernel && darwin && arm64), the blank import collects the archive's LDFLAGS at link, the committed header sits in the driver module's own zip while the 59 MB .a lives in the excluded nested-module subtree, and .gitignore correctly un-ignores include/ without matching the still-ignored lib/. The only concern is the placeholder require v0.0.1 versus the documented lockstep-version model (F1).
| // applies when this repo is the main module — so it is safe to ship: it never | ||
| // affects a downstream `go get`, which always resolves the published version. | ||
| require github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/darwin_arm64 v0.0.1 | ||
|
|
There was a problem hiding this comment.
🔵 Low — The require pins the nested module at v0.0.1, but the adjacent comment (and docs/RELEASING.md) assert these versions are "REAL published versions ... bumped in lockstep with each driver release," so that go get databricks-sql-go@vX.Y.Z transitively pins the matching kernel archive and "upgrading the driver is what moves the kernel version."
Right now v0.0.1 is a placeholder that only builds because the local replace shadows it in this repo. That's fine for this development-phase PR, but the code contradicts the documented model, and it is load-bearing at release time: if a driver release tag ships this go.mod verbatim, a downstream -tags databricks_kernel build on darwin/arm64 will (a) require a published internal/backend/kernel/kernellib/darwin_arm64/v0.0.1 tag to exist on the proxy, and (b) freeze the kernel at v0.0.1 regardless of driver version — defeating the lockstep pinning the comment promises. Worth a TODO/note that this must be bumped and tagged (RELEASING.md steps 2–3) before it goes out in a tagged release, so the placeholder isn't shipped by accident.
There was a problem hiding this comment.
Verdict: 2 High · 1 Medium · 1 Low
The code side is coherent — the kernel shims blank-import the external databricks-sql-kernel-bindings/lib/<platform> modules and go.mod/go.sum match. But the PR's new prose (README "Cloning the repository", docs/RELEASING.md, and the .gitignore comment) documents a different, unshipped design: in-repo nested modules under internal/backend/kernel/kernellib/<platform> with committed .a files and replace directives — none of which exist (verified: no such dir, no replace in go.mod). Two high-severity doc/impl divergences (F1, F2), plus an orphan !databricks_kernel_dynlib build tag on darwin/arm64 only that can produce an opaque link failure (F3). The PR title/description itself ("nested per-platform module committed in-repo") reflects the abandoned design and should be reconciled with the external-repo reality.
|
|
||
| ``` | ||
| github.com/databricks/databricks-sql-go (the driver module) | ||
| └── internal/backend/kernel/kernellib/<platform>/ (one NESTED module per platform) |
There was a problem hiding this comment.
🟠 High — This new release doc describes a distribution model that does not match the shipped code. It documents in-repo nested modules at internal/backend/kernel/kernellib/<platform>/ with committed .a files, and a require + replace ... => ./internal/backend/kernel/kernellib/<platform> pair in the driver go.mod.
But the actual implementation (go.mod:56-72, and every cgo_*.go shim) depends on the external, separate repo github.com/databricks/databricks-sql-kernel-bindings/lib/<platform> — there is no internal/backend/kernel/kernellib/ directory, no committed archive in this repo, and no replace directive in go.mod (verified: grep '^replace' go.mod returns nothing).
So the entire "module layout", "How versioning works" (the replace lines), and "Publishing: path-prefixed tags" sections instruct a release process against modules that live in a different repository. Step 2 ("keep the matching replace lines") and step 3 (tagging internal/backend/kernel/kernellib/<platform>/vX.Y.Z in this repo) would be actively wrong. Please rewrite this doc around the external databricks-sql-kernel-bindings model that go.mod actually uses, or split out which steps happen in which repo.
|
|
||
| This repo commits a small number of **prebuilt kernel binaries** (per-platform | ||
| `libdatabricks_sql_kernel.a`, ~62 MB each, each in its own nested module under | ||
| `internal/backend/kernel/kernellib/<platform>`) so that the SEA/kernel backend |
There was a problem hiding this comment.
🟠 High — The new "Cloning the repository" section states this repo "commits a small number of prebuilt kernel binaries (per-platform libdatabricks_sql_kernel.a, ~62 MB each, each in its own nested module under internal/backend/kernel/kernellib/<platform>)" and gives a git sparse-checkout example selecting internal/backend/kernel/kernellib/darwin_arm64.
None of that exists in the tree: the archives live in the external databricks-sql-kernel-bindings repo (go.mod:68-72), not in-repo, and there is no kernellib/ path to sparse-checkout here. The --filter=blob:none rationale ("skip the committed-archive history") therefore doesn't apply to this repo as shipped — a direct clone of this repo pulls no large kernel .a at all. This section will mislead contributors into a sparse-checkout that keeps a nonexistent path. Align it with the external-module reality, or drop the committed-binary framing.
| @@ -1,20 +1,11 @@ | |||
| //go:build cgo && databricks_kernel && darwin && arm64 | |||
| //go:build cgo && databricks_kernel && !databricks_kernel_dynlib && darwin && arm64 | |||
There was a problem hiding this comment.
🟡 Medium — This shim's build constraint gained && !databricks_kernel_dynlib, but none of the other four shims (cgo_darwin_amd64.go, cgo_linux.go, cgo_linux_arm64.go, cgo_windows.go) carry it, and grep 'dynlib' finds no complementary dynlib shim anywhere. The comment still claims "Same build constraint as that module's prebuilt.go" — which now can't be true if only darwin/arm64 has this extra term.
Concrete consequence: building darwin/arm64 with -tags databricks_kernel,databricks_kernel_dynlib excludes this file (via !databricks_kernel_dynlib), while cgo_unsupported.go also excludes darwin/arm64 (its !(darwin && arm64) term). So no link shim is compiled, the external module is never blank-imported, and cgo.go's kernel_* C symbols fail at final link with the exact opaque "undefined reference" dump the unsupported-guard exists to pre-empt at compile time. This looks like a stray/incomplete edit (a planned dynlib variant with no landing file). Either drop the !databricks_kernel_dynlib term, or add the dynlib shim + extend cgo_unsupported.go so the guard still fires.
| # | ||
| # The paths below remain ignored: they are scratch dirs still used by | ||
| # `make kernel-lib` for platforms not yet committed (linux/windows) and for | ||
| # source builds — never committed. |
There was a problem hiding this comment.
🔵 Low — This comment says the committed distribution model "commits ... each platform's prebuilt archive under kernellib// (nested modules), so go get needs no build step." That describes the same in-repo model that isn't implemented (see F1/F2) — nothing is committed under kernellib/ here. The actual ignore-rule change (un-ignoring include/ so the committed header lands) is correct; only the explanatory comment is inaccurate. Trim it to reference the committed header + the external bindings repo.
(Anchored to the nearest changed line — see the description for the exact location.)
msrathore-db
changed the title
There was a problem hiding this comment.
Verdict: 3 Medium · 1 Low
Mechanically the cgo rewiring (blank-import the external bindings module per platform, ABI-version handshake at OpenSession) is sound, and the committed header + ABI check are reasonable. The main issue is that the PR pivoted the distribution model to the external databricks-sql-kernel-bindings modules (go.mod requires them, with no replace and no in-tree kernellib/), but the new docs — all of docs/RELEASING.md and the README "Cloning" section — still describe the abandoned in-tree kernellib/<platform> nested-module model, which would misdirect a release (F1/F2). One stray build-tag (!databricks_kernel_dynlib on darwin/arm64 only, with no matching dynlib file or guard update) can produce the exact opaque linker error the unsupported guard is meant to prevent (F3). Note also the cgo.go package doc comment (unchanged, so not inline-anchorable) still describes the make kernel-lib / ".gitignore'd; nothing kernel-built is com
[...truncated to keep verdict scannable]
| also carries a `replace` pointing at the in-tree source: | ||
|
|
||
| ``` | ||
| require github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/darwin_arm64 v1.2.3 |
There was a problem hiding this comment.
🟡 Medium — This entire release doc describes an in-tree module layout that the shipped code does not use. It says the driver is a multi-module repo with one nested module per platform at internal/backend/kernel/kernellib/<platform>/, that go.mod carries a require plus a replace pointing at the in-tree source, and that release means tagging internal/backend/kernel/kernellib/<platform>/vX.Y.Z.
But the actual go.mod in this PR requires the external github.com/databricks/databricks-sql-kernel-bindings/lib/<platform> modules, with no replace directives and no in-tree kernellib/ modules anywhere in the repo (grep confirms kernellib appears only in docs/comments). The PR description itself says these external bindings modules are "replacing the old in-tree kernellib/ modules."
As written, RELEASING.md is instructions for the previous design: step 2 ("keep the matching replace lines") and step 3 ("tag internal/backend/kernel/kernellib/<platform>/vX.Y.Z") would both fail — there are no replace lines and no such modules to tag. A maintainer following this doc would mis-release. Rewrite it around the external bindings repo (versions pinned via the require on databricks-sql-kernel-bindings/lib/*; publishing/tagging happens in that repo).
|
|
||
| This repo commits a small number of **prebuilt kernel binaries** (per-platform | ||
| `libdatabricks_sql_kernel.a`, ~62 MB each, each in its own nested module under | ||
| `internal/backend/kernel/kernellib/<platform>`) so that the SEA/kernel backend |
There was a problem hiding this comment.
🟡 Medium — The new "Cloning the repository" section states this repo commits per-platform prebuilt binaries (libdatabricks_sql_kernel.a, ~62 MB each) "in its own nested module under internal/backend/kernel/kernellib/<platform>", and the --filter=blob:none / --sparse guidance selects paths under internal/backend/kernel/kernellib/....
That is no longer true for the model this PR ships: the archives now live in the external databricks-sql-kernel-bindings repo (pulled by go get), not committed here — there is no kernellib/ tree in this repo. So the stated motivation ("committed binaries cannot be delta-compressed... a full clone accumulates their whole history") and the sparse-checkout example paths point at directories that don't exist in this repo. This will mislead contributors. Update the section to reflect that the kernel archives are consumed as external modules, and drop/revise the committed-binary and sparse-checkout guidance accordingly.
| @@ -1,20 +1,11 @@ | |||
| //go:build cgo && databricks_kernel && darwin && arm64 | |||
| //go:build cgo && databricks_kernel && !databricks_kernel_dynlib && darwin && arm64 | |||
There was a problem hiding this comment.
🟡 Medium — This shim's build constraint adds !databricks_kernel_dynlib, but that tag appears nowhere else in the repo: there is no databricks_kernel_dynlib-gated link file, the other four shims (cgo_darwin_amd64.go, cgo_linux.go, cgo_linux_arm64.go, cgo_windows.go) do not carry the negation, and cgo_unsupported.go's exclusion list is not dynlib-aware.
Consequence: building darwin/arm64 with -tags 'databricks_kernel databricks_kernel_dynlib' excludes this file (its !databricks_kernel_dynlib), while cgo_unsupported.go is still excluded too (its !(darwin && arm64) is false on that target). So no shim compiles — the bindings module is never blank-imported, its #cgo LDFLAGS are never collected, and the link fails with the opaque undefined reference to kernel_* that cgo_unsupported.go exists precisely to prevent. Either drop this stray !databricks_kernel_dynlib negation, or complete the dynlib variant consistently across all platforms and teach the unsupported guard about it.
| # | ||
| # The paths below remain ignored: they are scratch dirs still used by | ||
| # `make kernel-lib` for platforms not yet committed (linux/windows) and for | ||
| # source builds — never committed. |
There was a problem hiding this comment.
🔵 Low — This comment now describes a committed-archive layout ("each platform's prebuilt archive under kernellib// (nested modules)... intentionally NOT ignored") that the shipped model abandons — the archives live in the external bindings repo, and there is no kernellib/ tree here. The include/ un-ignore (committed header) is still correct, but the kernellib/<platform>/ prose is stale. Trim the comment to match reality (committed header only).
(Anchored to the nearest changed line — see the description for the exact location.)
There was a problem hiding this comment.
Verdict: 2 High · 2 Medium · 1 Low
The mechanism (go.mod + cgo shims) is coherent — external per-platform databricks-sql-kernel-bindings modules, blank-imported behind build tags — but the newly-added docs describe a different, unimplemented model (committed in-tree internal/backend/kernel/kernellib/<platform> nested modules + replace), and grep confirms no such directories or committed archives exist. Two High doc/contract mismatches (README Cloning, docs/RELEASING.md) plus a stale .gitignore comment; also an orphan databricks_kernel_dynlib guard on darwin/arm64 only, and the new imports are missing from the depguard allowlist.
|
|
||
| This repo commits a small number of **prebuilt kernel binaries** (per-platform | ||
| `libdatabricks_sql_kernel.a`, ~62 MB each, each in its own nested module under | ||
| `internal/backend/kernel/kernellib/<platform>`) so that the SEA/kernel backend |
There was a problem hiding this comment.
🟠 High — The new "Cloning the repository" section describes a distribution model that this PR does not implement. It states the repo "commits a small number of prebuilt kernel binaries ... each in its own nested module under internal/backend/kernel/kernellib/<platform>," and the git sparse-checkout example selects internal/backend/kernel/kernellib/darwin_arm64.
But no such directories or committed .a files exist — a repo-wide grep kernellib matches only documentation, never a real path. The actual mechanism (go.mod + cgo_*.go) pulls each platform's archive from the external github.com/databricks/databricks-sql-kernel-bindings/lib/<platform> module. Nothing is committed in this tree.
This also directly contradicts the README's own Building → SEA/kernel section, which (correctly, for the external model) says the archive "is not committed — build it first." A contributor following the Cloning guidance will sparse-checkout a path that does not exist and expect ~62 MB committed binaries that aren't there. The whole --filter=blob:none / --sparse rationale ("committed binaries cannot be delta-compressed by git") is moot under the shipped external-module design.
| also carries a `replace` pointing at the in-tree source: | ||
|
|
||
| ``` | ||
| require github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/darwin_arm64 v1.2.3 |
There was a problem hiding this comment.
🟠 High — This entire release runbook documents a mechanism that does not match the shipped go.mod. It claims the driver requires each platform module at github.com/databricks/databricks-sql-go/internal/backend/kernel/kernellib/<platform> and carries a replace pointing at the in-tree source, and that releasing means tagging internal/backend/kernel/kernellib/<platform>/vX.Y.Z.
The actual go.mod requires github.com/databricks/databricks-sql-kernel-bindings/lib/<platform> v0.1.0 (a separate repo) and contains no replace directives and no in-tree kernellib modules to tag. A maintainer following these steps — bump the kernellib requires, keep replace lines, tag internal/backend/kernel/kernellib/.../vX.Y.Z — would edit modules that don't exist and produce no working release; the real release must instead bump the databricks-sql-kernel-bindings/lib/* versions and coordinate tags in that other repo. The doc needs to be rewritten around the external-bindings model the PR actually ships (the cgo shim comments already reference databricks-sql-kernel-bindings).
| # | ||
| # The paths below remain ignored: they are scratch dirs still used by | ||
| # `make kernel-lib` for platforms not yet committed (linux/windows) and for | ||
| # source builds — never committed. |
There was a problem hiding this comment.
🟡 Medium — The rewritten comment describes committed in-tree paths that don't exist and misstates the model: it says archives are committed "under kernellib/<platform>/ (nested modules), so go get needs no build step," and that make kernel-lib scratch dirs remain "for platforms not yet committed (linux/windows)."
Under the shipped design nothing is committed in-tree at all (archives live in the external databricks-sql-kernel-bindings repo), and all five platforms — including linux and windows — are served by that external repo, so the "platforms not yet committed (linux/windows)" carve-out is inaccurate. Please align this comment with the external-module reality (or with F1/F2, whichever way the model is finalized).
(Anchored to the nearest changed line — see the description for the exact location.)
| @@ -1,20 +1,11 @@ | |||
| //go:build cgo && databricks_kernel && darwin && arm64 | |||
| //go:build cgo && databricks_kernel && !databricks_kernel_dynlib && darwin && arm64 | |||
There was a problem hiding this comment.
🟡 Medium — This is the only file in the repo that references the databricks_kernel_dynlib tag (&& !databricks_kernel_dynlib), and no companion dynlib shim file exists (grep finds a single hit). None of the other four platform shims gained the guard.
Consequence: building darwin/arm64 with -tags databricks_kernel,databricks_kernel_dynlib selects no shim — this file is excluded by !databricks_kernel_dynlib, and cgo_unsupported.go is excluded by its !(darwin && arm64) clause. With no blank-import providing #cgo LDFLAGS, cgo.go's C ABI calls fail at link with the opaque "undefined reference to kernel_*" that cgo_unsupported.go was specifically written to prevent. Either drop the orphan tag, add the missing cgo_darwin_dynlib.go variant, and/or extend cgo_unsupported.go so the dynlib case still produces the legible compile-time error.
| // linux/amd64 link wiring: blank-import the external per-platform module | ||
| // github.com/databricks/databricks-sql-kernel-bindings/lib/linux_amd64 (databricks-sql-kernel-bindings) so cgo collects its | ||
| // `#cgo LDFLAGS` at link time and pulls libdatabricks_sql_kernel.a into the | ||
| // binary. Same build constraint as that module's prebuilt.go. See docs/RELEASING.md. |
There was a problem hiding this comment.
🔵 Low — The new github.com/databricks/databricks-sql-kernel-bindings/lib/* imports (this file plus the other four cgo_*.go shims) are not in the depguard allowlist, and the allowed prefix github.com/databricks/databricks-sql-go does not cover the differently-named ...-kernel-bindings module. The default Lint job doesn't set build-tags, so it never compiles these databricks_kernel-tagged files and won't flag this today — but any lint run with -tags databricks_kernel (or a future kernel-tagged lint leg) will fail per the repo's stated depguard convention. Add github.com/databricks/databricks-sql-kernel-bindings to .golangci.yml's depguard.rules.main.allow.
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-documented distribution refactor — the five per-platform cgo shims carry mutually-exclusive //go:build constraints and cgo_unsupported.go excludes exactly those five, so there's no double-linking; go.sum carries both go.mod and zip hashes for all five. One medium concern: the new direct requires mean Go must resolve these (currently private) bindings modules' go.mod for every consumer build, including the default Thrift path, so the "Thrift is completely unchanged" framing is inaccurate until the bindings repo is public.
| // matching per-platform kernel archive; upgrading the driver moves the kernel | ||
| // version. See docs/RELEASING.md and the bindings repo README. | ||
| require ( | ||
| github.com/databricks/databricks-sql-kernel-bindings/lib/darwin_amd64 v0.1.0 |
There was a problem hiding this comment.
🟡 Medium — These five modules are now direct requirements of the driver, so Go must resolve each one's go.mod to compute the build list for any consumer build — including the default Thrift CUJ (CGO_ENABLED=0, no build tag). The build-tag gating on the blank imports (cgo_*.go) only prevents fetching the multi-megabyte archive zips; it does not exempt these modules from module-graph resolution. A fresh consumer with an empty module cache still fetches the (tiny) go.mod files and verifies them against go.sum.
Consequence: while databricks-sql-kernel-bindings remains private (as docs/RELEASING.md notes), an external go get github.com/databricks/databricks-sql-go@<this version> followed by go build will fail for Thrift-only consumers too — not just the kernel opt-in — because the proxy cannot serve the private go.mod files. This contradicts the README's framing that the default Thrift build is "completely unchanged" / pulls "no kernel binaries at all": no binaries, correct, but a new hard module-graph dependency on a currently-unreachable repo now gates every consumer.
Suggest: (a) do not tag a public driver release with these requires until the bindings repo is public, and (b) soften the README claim to make clear the go.mod dependency applies to all consumers, so the gating condition isn't buried in RELEASING.md.
65fef44 to
d384936
Compare
There was a problem hiding this comment.
Verdict: 1 High · 1 Medium · 1 Low
Distribution refactor is sound where it can be verified in-repo: the five per-platform build tags are mutually exclusive and cgo_unsupported.go excludes exactly that set, the compile-side #cgo CFLAGS -I${SRCDIR}/include correctly targets the newly committed header, and the enum-drift assertions are intact. Main concerns are in the module/release wiring: the unconditional requires pull the (currently private) bindings modules into the default build graph (F1, high), the documented go mod tidy step would prune those requires (F2), and the depguard allowlist omits the new import (F3).
Other findings
- 🔵 Low — The depguard
mainrule is allow-only, so any import outside the list is denied. The new blank-importsgithub.com/databricks/databricks-sql-kernel-bindings/lib/<platform>are not in the allowlist. The defaultmake lint(golangci-lint run, no-tags databricks_kernel) won't compile these tag-gated files, so it passes today — but any lint run that enables thedatabricks_kerneltag will fail depguard with a denied-import error. Recommend addinggithub.com/databricks/databricks-sql-kernel-bindingsto the allowlist now so kernel-tagged linting stays green.
| // The versions are pinned in lockstep with the driver release. A consumer's | ||
| // `go get github.com/databricks/databricks-sql-go@vX.Y.Z` transitively pins the | ||
| // matching per-platform kernel archive; upgrading the driver moves the kernel | ||
| // version. See docs/RELEASING.md and the bindings repo README. |
There was a problem hiding this comment.
🟠 High — These five require directives are unconditional entries in the module graph, not gated by the databricks_kernel build tag. Go's MVS build-list computation must read every required module's go.mod for any build in this module — including the default CGO_ENABLED=0 pure-Thrift go build ./... / go get. Only the archive zips are tag-gated (fetched lazily when a tagged file is compiled); the go.mod files of all five modules are always resolved.
The PR description states the default Thrift CUJ is "completely unchanged" and "fetches nothing kernel-related." That holds for the multi-megabyte .a zips, but not for module-graph resolution: a consumer's default build now must resolve these five modules. docs/RELEASING.md itself notes the bindings repo is private today and that public go get "requires it to be made public." Consequently, if the driver is tagged/released before the bindings repo is public, every external consumer's build breaks (even pure Thrift), and any CI without GOPRIVATE=github.com/databricks/* + git auth breaks too. Please confirm the driver release is hard-gated on the bindings repo being public, and that this doesn't regress the default consumer path in the interim.
| 2. In this **driver** repo: add a `require` for the new module in `go.mod`, add a | ||
| build-tagged `internal/backend/kernel/cgo_<os>_<arch>.go` that blank-imports it, | ||
| and drop the platform from the exclusion list in `cgo_unsupported.go`. | ||
|
|
There was a problem hiding this comment.
🟡 Medium — The only importers of the five bindings modules are the cgo_<os>[_<arch>].go shims, all gated behind //go:build ... databricks_kernel. go mod tidy does not enable custom build tags by default (it iterates GOOS/GOARCH but not arbitrary tags like databricks_kernel), so from tidy's perspective nothing imports these modules — a plain go mod tidy will prune the five require lines and their go.sum entries rather than refresh them. As written, step 6 would delete exactly the requirements it is meant to preserve. The invocation needs the tag (e.g. GOFLAGS=-tags=databricks_kernel ... or go mod tidy -tags databricks_kernel), or the requires must be pinned some other way that survives an untagged tidy.
…ules Switch the opt-in kernel/SEA backend from the local `make kernel-lib` source build to prebuilt per-platform archives delivered by the separate github.com/databricks/databricks-sql-kernel-bindings modules (pinned at v0.2.1), so a kernel `go get` build needs no Rust toolchain and no build step and pulls only the target platform's archive. Adds darwin_amd64 and linux_arm64 alongside darwin_arm64/linux_amd64/windows_amd64. - go.mod/go.sum: require the 5 bindings modules at v0.2.1; cgo_<os>[_<arch>].go blank-import the matching module so cgo collects its #cgo LDFLAGS at link time. - cgo.go: include the committed C header; commit include/databricks_kernel.h at the pinned KERNEL_REV (databricks-sql-kernel v0.2.1) so the driver compiles with no build step. - cgo_unsupported.go: legible compile error on unsupported GOOS/GOARCH. - README/RELEASING/.gitignore/Makefile: document the external-bindings model; drop the obsolete kernel-lib-download stub. Default pure-Go (CGO_ENABLED=0) build is unaffected — no bindings archive is downloaded without -tags databricks_kernel. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
d384936 to
5b4c90f
Compare
There was a problem hiding this comment.
Verdict: 1 Medium
Mostly a clean build/distribution + docs change (go-duckdb-style per-platform bindings). Build constraints across the six cgo shims are mutually exclusive and fully cover the excluded set in cgo_unsupported.go; header commit and .gitignore are consistent. One medium concern: the five requires pull the (currently private) bindings modules into the module graph, so even a default Thrift go build needs their go.mod metadata — the README's "Thrift path unchanged / fetches nothing" framing overstates this while the bindings repo is private.
| // matching per-platform kernel archive; upgrading the driver moves the kernel | ||
| // version. See docs/RELEASING.md and the bindings repo README. | ||
| require ( | ||
| github.com/databricks/databricks-sql-kernel-bindings/lib/darwin_amd64 v0.2.1 |
There was a problem hiding this comment.
🟡 Medium — These five modules are added as direct, unconditional requires in the main module's go.mod. Module-graph resolution (MVS) needs each required module's go.mod to compute the build list — that step runs for every go build, including a default Thrift (CGO_ENABLED=0, no tag) build. The build-tag gating only prevents downloading the multi-megabyte .a zips; it does not remove these modules from the require graph.
Consequently, while databricks-sql-kernel-bindings remains private (as docs/RELEASING.md states it is today), a public consumer running even a plain go get github.com/databricks/databricks-sql-go + go build will fail at module resolution when the proxy can't serve those go.mod files — not just "skip the archive." This contradicts the README's framing that the default Thrift CUJ is "completely unchanged" and "pulls no kernel binaries at all": binaries no, but module metadata for all five is still required.
RELEASING.md acknowledges the private-repo constraint, but the README (Cloning section) does not carry the same caveat for the Thrift path. Suggest either (a) gating the README's "works straight from go get" / "pulls nothing kernel-related" claims on the bindings repo being public, or (b) confirming the repo is public before this lands so the public consumer path actually resolves. Worth verifying a public go build (Thrift-only, no GOPRIVATE/auth) succeeds against the pinned v0.2.1.
1 participant
What
Delivers the kernel/SEA backend's native library as prebuilt, per-platform Go modules (the go-duckdb model), so a kernel opt-in
go getbuild links the right archive with no Rust toolchain and nomake kernel-libstep — while the default Thrift CUJ is completely unchanged.databricks/databricks-sql-kernel-bindingsrepo — one nested Go module per platform (lib/<os>_<arch>), each carrying that platform's prebuiltlibdatabricks_sql_kernel.a+ its#cgo LDFLAGS. Replaces the old in-treemake kernel-libdelivery.cgo_<os>[_<arch>].go) blank-import the matching bindings module; a build downloads only the archive for the platform it targets (verified — a darwin build never fetches the linux archive), and a pure-Go Thrift build fetches none.include/databricks_kernel.h) — required so the driver's cgo layer compiles without themake kernel-libstep.cgo_unsupported.goupdated).docs/RELEASING.md(per-platform path-prefixed tag versioning) + README "Cloning" (committed-binary +--filter=blob:none).CUJ
go get+go buildgo get+CGO_ENABLED=1 go build -tags databricks_kernelmake kernel-lib, no Rust)Verification
Runtime-verified end-to-end (real
SELECT 1+ full data-type suite through the kernel against a live warehouse) on all 5 platforms — darwin/arm64 (native), darwin/amd64 (Rosetta), linux/amd64 (native), linux/arm64 (qemu), windows/amd64 (native Windows) — built from the real bindings modules viago get(the true consumer path).Notes
go.mod(the bindings module versions), so it moves in lockstep with the driver release — seedocs/RELEASING.md.v0.1.0archives are built from the current kernel source; they get rebuilt from kernelmainon each kernel release per RELEASING.md.This pull request and its description were written by Isaac.