fix(kernel): Thrift-parity fixes + GetArrowBatches + decimal-as-float opt-in + U2M scopes/telemetry + SPOG warehouse-id routing - #416
Conversation
The kernel Rows path converted arrow timestamps with arrow's ToTime, which forms an int64-nanosecond intermediate (value * unit-multiplier). For a microsecond TIMESTAMP — Databricks' wire unit — that overflows int64 outside ~1678-2262 and silently wraps a valid instant to a wrong one (TIMESTAMP '0001-01-01' scanned back as 1754-08-30, '9999-12-31' as 1816-03-30). Convert via the unit-specific time.Unix/UnixMilli/UnixMicro constructors, which split into (seconds, nanoseconds) internally and represent the full 0001-9999 range Databricks TIMESTAMP allows. The default Thrift path is unaffected — it receives TIMESTAMP as a preformatted server string and never runs this multiply — so this closes a kernel-only divergence from Thrift. TestScanCellTimestampOutOfNanoRange pins both repro endpoints plus the int64-ns boundary years; the existing unit and location tests still pass. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The C ABI returns -1 for "not applicable / unknown" affected-row counts — DDL, SELECT, or a warehouse that doesn't surface the counter (the kernel's num_modified_rows Option<i64> None). The Thrift path reports 0 in those cases, so Result.RowsAffected() diverged: a CREATE/DROP SCHEMA returned -1 on the kernel backend and 0 on Thrift. Fold the -1 sentinel to 0 so RowsAffected() is identical across backends; a real DML count (>= 0) passes through unchanged. The rule lives in a pure, untagged normalizeAffectedRows so it is unit-testable in the default CGO_ENABLED=0 build alongside the other kernel decision helpers. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
838bb0c to
21ad6fa
Compare
…iver The kernel backend never forwarded the driver's User-Agent, so the kernel's built-in UA leaked through and query history misattributed SEA-path queries. Forward the same composed UA the Thrift path sends via set_custom_header, so both backends are attributed alike. Default-on; WithUserAgentEntry still customizes it. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The server stringifies VOID over the Thrift wire (verified live: even bare SELECT NULL reports STRING_TYPE), same as intervals, but hands the kernel a native arrow.NULL schema. Map arrow.NULL to STRING so both backends report identical column metadata; correct the two parity tests that wrongly pinned NULL_TYPE (derived from the enum name, never captured live). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
db597fd to
8815db9
Compare
mani-mathur-arch
changed the title
Address review of the parity fixes: assert kc.UserAgent == BuildUserAgent (non-empty) in TestBuildKernelConfig so deleting the forwarding or an empty UA fails a pure-Go run; add a CAST(NULL AS VOID) column to the live column-type parity query so the arrow.NULL→STRING mapping is guarded against the real server, not only the pure-Go tests. Trim the duplicated -1→0 rationale at the affected-rows call site to a one-liner. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
| // None). The Thrift path reports 0 in exactly those cases (TGetOperationStatusResp | ||
| // defaults NumModifiedRows to 0), so the -1 sentinel is folded to 0 for parity. | ||
| // A real DML count (>= 0) passes through unchanged. | ||
| func normalizeAffectedRows(n int64) int64 { |
There was a problem hiding this comment.
make this change in kernel not here
There was a problem hiding this comment.
changing this in the kernel will break the other drivers. other drivers have different specs for each "not applicable" row counts (python has -1, node has none). Go thrift has 0 for the database/sql spec.
| } | ||
|
|
||
| // User-Agent so query history attributes the kernel path to this driver. | ||
| if k.cfg.UserAgent != "" { |
There was a problem hiding this comment.
please verify from query history
There was a problem hiding this comment.
verified, the behavior is sound and matches how the python driver passes its user agent and what the query history shows
| "INTERVAL '1' MONTH a_iv_ym, st_point(1,2) a_geom" | ||
| // VOID: the server declares it STRING_TYPE over Thrift; asserts the kernel | ||
| // arrow.NULL→STRING mapping matches live, not just in the pure-Go tests. | ||
| "INTERVAL '1' MONTH a_iv_ym, st_point(1,2) a_geom, CAST(NULL AS VOID) a_void" |
There was a problem hiding this comment.
can you please confirm this NULL to string in other drivers too?
There was a problem hiding this comment.
The other drivers (node and python) also have this gap, the kernel returns nulls as void but thrift returns as string. i think this is an intentional design decision. verified by the python comparator, node one is currently broken due to dependency differences. I can remove it if that should be taken up as a separate fix for all the drivers or at the kernel level
msrathore-db
left a comment
msrathore-db
left a comment
There was a problem hiding this comment.
Please confirm the similar issues in other drivers over kernel like node and python so we can decide the location of these fixes
msrathore-db
left a comment
msrathore-db
left a comment
There was a problem hiding this comment.
Verify other drivers
kernelRows implemented only driver.Rows + the ColumnType* interfaces, so a Conn.Raw + rows.(dbsqlrows.Rows) assertion failed on the kernel backend (it succeeds on Thrift). Implement GetArrowBatches by wrapping the existing zero-copy next_batch pull, and reject GetArrowIPCStreams (the kernel C ABI exports Arrow C Data, not IPC bytes). A compile-time dbrows.Rows assertion guards the gap. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
m2m.Authenticate logged via the global zerolog logger, whose default level is Debug, so "OAuth token fetched successfully" printed regardless of the driver's configured log level (default Warn) on both backends. Route it through the driver's logger package so it honors WithLogLevel like every other line. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
changed the title
The U2M, OAuth-config, and token-provider paths still logged via the global zerolog logger (default level Debug), so their lines printed as raw JSON regardless of the driver's configured level — the same leak the m2m fix addressed. Route all of them through the driver's logger package so every auth log honors WithLogLevel and uses the driver's format; auth logic is untouched. This closes the last global-zerolog call sites in the driver. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The kernel U2M path forwarded only the client id, so the kernel applied its own default scope set (all-apis + offline_access). A workspace whose built-in databricks-sql-connector client isn't granted all-apis rejects that with access_denied. resolveKernelAuth now populates Auth.Scopes from oauth.GetScopes — the same function the Thrift path uses (offline_access + sql on AWS/GCP, offline_access + <tenant>/user_impersonation on Azure) — so both backends authorize identically. The setAuth -> set_auth_u2m wiring already forwarded Scopes; only the population was missing. No kernel/C-ABI change. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
changed the title
DAY-TIME intervals can arrive from the kernel as arrow.INTERVAL_DAY_TIME
(*array.DayTimeInterval), not just arrow.DURATION. ColumnTypeInfoFor lacked the
case, so dt_interval columns reported ""/*interface{} vs Thrift's STRING/string
(comparator diff across 5 configs); the value scanner also lacked it, so a real
DT-interval row would have errored. Add both — metadata → STRING and the scanner
folding {Days,Milliseconds} to ms through the same renderer — mirroring the
existing DURATION/INTERVAL_MONTHS handling. Kernel-path only; no other driver
and no kernel/C-ABI change.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Detach kernelRows.ctx from the caller's QueryContext (context2.WithoutCancel + Close-driven cancel), matching the Thrift path: a deadline that only gated statement submission no longer truncates a large CloudFetch stream. Seed iterationErr on batch-path fetch errors so a failed stream is recorded as a failed statement in telemetry, not a success. Add kernel-tagged coverage for the previously-untested batch API: white-box unit tests (IPC rejection, schema, drained-stream, sticky fetch-error → iterationErr) and a live GetArrowBatches draining e2e with an exact row count. Add a literal-assertion TestGetScopes (AWS/GCP/Azure) so a scope regression — the cause of the kernel U2M access_denied incident — is caught rather than asserted tautologically. Fix stale comments: the U2M doc now reflects that scopes are forwarded, m2m references SetLogLevel (not the nonexistent WithLogLevel), and the m2m success log moved after the error check. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
… flow On the kernel U2M path the kernel owns the interactive browser login, but the driver also wrapped the same authenticator into its telemetry/feature-flag HTTP client, which triggered a second, redundant browser prompt (and could block connect on its ~120s callback wait). Detect the interactive U2M authenticator via a structural interface (U2MClientID, unique to it) and skip telemetry init on the kernel+U2M path. Telemetry is best-effort, so it's dropped rather than made to prompt, mirroring the Python driver's default. Unauthenticated telemetry (Python/Node parity) is tracked in PECOBLR-3839. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
changed the title
On a unified/SPOG host the workspace id rides in the HTTP path's ?o=<org>, and the kernel injects x-databricks-org-id only when the path goes through set_http_path (kernel-side ConnectionConfig::from_http_path parses both the warehouse id and ?o=). set_warehouse takes only host+id and drops the org id, so a WithWarehouseID-addressed SPOG session 303s to /login. The kernel also refuses a caller-supplied x-databricks-org-id custom header, so the org id can reach the kernel only via the path. When a warehouse id is set but the HTTP path is a canonical warehouses/endpoints path carrying a non-empty ?o=, route by the path instead: the kernel still parses the same warehouse id out of it, plus the org id it needs. Guarded by httpPathCarriesOrgRouting so non-SPOG connections (no ?o=) keep the existing set_warehouse routing untouched. Go-only: Python and Node always use the http_path route, and the ODBC driver calls set_http_path with the raw path, so all three already parse ?o=. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
changed the title
mani-mathur-arch
changed the title
Adds a kernel-only connection option that scans top-level DECIMAL columns to a lossy float64 instead of the exact fixed-point string. The kernel still receives native Arrow Decimal128; only the Go scanner changes, skipping the per-cell string materialization for a cheap scalar. Off by default; mirrors the Thrift driver's pre-UseArrowNativeDecimal behavior. Nested decimals still render exactly. On decimal-heavy large results this is ~25% faster than the Thrift path and up to ~37% faster than the exact-string kernel path. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
changed the title
TestKernelExperimentalFieldsClassified reflects over every KernelExperimentalConfig field and requires an explicit disposition entry so no experimental knob is silently dropped. Register DecimalAsFloat (wired Go-side via kernel.Config -> kernelOp -> arrowscan). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
added
the
integration-test-kernel
|
Go integration tests triggered ( |
msrathore-db
left a comment
msrathore-db
left a comment
There was a problem hiding this comment.
Follow-up review of the latest push (9257e65). Verified the new WithKernelDecimalAsFloat option end-to-end on the kernel path against a live warehouse: default returns the exact string "99.98", the flag returns float64 99.98, and the Thrift-path reject gate correctly covers it. Everything builds and all unit + live kernel E2E tests pass. A few non-blocking items below (2 doc/invariant, 1 test-coverage, 1 carried over from the prior push).
| // connection through set_http_path so the kernel receives the SPOG ?o= org id. | ||
| // It must fire ONLY for a canonical warehouses/endpoints path with a non-empty | ||
| // o= value, so a reroute can never hand set_http_path a path from_http_path would | ||
| // reject. Runs under CGO_ENABLED=0 (pure Go, no kernel tag). |
There was a problem hiding this comment.
(P2, carried over from previous push) Inaccurate build-tag comment. This says "Runs under CGO_ENABLED=0 (pure Go, no kernel tag)", but the file carries //go:build cgo && databricks_kernel — I confirmed CGO_ENABLED=0 go test -run TestHTTPPathCarriesOrgRouting reports "no tests to run", so this predicate only runs under the kernel tag (nightly), not in default PR CI.
Since httpPathCarriesOrgRouting is pure string logic, consider moving it + this test out from behind the build tag — the way affectedrows.go is deliberately kept tag-free for pure-Go unit testing — so the SPOG-routing predicate runs in default PR CI. Otherwise, please just fix the comment.
There was a problem hiding this comment.
Fixed the comment in 0d33408 (it now correctly states the file is cgo && databricks_kernel-tagged, so the test runs under the kernel tag / nightly, not default PR CI). Kept it behind the tag rather than moving it out for this PR — happy to do the affectedrows.go-style extraction as a follow-up if you'd prefer the predicate in default CI.
| // backends indistinguishable). | ||
| // Match Thrift's sql.RawBytes scan type even though the kernel renders the | ||
| // value as an exact string — both convert cleanly to a caller's *string/*[]byte. | ||
| return ColumnTypeInfo{DatabaseTypeName: "DECIMAL", ScanType: scanTypeRawBytes} |
There was a problem hiding this comment.
(P3, doc) DECIMAL scan-type vs value in the new decimal-as-float mode. With WithKernelDecimalAsFloat(true), ScanCellCached now yields float64 for a top-level DECIMAL, while this arm still reports ScanType: sql.RawBytes. That mirrors the Thrift path (RawBytes scan type + float value under UseArrowNativeDecimal), so it's not a bug — but this package leans hard on the "reported scan type and scanned value stay in lockstep" contract. Worth a one-line note here that the opt-in decimal-as-float mode is a deliberate, Thrift-matching exception, so a future reader doesn't read it as drift.
There was a problem hiding this comment.
Done in 0d33408 — added a note here that RawBytes scan type + float64 value under decimal-as-float is a deliberate Thrift-matching exception (Thrift does the same under UseArrowNativeDecimal).
| // | ||
| // EXPERIMENTAL, kernel-only: the default (Thrift) backend rejects this at connect | ||
| // (use WithArrowNativeDecimal there instead). | ||
| func WithKernelDecimalAsFloat(asFloat bool) ConnOption { |
There was a problem hiding this comment.
(P2, stale invariant) This option contradicts a documented parity invariant. internal/rows/arrowbased/arrowscan_parity_test.go (TestTopLevelDecimalRendering) documents: "across every real driver configuration a top-level DECIMAL comes back as the exact string on both backends (verified live)." WithKernelDecimalAsFloat introduces exactly such a config where it does not — a top-level kernel DECIMAL comes back as float64 (I confirmed this live). Please update that comment to carve out this opt-in mode so the invariant no longer reads as violated. (That file isn't part of this PR, so leaving a note here since this is the change that breaks the claim.)
There was a problem hiding this comment.
Fixed in 0d33408 — updated the TestTopLevelDecimalRendering comment in arrowscan_parity_test.go to carve out WithKernelDecimalAsFloat (off by default, so the default-config invariant still holds), so it no longer reads as violated.
| // (use WithArrowNativeDecimal there instead). | ||
| func WithKernelDecimalAsFloat(asFloat bool) ConnOption { | ||
| return func(c *config.Config) { | ||
| kernelExperimental(c).DecimalAsFloat = asFloat |
There was a problem hiding this comment.
(P3, test gap) Add this option to the Thrift-reject test. TestWithKernelOptionsRejectedOnThriftPath in kernel_experimental_test.go enumerates each WithKernel* option (certs, skip-hostname, proxy, retry, max-chunks) but wasn't extended with WithKernelDecimalAsFloat. The generic KernelExperimental != nil gate covers it at runtime (verified), but every sibling option is pinned individually — add the case for consistency and to guard the reject path for this option too.
There was a problem hiding this comment.
Added in 0d33408 — WithKernelDecimalAsFloat is now a case in both TestWithKernelOptionsRejectedOnThriftPath and TestWithKernelTLSOptionsSetExperimental, matching the sibling options.
- Carve out the decimal-as-float opt-in in the top-level-DECIMAL parity invariant comment so it no longer reads as violated. - Note in coltype.go that RawBytes scan type + float64 value under decimal-as-float is a deliberate Thrift-matching exception. - Add WithKernelDecimalAsFloat to the Thrift-reject and set-experimental option tables for per-option coverage. - Fix the stale CGO_ENABLED=0 build-tag comment in spog_routing_test. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
github-actions
Bot
removed
the
integration-test-kernel
|
Integration test approval reset. New commits were pushed to this PR. Label(s) A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.) Latest commit: 0d33408 |
Co-authored-by: Isaac
![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 good — a large, well-tested parity/feature PR (timestamp overflow fix, decimal-as-float opt-in, GetArrowBatches, U2M scopes/telemetry, SPOG routing, auth-logger routing). Changes are cohesive, logger swaps verified against the driver logger package, and each behavior change carries a pinning test. One low-severity note on the new batch iterator's HasNext()/sticky-error semantics; nothing blocking.
peco-review-bot F1 (Low): kernelBatchIterator.HasNext() returned true forever after a non-EOF fetch error — err was sticky and done was never set, so a caller looping on HasNext() that logs-and-continues (instead of breaking) on error would spin forever, with Next() re-returning the same error and a nil record. Surface the error exactly once from Next(), then mark done so HasNext() goes false and a further Next() returns io.EOF. The failed stream is still recorded via r.iterationErr for OnClose telemetry. Updates the sibling test (which pinned the old sticky behavior) and adds a termination regression test. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
2 participants
What
SEA-via-kernel↔Thrift parity fixes surfaced by the Go comparator (two
database/sqlconnections against the same warehouse, differing only inuseKernel), plus the public Arrow batch API, U2M scope/browser fixes, an auth-logging cleanup, and SPOG routing. All kernel-path-only and entirely Go-side (no kernel/C-ABI change).Parity fixes
ToTimeforms anint64-ns intermediate that overflows for microsecond TIMESTAMPs outside ~1678–2262 ('0001-01-01'→1754-08-30). Now uses the unit-specifictime.Unix*constructors, covering 0001–9999. Thrift unaffected (gets a preformatted string).-1vs0. The C ABI returns-1for unknown/NA counts (DDL/SELECT); Thrift reports0. Fold the-1sentinel to0; real DML counts pass through.set_custom_header, matching Thrift;WithUserAgentEntrystill customizes.NULLvsSTRING. Server stringifies VOID over the wire (evenSELECT NULLreportsSTRING_TYPE), soarrow.NULLnow maps toSTRING, matching Thrift. Corrects two tests that pinnedNULL_TYPEfrom the enum name, not live.Arrow batch API
GetArrowBatchesnot exposed on kernel.kernelRowsnow implements it over the zero-copynext_batchpull (prefetch for exactHasNext; transfer record ownership to the caller).GetArrowIPCStreamsreturnsErrNotSupportedByKernel(kernel exports via C Data Interface, not IPC). Compile-timerows.Rowsassertion guards the gap.Decimal-as-float opt-in (
WithKernelDecimalAsFloat)float64(arrow-goNum.ToFloat64) instead of the exact fixed-point string. The kernel still receives native ArrowDecimal128over the wire — only the Go scanner changes, skipping the per-cell string materialization for a cheap scalar. Off by default (behavior byte-identical to today when unset); mirrors the Thrift driver's pre-UseArrowNativeDecimaldefault, and is rejected on the Thrift path (useWithArrowNativeDecimalthere). Nested decimals (inside list/struct/map) still render exactly.tpcds_sf100.catalog_sales, 15×DECIMAL(7,2), 1.3M–31M rows) the option is ~25% faster than Thrift (0.75–0.76×) and up to ~37% faster than the exact-string kernel path (0.63×), consistently across sizes and reps. Lossy beyond ~15–17 digits, so it's an opt-in speed knob, not a default.U2M
all-apis, rejected where the built-in client lacks it.resolveKernelAuthnow populatesAuth.Scopesfrom the sameoauth.GetScopesThrift uses.U2MClientIDstructural interface); best-effort telemetry is dropped rather than made to prompt, matching Python's default. Unauthenticated telemetry is the follow-up — PECOBLR-3839.Auth logging
zerolog/log(default Debug), ignoringWithLogLevelon both backends. Routed through the driverloggerso they honor the configured level; auth logic untouched.SPOG routing (
WithWarehouseIDpath)x-databricks-org-id, which it injects only viaset_http_path→from_http_path(parses the warehouse id +?o=). But Go callsset_warehouse(→ConnectionConfig::new, no org id) whenever a warehouse id is set, and the kernel refuses a caller-supplied org-id header — so aWithWarehouseIDSPOG session 303'd to/login. Now, when a warehouse id is set and the HTTP path is a canonicalwarehouses/endpointspath with a non-empty?o=, route viaset_http_path(kernel parses the same id plus the org id). Guarded byhttpPathCarriesOrgRouting, so non-SPOG connections keep the existingset_warehouserouting untouched. Go-only — Python/Node/ODBC always use thehttp_pathroute.Testing
TestScanCellTimestampOutOfNanoRange,TestNormalizeAffectedRows,TestColumnTypeInfoFor/…MatchesThriftMapping/…ScanTypeCoversScanner(VOID→STRING),TestScanCellDecimalAsFloat(float64 arm vs unchanged exact-string default), U2M scope assertions inTestValidateKernelConfig/TestResolveKernelAuthRealAuthenticators,TestInteractiveU2MAuthenticatorDetection,TestHTTPPathCarriesOrgRouting../auth/...green after the logger swap (no import cycle).GetArrowBatches— 100,000 rows / 32 zero-copy batches, schema + count pass;GetArrowIPCStreamsreturns the sentinel. U2M double-browser prompt confirmed gone. SPOG live onpeco.azuredatabricks.net— theWithWarehouseIDrouting that previously 303'd now returnsSELECT 1 == 1/current_user()with the org-id header injected; non-SPOG queries unchanged.WithKernelDecimalAsFloatverified live: default returnsDECIMAL/string, option-on returnsSTRING/float64for the same value, both99.98.CGO_ENABLED=0) and tagged cgo (-tags databricks_kernel).main's 41). The 5dt_interval_columnmetadata diffs are fixed here (INTERVAL_DAY_TIME→STRING). Of the 9 remaining, 8 are not driver bugs (NaN==NaN artifact ×3, non-deterministic EXPLAIN node ids, by-design volume-staging rejection, raw-fetch_arrowshape ×2, a permission-gated CREATE PROCEDUREsqlStatedetail); 1 is a deterministicDESCRIBE TABLEextra-trailing-rows diff that's server output (likely a comparator-sideskip_rowsfilter, not this repo). Native decimal/interval on the raw-fetch_arrowpath is a tradeoff, tracked as PECOBLR-3838 (needs a kernelKERNEL_REVbump).Stacked on
mani/sea-kernel-new-items(#412).Co-authored-by: Isaac