feat(kernel): SEA-via-kernel PuPr tail: advanced proxy, Geography, conn/error telemetry, token-caching docs, configurable backoff#412
Conversation
Fill the username / password / bypass_hosts args into the kernel's set_proxy setter (the kernel path passed nil,nil,nil, carrying only the URL). These are the "advanced" fields the HTTP(S)_PROXY / NO_PROXY environment path can't express: a structured bypass list (NO_PROXY is consumed during environment resolution, not forwarded) and out-of-band basic-auth credentials (rather than embedded in the URL userinfo). - New KernelExperimentalConfig proxy fields + WithKernelProxy(url, user, pass, bypassHosts) connector option (kernel-only; the Thrift path rejects it like the other WithKernel* options). - newKernelBackend prefers an explicit WithKernelProxy over the env-var resolution — an explicit proxy is a deliberate override, and consulting both would be ambiguous. resolveKernelProxy is untagged so the explicit-over-env precedence is covered under CGO_ENABLED=0. - Kernel backend Config gains ProxyUsername / ProxyPassword / ProxyBypassHosts; applyProxy passes each as NULL when empty. Tests: experimental-field classification guard + option-wiring + Thrift-reject + DeepCopy extended for the proxy fields (untagged); resolveKernelProxy precedence (untagged); TestSetProxy drives the real cgo setter across all field combinations (tagged); live e2e TestKernelE2EProxyRejectsBadURL (routing through an unreachable proxy must fail — proof the setter is applied). Both build paths + full suite green; go vet clean. Isaac Review clean (0 valid findings). Co-authored-by: Isaac <isaac@databricks.com> Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
#393 shipped Geometry (WKT string); Geography is the sibling geospatial type left out (split as separate subtasks). No renderer is needed: the kernel maps GEOMETRY and GEOGRAPHY to the same Arrow Utf8 shape (WKT text), distinguished only by a databricks.type_name field-metadata hint that does not change the scanned value — both hit the same string arm on the kernel and Thrift paths. So this is verify-and-close: prove the parity and pin it, no source change. - arrowscan parity test gains a geography_wkt case (kernel == Thrift == "POINT(1 2)"), documenting the shared rendering. - Live e2e TestKernelE2EDataTypes gains a geography case that SKIPs (not fails) when the warehouse rejects the type — GEOGRAPHY is gated on some warehouses, and the case documents the parity without breaking runs where it is unavailable. - doc.go lists GEOMETRY / GEOGRAPHY (WKT) in the result-type parity set. Isaac Review clean (0 actionable findings). Co-authored-by: Isaac <isaac@databricks.com> Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…d error drop Emit connection-configuration telemetry on the kernel path (PECOBLR-3627) and stop silently dropping connection-scoped errors (PECOBLR-3628). Neither backend populated DriverConnectionParameters before: the struct existed but was never filled, and recordConnection was dead code. This is a net-new emission on top of the #399 per-statement telemetry (which already emits EXECUTE_STATEMENT / CLOSE_STATEMENT + chunk details and per-statement errors — not duplicated here). - telemetry: telemetryMetric gains connParams; createTelemetryRequest serializes it (and mirrors AuthMech onto the top-level AuthType) only for a "connection" metric — a statement/operation/error metric leaves it nil and serializes byte-identically to before (pinned by a test). Interceptor.RecordConnectionConfig replaces the dead recordConnection. - aggregator: a non-terminal error with no statement to attach to (a connection-scoped error, empty statementID) was silently dropped. It now flushes immediately instead — the connection-scoped error-log path (3628). Both backends benefit. - driver: the connector emits RecordConnectionConfig at connect, KERNEL PATH ONLY (gated on the backend type, not just WithUseKernel), so the default (Thrift) path's telemetry is byte-identical. The payload is built by untagged kernelConnectionTelemetry(cfg): mode=SEA (the closed DatabricksClientType enum has no "kernel" member — emitting it NULLs the field on ingestion), auth_mech ∈ {PAT, OAUTH} + auth_flow ∈ {CLIENT_CREDENTIALS, BROWSER_BASED_AUTHENTICATION} (the closed enums, not the driver's own auth names), proxy usage, arrow, query tags, metric-view — mirroring Python's TelemetryHelper enum mapping so all drivers land the same values. Tests: untagged builder test; serializer + "statement metric unchanged" + full-loop (RecordConnectionConfig lands; connection-scoped error not dropped) tests. Both build paths + full suite green; go vet clean. Isaac Review clean (0 actionable findings). Co-authored-by: Isaac <isaac@databricks.com> Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…e kernel PECOBLR-3606 (token caching) and PECOBLR-3626 (client tokens / connection reuse) are verify-and-close: both are handled entirely inside the kernel, below the C ABI, so there is no driver work and no driver-side knob. Verified against kernel source: OAuth U2M tokens persist to disk (~/.config/databricks-sql-kernel/oauth/, with refresh-token lifecycle), M2M tokens cache in-memory with background refresh, and a single pooled reqwest client is reused per session across the control-plane, CloudFetch, and auth-refresh calls (pool_max_idle_per_host). doc.go now states this so the inherited behavior is documented rather than silently assumed. Isaac Review clean (0 findings). Co-authored-by: Isaac <isaac@databricks.com> Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Honor the driver's WithRetries policy (RetryWaitMin / RetryWaitMax / RetryMax) on the kernel path by forwarding it to the kernel's new kernel_session_config_set_retry_config C-ABI setter (PECOBLR-3598). Before, WithRetries was silently ignored on the kernel path and the disable form (RetryMax < 0) was rejected at connect — the kernel had exponential backoff with configurable HttpConfig::retry_* fields but no C-ABI setter, so the caller's policy could not reach it. - kernel backend Config gains Retry *RetryConfig; applyRetry forwards it via the setter (a no-op when nil, so the kernel's default policy — 5 retries, 1s..60s, 900s budget — is preserved). - Untagged kernelRetryConfig resolves WithRetries into the descriptor: the connector's WithDefaults guarantees positive waits + RetryMax 4; a negative RetryMax (the disable form) maps to MaxRetries=0 and is honored EVEN with zero waits (WithRetries(-1,0,0) is idiomatic) by substituting a valid placeholder range the setter accepts (backoff unused with no retries). A non-disabling degenerate range returns nil (keep kernel default) so a stray zero can't fail the connect. - validateKernelConfig no longer rejects WithRetries(-1). - WithKernelRetryOverallTimeout adds the 4th knob (cumulative retry budget) — kernel-only, since the Thrift WithRetries surface has no overall-budget equivalent; mirrors the pyo3/napi retry_overall_timeout. - KERNEL_REV bumped to the kernel PR head that adds the setter (b5f6dda); re-pin to the squash-merge SHA once that kernel PR lands. cgo note: cgo silently drops the direct declaration of the retry setter (a parser quirk — valid C, compiles + links, but omitted from the Go bindings). A static inline shim in backend.go's preamble forwards to it; the shim must be in the same file as its caller (a shim in another cgo preamble in this package is dropped the same way). The kernel header is unchanged (valid C, used verbatim by the C-only ODBC consumer). Tests: untagged TestKernelRetryConfig (defaults / disable incl. zero waits / overall timeout / degenerate range); field-classification guards updated (RetryMax/Wait* now forwarded; RetryOverallTimeout classified); tagged TestSetRetry drives the real cgo setter incl. the InvalidArgument rejections; live e2e retry config / disable / overall-timeout. Both build paths + full suite green; go vet + gofmt clean. Isaac Review clean. Co-authored-by: Isaac <isaac@databricks.com> Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
golangci-lint's gosec G101 ("potential hardcoded credentials") flagged
three non-credential string literals: the ProxyPassword test literals in
TestResolveKernelProxy / TestSetProxy, and the CLIENT_CREDENTIALS
telemetry auth_flow enum value. None is a real secret. Suppress each with
//nolint:gosec + a reason (the repo's established pattern), clearing the
Lint CI gate. gosec anchors the struct-literal G101 to the composite-lit
line, so the nolint sits there, not on the field.
Co-authored-by: Isaac <isaac@databricks.com>
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
kernelRows implemented only driver.Rows, so database/sql fell back to ""
DatabaseTypeName and an interface{} ScanType for every column on the
kernel backend — whereas the Thrift path returns BIGINT/DECIMAL/DATE
names and int64/float64/time.Time/sql.RawBytes scan types (PECOBLR-3692).
ORMs and typed-scan tooling (GORM, sqlx, schema reflection, BI drivers)
rely on that metadata to pick Go destination types, so the omission was a
silent behavioral regression vs Thrift that value-level tests can't see.
- New shared, pure-Go arrowscan.ColumnTypeInfoFor(arrow.DataType) maps an
Arrow column type to {DatabaseTypeName, ScanType, Length} matching the
Thrift backend exactly (getScanType / GetDBTypeName / ColumnTypeLength).
It lives next to ScanCellCached and covers precisely the Arrow types the
scanner produces, so the reported type and the scanned value stay in
lockstep. Ground truth for the mapping was captured live from the Thrift
backend across all 21 supported types.
- kernelRows now implements RowsColumnTypeScanType /
DatabaseTypeName / Nullable / Length, computing per-column info once at
construction from the schema it already imports for Columns(). Nullable
returns ok=false like Thrift (no reliable per-column flag); Length
reports MaxInt64 for variable-length types and (0,false) otherwise.
- DECIMAL reports sql.RawBytes (Thrift's DECIMAL scan type) though the
value renders as an exact string; VARCHAR/CHAR/VARIANT/GEOMETRY and both
INTERVAL types collapse to STRING, matching what the prod-default Thrift
server declares (documented inline, incl. the native-interval caveat).
Why this wasn't caught: the kernel-vs-Thrift parity suites compare
scanned VALUES via sql.RawBytes, which is structurally blind to the
Rows.ColumnType* metadata (it was a missing-interface gap, not a wrong
value). This adds the guards that would have caught it: pure-Go
TestColumnTypeInfoFor / ...CoversScanner (run in the default CGO_ENABLED=0
CI build, no warehouse) and the live TestKernelThriftColumnTypeParity,
which compares full sql.ColumnType metadata across both backends for every
type. Verified: neutering the mapper makes the live test fail (empty
name / nil scan type), confirming it's a real regression guard.
Both build paths + full suite green; go vet + gofmt + golangci-lint clean.
Live kernel parity/datatype suites pass. Isaac Review clean.
Co-authored-by: Isaac <isaac@databricks.com>
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…nel) decimalfmt.ExactString rendered every DECIMAL cell via big.Int.String() plus ~4 heap allocations, and it is on the shared render path both backends use (the Thrift arrow path's ValueString and the kernel scan path). On decimal-heavy result sets that per-cell allocation churn was a measurable throughput/RSS cost on the kernel backend vs Thrift, whose server-pre-formatted decimals arrive as zero-copy Arrow STRING (PECOBLR-3691). Rewrite the renderer to allocate only the returned string: - render the unscaled magnitude right-to-left into a stack scratch and place the decimal point by slice copy, with math/big only as the fallback for a true 128-bit magnitude (hi != 0) — which every DECIMAL(<=18), and every DECIMAL value under 2^64, never reaches. - factor an exported Append(dst, n, scale) so a caller can render into a reused buffer with zero heap allocation (byte-identical to ExactString). Output is unchanged: this is a cost-only rewrite of a value on the prod Thrift path, so it must be byte-for-byte identical for every input. The guard is TestExactStringOracleParity, which keeps the pre-rewrite implementation verbatim as an oracle and fuzzes the two against each other across both signs, the full scale range, the uint64/128-bit boundary, and the DECIMAL(38) / 2^127-1 extremes. Micro-bench: 38.9ns/18B/1alloc -> ~12ns/0B/0alloc; Append into a reused buffer is ~9ns/0alloc. The kernel scan path continues to render top-level decimals through this shared renderer (one string per cell, now alloc-cheaper); the further zero-alloc batch-arena optimization is deliberately deferred to a follow-up so this first kernel release carries no unsafe. Also adds the live TestKernelThriftDecimalScaleParity (50k rows spanning multiple kernel batches, incl. a DECIMAL(38,4) past float64 range) to pin the integrated kernel scan path byte-identical to Thrift at scale. Both build paths + full suite green; go vet + gofmt + golangci-lint clean. Live kernel parity/datatype suites pass. Isaac Review clean. Co-authored-by: Isaac <isaac@databricks.com> Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…sInMemory Give callers a way to bound the kernel's peak RSS on large result sets (PECOBLR-3691, the RSS half of the decimal-heavy launch risk). The kernel holds cloudfetch_max_chunks_in_memory (default 16) decompressed CloudFetch chunks at once; on wide, row-heavy results that drives ~1.2 GB peak, vs Thrift's flat ~+200 MB. Lowering the bound trades a little throughput for a large RSS reduction (measured: chunks=4 -> ~265 MB vs chunks=32 -> ~856 MB peak on a 3M-row query, a 3.2x swing driven purely by the knob). - New EXPERIMENTAL kernel-only option WithKernelMaxChunksInMemory(n), carried on KernelExperimentalConfig.MaxChunksInMemory. A value <= 0 (the default) leaves the kernel's built-in default (16) untouched. - buildKernelConfig injects it into the KERNEL backend's own SessionConf as the client-only "cloudfetch_max_chunks_in_memory" key (config.KernelMaxChunksInMemoryConfKey) — NOT via EffectiveSessionParams, which both backends share, so it can never leak onto the Thrift path or the server. The kernel (PR that pairs with this KERNEL_REV bump) reads the key in Session::open, folds it into its result config, and strips it before the SEA wire; the key is absent from the kernel's server allowlist so it never becomes a server SET param. - No new C-ABI symbol: it rides the existing kernel_session_config_set_session_conf setter. - KERNEL_REV bumped to the kernel head that adds the override reader; re-pin to the squash-merge SHA once that kernel PR lands. Tests: the experimental-field classification guard gains the new field (so it can't be silently dropped); option->config wiring + Thrift-reject + DeepCopy cases; buildKernelConfig subtests assert the key is injected when set, absent when unset/zero, and NOT present in EffectiveSessionParams (the kernel-only invariant); live TestKernelE2EMaxChunksInMemory drains a 1M-row CloudFetch result with the bound lowered (proves the key is accepted by the kernel and stripped before the wire — a bad key would error server-side). Both build paths + full suite green; go vet + gofmt + golangci-lint clean. Isaac Review clean. Co-authored-by: Isaac <isaac@databricks.com> Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Code Review Squad — ReviewScore: 85/100 — MODERATE RISK Large additive PR closing out the SEA-via-kernel new-items backlog (~8 features). The high-risk internals are solid: the alloc-free decimal renderer is correct at the uint64↔128-bit boundary (benchmarked 12.4ns/0-alloc), carries no injection surface, and proxy credentials/tokens never reach logs or telemetry — no Critical and no security findings. Remaining findings cluster around one real correctness gap (WithRetries silently ignored on a user-reachable input) plus advertised-but-thin items (a stale rejection message, a telemetry fix targeting a currently-dead path) and documentation/test-coverage polish. A separately-tracked KERNEL_REV re-pin (acknowledged by the author, to be fixed before finalizing) was dismissed from this review. All findings verified against head SHA 1cff39c. Medium Findings[Medium] Thrift-path rejection message names a stale 2-option subset — (Posted here rather than inline: the cited lines are pre-existing code not present in this PR's diff, so GitHub can't anchor an inline comment there.) The rejection error hardcodes Suggested fixGeneralize the message to "a WithKernel* option" (drop the specific list) or enumerate all five current options; update the guarding comment (connector.go:48-52) to drop "TLS." Posted by code-review-squad · |
…, telemetry framing, coltype parity Address the 9 findings from the PR #412 code review (Isaac Review clean on the result). One real correctness fix (WithRetries silently ignored on a user-reachable input) plus advertised-but-thin items and doc/test polish. - kernel_config: WithRetries(n, 0, 0) now forwards the caller's RetryMax on the kernel path instead of dropping to the kernel's default policy. Because WithDefaults() runs before options, WithRetries(n, 0, 0) — valid per its godoc — overwrote the waits to zero and hit the degenerate-range guard, so the caller's attempt count was silently ignored (a cross-backend divergence: the Thrift path honored the same input). It now substitutes placeholder waits while keeping RetryMax; only a zero-value range with no attempt count falls back to the kernel default. New WithRetries(n,0,0) test; the disable constants are renamed kernelRetryPlaceholderWait* since both paths use them. - connector: the Thrift-path rejection message named only the two TLS options, but all five WithKernel* options trip the same gate — a caller who set WithKernelProxy and forgot WithUseKernel was misdirected. Generalized to name the WithKernel* family; the test now guards against a stale subset. - telemetry/aggregator: reframed the "error" metric branch (and its test) as defensive — no exported API emits a standalone "error" metric today, so the branch is unreachable from production and guards routing for a future producer, rather than being an active connection-scoped-error fix. - arrowscan/coltype: added UINT8/16/32/64 -> BIGINT/int64 (matching ScanCellCached's int64 widening), so the scanner-coverage guard no longer silently omits them; fixed a doc comment that named a nonexistent guard test. - internal/rows: new pure-Go TestColumnTypeInfoMatchesThriftMapping cross-checks ColumnTypeInfoFor against the Thrift getScanType / GetDBTypeName / length logic, so a Thrift-mapping drift (e.g. DECIMAL's scan type) fails in CGO_ENABLED=0 PR CI instead of only the warehouse-gated nightly parity test. - doc.go: added WithKernelMaxChunksInMemory to the experimental-options catalog. - kernel_newitems_e2e: new TestKernelE2EProxyForwardsCredentials routes through a local CONNECT proxy and asserts the captured Proxy-Authorization decodes to exactly user:pass (a slot swap would show pass:user) — the credential/bypass forwarding TestSetProxy couldn't cover against the opaque kernel C config. - dropped history-reference comments that described pre-setter behavior. Both build paths green (CGO_ENABLED=0 go test ./... + CGO_ENABLED=1 go test -tags databricks_kernel ./...), go vet clean both paths, gofmt clean, golangci-lint 0 issues on the changed files. Co-authored-by: Isaac <isaac@databricks.com> Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
changed the title
|
Fixed in 29edbba (the last remaining Medium — the stale rejection message at All five The guarding comment was de-scoped from "TLS" to cover the trusted-CA bundle, hostname-verify skip, proxy, retry budget, and CloudFetch chunk cap. With this, all 9 findings from the review are addressed; a follow-up |
Point KERNEL_REV at f299577, the current head of kernel PR #178 (configurable HTTP retry / backoff policy). The prior pin (ffa73c0) is a direct ancestor — f299577 just adds the retry→HttpConfig merge test and cloudfetch chunk clamp on top. All C-ABI symbols this backend calls, including set_retry_config and set_proxy, are present in the header at the new rev. 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 ( |
| minWait, maxWait := cfg.RetryWaitMin, cfg.RetryWaitMax | ||
| if minWait <= 0 || maxWait < minWait { | ||
| if cfg.RetryMax <= 0 { | ||
| return nil |
There was a problem hiding this comment.
[Medium] WithKernelRetryOverallTimeout is silently discarded when retries are zeroed — reproduced
overall is computed up-front from KernelExperimental.RetryOverallTimeout, but this degenerate-range branch return nil whenever RetryMax <= 0 and the waits are non-positive — throwing the caller's explicit overall budget away. The kernel then keeps its 900s default. This is the same "silently ignored option" failure the WithKernel* rejection gate was built to prevent, but here on an accepted option.
Reproduced under CGO_ENABLED=0 against this PR head:
WithRetries(0,0,0) + WithKernelRetryOverallTimeout(5*time.Minute)
→ kernelRetryConfig returns nil → the 5-minute budget never reaches set_retry_config.
Fix
in the RetryMax <= 0 degenerate case, when overall > 0 still return a &kernel.RetryConfig{MinWait: placeholder, MaxWait: placeholder, MaxRetries: 0, OverallTimeout: overall} instead of nil, so the budget is forwarded even with zero retries.
/full-review · feedback: #code-review-squad-feedback
There was a problem hiding this comment.
Fixed in bd84ae8.
The problem: with WithRetries(0,0,0) the retry count is 0 and the waits are zeroed, so we returned nil and your WithKernelRetryOverallTimeout was thrown away the kernel just kept its 900s default.
Now, if you set an overall timeout, we still send it to the kernel (with placeholder waits and 0 retries) instead of returning nil. We only fall back to the kernel default when there's genuinely nothing to keep no retry count and no timeout. Added a regression test.
| // Via the go_kernel_set_retry_config shim in cgo.go — cgo drops the direct | ||
| // declaration of the underlying symbol (see the shim's comment). | ||
| return C.go_kernel_set_retry_config(cfg, | ||
| C.uint64_t(r.MinWait.Milliseconds()), C.uint64_t(r.MaxWait.Milliseconds()), |
There was a problem hiding this comment.
[Medium] Sub-millisecond RetryWaitMin passes the guard, truncates to 0ms, and the kernel rejects the connect — reproduced
kernelRetryConfig validates the wait range in time.Duration space (minWait <= 0), but applyRetry forwards r.MinWait.Milliseconds(). Any 0 < MinWait < 1ms passes the guard yet becomes min_wait_ms == 0, which the kernel setter rejects as InvalidArgument (src/c_abi/session.rs:372 on the pinned rev, "retry min_wait_ms must be > 0"). Same config connects fine on the Thrift path (go-retryablehttp accepts any Duration) — a cross-backend divergence contradicting the "WithRetries is honored on the kernel path" doc claim.
Reproduced: WithRetries(5, 999*time.Microsecond, 30*time.Second) → min_wait_ms=0 → kernel Connect fails.
Fix
clamp MinWait/MaxWait up to at least 1ms before forwarding (or perform the guard in millisecond space), so a valid sub-ms Duration can't degrade to a rejected 0.
/full-review · feedback: #code-review-squad-feedback
There was a problem hiding this comment.
Fixed in bd84ae8.
We checked the wait range in time.Duration terms but hand the kernel milliseconds, so a wait under 1ms (like 999µs) became 0ms and the kernel rejected the connect even though the Thrift path is happy with it.
Now we clamp MinWait/MaxWait up to a 1ms floor before forwarding, so a sub-ms wait can't degrade to a rejected 0. Added a regression test.
| @@ -1 +1 @@ | |||
| 4d301455f7e70de9cb747e0f1143b8a3df8c5b48 | |||
| f29957733c8f6d79a7212bc8a50cf4e5ffa8dd69 | |||
There was a problem hiding this comment.
[Medium] KERNEL_REV pins an unmerged PR-branch head → non-reproducible / breakable CI
f299577… is the head of kernel PR #178 (OPEN). The required symbols (set_retry_config, apply_client_result_overrides) exist only on that branch, not kernel main (verified). A PR head is a mutable ref: a squash-merge / rebase / force-push moves it and GitHub GCs the orphaned commit, after which the kernel-lib fetch of refs/pull/*/head can no longer reach this SHA and every cold (cache-miss) CI build goes red with no source change here.
The PR body already commits to re-pinning to the squash-merge SHA once #178 lands — this should block merge, and ideally #178 merges first so the pin points at a main commit.
Posted by code-review-squad · /full-review · feedback: #code-review-squad-feedback
There was a problem hiding this comment.
Yes this is already called out in the pr description. This will be taken care of while merging.
| // server or the Thrift path. Zero/negative keeps the kernel default (16). | ||
| if ke.MaxChunksInMemory > 0 { | ||
| // EffectiveSessionParams returned a fresh map, so mutating it is safe. | ||
| kc.SessionConf[config.KernelMaxChunksInMemoryConfKey] = strconv.Itoa(ke.MaxChunksInMemory) |
There was a problem hiding this comment.
[Medium] MaxChunksInMemory rides a stringly-typed session-conf key coupled to an unmerged ABI, with no build/unit guard
The knob is injected as the magic string cloudfetch_max_chunks_in_memory into the kernel SessionConf; correctness depends entirely on the kernel's apply_client_result_overrides recognizing and stripping it before the SEA wire. I verified the kernel does this correctly on the pinned rev (strips before filter_session_conf, clamps to a ceiling of 256, ignores invalid/zero) — so it works today. But the sibling retry knob got a typed C setter with compile-time status assertions, whereas this one is guarded only by the nightly warehouse test. If the merged kernel renames the client key, the value silently no-ops or leaks to the server, and no CGO=0 / build-time check catches it.
Alternative design
give it a dedicated typed C setter (mirroring set_retry_config) so a key/signature drift is a link/compile error, or add a kernel-side ABI accessor the driver can assert the key against at build time.
/full-review · feedback: #code-review-squad-feedback
There was a problem hiding this comment.
Acknowledged, kept the string-key design (a typed C setter needs a new kernel ABI symbol, which is heavier than this knob warrants), but closed the "no cheap guard" gap in bd84ae8.
Added a pure-Go test that pins the exact key literal cloudfetch_max_chunks_in_memory. So an accidental change to the Go constant now fails in CGO_ENABLED=0 PR CI (not just the nightly warehouse test), and the test is the greppable anchor a kernel-side rename has to update in lockstep.
|
|
||
| // kernelAuthMech maps each resolvable auth form to the closed AuthMech/AuthFlow | ||
| // enums. M2M and U2M are both OAUTH but differ in flow; PAT is PAT with no flow. | ||
| func TestKernelAuthMech(t *testing.T) { |
There was a problem hiding this comment.
[Medium] Telemetry auth-mech OAUTH/M2M/U2M arms untested; proxy slot-correctness proven only in the nightly warehouse test
kernelAuthMech maps M2M→(OAUTH, CLIENT_CREDENTIALS) and U2M→(OAUTH, BROWSER_BASED_AUTHENTICATION), but TestKernelAuthMech exercises only the PAT/default arm. The OAUTH mech and both flow strings — the values that silently NULL server-side if wrong — are never asserted, and both arms are pure-Go testable via a config that resolves to M2M/U2M auth. Separately, proxy credential slot-ordering is proven only by the cgo+warehouse TestKernelE2EProxyForwardsCredentials (verified genuine, not hollow), so it is unguarded in CGO_ENABLED=0 PR CI.
Suggested fix
add TestKernelAuthMech subtests for M2M and U2M configs asserting the exact (mech, flow) pairs; if a cgo-without-warehouse CI lane exists, run the proxy-forwarding test there (the local CONNECT-proxy capture needs no real warehouse).
/full-review · feedback: #code-review-squad-feedback
There was a problem hiding this comment.
Fixed in bd84ae8.
Added M2M and U2M subtests to TestKernelAuthMech, asserting the exact pairs, M2M → (OAUTH, CLIENT_CREDENTIALS), U2M → (OAUTH, BROWSER_BASED_AUTHENTICATION). Both run pure-Go (they reuse the existing fake authenticators, since the real u2m.NewAuthenticator does live OIDC discovery at construction). So the enum values that NULL server-side if wrong are now guarded.
| // thriftLength mirrors rows.ColumnTypeLength's variable-length classification | ||
| // (the method needs a *rows with a live client, so replicate its switch here on | ||
| // the same TTypeId set — the exact list the method checks). | ||
| thriftLength := func(id cli_service.TTypeId) (int64, bool) { |
There was a problem hiding this comment.
[Low] The Length dimension of the coltype drift-guard re-copies the Thrift rule instead of calling it (flagged independently by test + maintainability reviewers)
TestColumnTypeInfoMatchesThriftMapping genuinely cross-checks ScanType and DatabaseTypeName by calling the real Thrift getScanType / rowscanner.GetDBTypeName, but Length is compared against thriftLength, a hand-copied replica of rows.ColumnTypeLength's type switch. So for the Length dimension both sides can drift together and stay green — the exact silent-divergence class PECOBLR-3692's guard was added to prevent. There are now three copies of the variable-length rule.
Suggested fix
drive the length expectation from the real classifier (rowscanner.GetDBTypeID + the shared var-length set) rather than a copied switch, so a Thrift-side change fails this test. Interval types (DURATION / INTERVAL_MONTHS) are also absent from the cross-check cases.
/full-review · feedback: #code-review-squad-feedback
There was a problem hiding this comment.
Fixed in bd84ae8.
You're right, the Length side was comparing against a hand-copied thriftLength switch, so both copies could drift together and stay green (the third copy of that rule).
I pulled the rule out of ColumnTypeLength into one shared helper, columnTypeLengthForID, and now both the method and this test call it, no replica. A change to the Thrift-side length rule now fails this test. Also added the interval types (DURATION / INTERVAL_MONTHS) to the cross-check cases.
| // both would be ambiguous, and an explicit proxy is a deliberate override. | ||
| // | ||
| // EXPERIMENTAL, kernel-only: the default (Thrift) backend rejects this at connect. | ||
| func WithKernelProxy(url, username, password, bypassHosts string) ConnOption { |
There was a problem hiding this comment.
[Low] WithKernelProxy(url, username, password, bypassHosts) is four adjacent same-typed string params — swap-prone
Four consecutive string parameters give a caller (human or codegen/agent) nothing but argument order to get right; a username/password swap or a misplaced bypassHosts compiles cleanly and fails only at runtime with wrong proxy credentials. The authors' own TestKernelE2EProxyForwardsCredentials explicitly guards against a "slot swap," confirming the footgun is real.
Suggested fix
accept an options struct (e.g. WithKernelProxy(KernelProxy{URL, Username, Password, BypassHosts})) or split into single-purpose options so each field is named at the call site; the C-ABI mapping stays internal.
/full-review · feedback: #code-review-squad-feedback
There was a problem hiding this comment.
Fixed in bd84ae8.
Agreed, four adjacent strings is a footgun. Changed the signature to a named struct: WithKernelProxy(KernelProxy{URL, Username, Password, BypassHosts}), so a username/password swap can't compile. Updated all call sites and the doc.go catalog. The C-ABI mapping stays internal.
| // reused buffer allocates nothing (the digit scratch stays on the stack). Run: | ||
| // | ||
| // go test -run '^$' -bench 'ExactString|AppendReused' -benchmem ./internal/decimalfmt/ | ||
| func BenchmarkExactString(b *testing.B) { |
There was a problem hiding this comment.
[Low] The "0 alloc" ExactString claim is a benchmark artifact; production still allocates 1 string/cell
BenchmarkExactString does _ = ExactString(n, 2), so the compiler proves the returned string doesn't escape and elides the heap allocation (0 B/0 allocs). Every production caller keeps the result, so the string escapes — measured ~14ns/5B/1 alloc in the escaping case vs the benchmark's 8.1ns/0B/0 allocs. Still a genuine win over the old 38.9ns/18B/1 alloc, but not alloc-free on the render path. The truly zero-alloc Append variant has no production callers.
Suggested fix
add a sink-assigned benchmark (var sink string; sink = ExactString(...)) to report the real 1-alloc production profile, and/or wire the arrowbased/kernel render loops to Append into a reused batch-scoped buffer to actually capture the 0-alloc path.
/full-review · feedback: #code-review-squad-feedback
There was a problem hiding this comment.
Fixed in bd84ae8.
Right, the benchmark's _ = ExactString(...) lets the compiler prove the string never escapes, so it reports 0 allocs; real callers keep the result and pay 1 alloc.
Added BenchmarkExactStringSink that assigns to a package-level var so the string escapes, reporting the true ~1-alloc/5B production profile alongside the escape-elided one, and clarified the comment to read the two together. (Still a solid win over the old 38.9ns/18B renderer; the truly 0-alloc Append form has no production caller yet, as noted.)
| // TestKernelE2ERetryConfig proves a tuned WithRetries policy (backoff bounds + max | ||
| // attempts) reaches the kernel's HTTP retry config and the connection still works: | ||
| // the setter accepts the range and a normal query succeeds. | ||
| func TestKernelE2ERetryConfig(t *testing.T) { |
There was a problem hiding this comment.
[Low] Retry / overall-timeout e2e tests are smoke tests that pass whether or not the policy is applied
TestKernelE2ERetryConfig / TestKernelE2ERetryOverallTimeout assert only that SELECT 1 returns 1. A silently-dropped or ignored retry/overall-timeout config produces the identical result, so the docstring claim "reaches the kernel's HTTP retry config" is unproven — the test can't distinguish applied-vs-ignored (near-zero signal over connect-succeeds).
Suggested fix
soften the docstrings to "connect succeeds with the option set," or add fault injection (unreachable-then-recover) to observe retry behavior. TestKernelE2ERetryDisabled is fine — it guards the validateKernelConfig acceptance change.
/full-review · feedback: #code-review-squad-feedback
There was a problem hiding this comment.
Fixed in bd84ae8, per your suggestion.
Softened both docstrings to "connect succeeds with the option set", a happy-path SELECT 1 can't tell applied-from-ignored, so I stopped claiming otherwise. The pure-Go TestKernelRetryConfig is what actually pins the WithRetries → kernel.RetryConfig mapping. Left TestKernelE2ERetryDisabled as-is since it genuinely guards the validateKernelConfig acceptance change.
| // untagged here (like buildKernelConfig) so the explicit-over-env precedence is | ||
| // asserted under CGO_ENABLED=0 (see TestResolveKernelProxy); newKernelBackend | ||
| // calls it after buildKernelConfig so it stays a thin assembler. | ||
| func resolveKernelProxy(cfg *config.Config, kc *kernel.Config) { |
There was a problem hiding this comment.
[Low] Config-validation failures surface as opaque wrapped kernel strings, not errors.Is-able sentinels
resolveKernelProxy copies ProxyURL verbatim with no url.Parse; a malformed proxy URL or a degenerate retry range surfaces only as an opaque fmt.Errorf("kernel: set_proxy: %w", …) / set_retry_config wrap. Unlike the Thrift-path rejection (which wraps the ErrRequiresKernelBackend sentinel for errors.Is), there's no typed error distinguishing "your config was invalid" from a transient connect failure, so a caller can't branch on it.
Suggested fix
validate the proxy URL in the Go layer (url.Parse) and wrap config-validation failures in a distinct sentinel analogous to ErrRequiresKernelBackend.
/full-review · feedback: #code-review-squad-feedback
There was a problem hiding this comment.
Fixed in bd84ae8.
Now validateKernelConfig runs url.Parse on the WithKernelProxy URL (and requires a scheme + host) in the Go layer, and wraps a malformed one in a new sentinel, errors.ErrInvalidKernelConfig. So a bad proxy URL is an errors.Is-able config error a caller can branch on — instead of surfacing as an opaque kernel: set_proxy: … wrap at connect. Added tests for the reject cases.
Code Review Squad — ReviewScore: 76/100 — MODERATE RISK Clean, well-tested kernel-integration work — the author pre-ran Isaac Review and fixed 9 findings before submitting, and the security and language passes came back clean after deep verification. Two genuine logic bugs survive in the retry-config resolver ( Could not post inline comments for: F3, F9 — see body below. Medium FindingsF3 — Options apply after Arguably the kernel's paced behavior is the safer one — consider normalizing the waits once (clamp to the documented 1s/30s defaults when a positive Low FindingsLow findings (1) — click to expandF9 — The resolved retry policy (RetryMax / wait bounds / RetryOverallTimeout) and MaxChunksInMemory are forwarded to the kernel but never logged and are absent from the
|
…y validation, test/doc parity Address the review findings from the second PR #412 pass (msrathore-db). Two real correctness fixes on the kernel retry path plus API-safety, test-gap, and doc-honesty items. Isaac Review clean on the result (0 posted comments). - kernel_config: WithKernelRetryOverallTimeout was dropped whenever retries were zeroed. WithRetries(0,0,0) leaves RetryMax==0 on a degenerate wait range, so kernelRetryConfig returned nil and the caller's explicit overall budget never reached the kernel (it kept its 900s default) — the same "silently ignored option" failure the WithKernel* gate exists to prevent, on an accepted option. The degenerate branch now also preserves an explicit overall budget (placeholder waits + zero retries) and only returns nil when there is neither an attempt count nor a budget to keep. New regression test. - kernel_config: sub-millisecond RetryWaitMin truncated to 0ms and failed the connect. The resolver validated the range in time.Duration space, but applyRetry forwards .Milliseconds(); a valid wait in (0, 1ms) passed the guard yet became min_wait_ms==0, which the kernel setter rejects (InvalidArgument) — a cross-backend divergence (the Thrift path, go-retryablehttp, accepts any Duration). Clamp MinWait/MaxWait up to a 1ms floor before forwarding. New regression test. - kernel_config / errors: validate the WithKernelProxy URL in the Go layer (url.Parse + require scheme/host) and wrap malformed input in a new ErrInvalidKernelConfig sentinel, so a bad proxy URL is an errors.Is-able config error rather than an opaque "kernel: set_proxy: …" wrap at connect. - connector / doc: WithKernelProxy now takes a named KernelProxy{URL, Username, Password, BypassHosts} struct instead of four adjacent string params, so a credential slot swap can't compile. Call sites and the doc.go catalog updated. - kernel_telemetry_test: TestKernelAuthMech covered only the PAT arm; added M2M (OAUTH/CLIENT_CREDENTIALS) and U2M (OAUTH/BROWSER_BASED_AUTHENTICATION) subtests so the enum values that NULL server-side if wrong are asserted. - internal/rows: the coltype parity guard compared Length against a hand-copied replica of ColumnTypeLength's switch (a third copy that could drift in lockstep). Extracted columnTypeLengthForID as the single classifier the method and the test both call, and added the interval types (DURATION / INTERVAL_MONTHS) to the cross-check cases. - kernel_config_test: pin the kernel's cross-repo client-conf key (cloudfetch_max_chunks_in_memory) in pure-Go CI, so a Go-side edit fails PR CI (not just the warehouse-gated nightly) and the coupling is greppable. - internal/decimalfmt: added BenchmarkExactStringSink (sink-assigned) reporting the real 1-alloc production profile alongside the escape-elided 0-alloc micro-bench, and clarified the benchmark comment. - kernel_newitems_e2e_test: softened the retry-config / overall-timeout e2e docstrings to "connect succeeds with the option set" — a happy-path query can't distinguish applied-vs-ignored; TestKernelRetryConfig pins the mapping. 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: bd84ae8 |
…CloudFetch chunk cap The resolved retry policy (RetryMax / wait bounds / RetryOverallTimeout) and the CloudFetch in-memory-chunk cap (WithKernelMaxChunksInMemory) were forwarded to the kernel with no observable trace — neither logged nor in the connection telemetry. If a customer reported a hung connect or a large-result OOM, on-call had no log line or telemetry field showing what the customer actually configured. - internal/backend/kernel/backend.go: OpenSession now logs the resolved retry policy and chunk cap at Debug right before connect (describeRetry renders "kernel-default" when Config.Retry is nil, else the forwarded values). The chunk-cap key is a local const mirroring config.KernelMaxChunksInMemoryConfKey — read only for the log line, so the cgo backend keeps its no-internal/config-dependency. - telemetry/request.go: added retry_max_attempts / retry_overall_timeout_ms / max_chunks_in_memory to DriverConnectionParameters (additive, omitempty). - kernel_telemetry.go: populate the three fields on the kernel path — only positive values (a non-positive RetryMax is the disable/default form; a zero timeout / chunk cap means keep the kernel default). The Thrift path leaves them zero, so nothing changes for that backend. - kernel_telemetry_test.go: set-and-reflected + zero-when-unset coverage. Both build paths green, go vet / gofmt / golangci-lint clean, full untagged suite + tagged kernel-package unit tests pass. Isaac Review clean on the change (0 posted). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
|
F3 (retry pacing divergence): not fixing in this PR. Both backends diverge in the behaviour but the kernel's paced behavior is the safer of the two. The reason we're leaving it: the only way to make the two share one policy is to normalize the zero-wait case in the shared WithRetries option (or WithDefaults), which would change the Thrift path's long-standing retry pacing for every existing user of this driver, a behavior change well outside the scope of this kernel PR, and one that deserves its own change with its own testing and release note. F9 (resolved retry policy + chunk cap not observable), fixed.
|
mani-mathur-arch
added
the
integration-test-kernel
|
Go integration tests triggered ( |
2 participants
The genuinely-new SEA-via-kernel driver work from the new-items backlog: the short PuPr telemetry/config/Geography tail plus advanced proxy and configurable backoff, followed by a datatype-parity + decimal-perf + RSS pass. Each is re-implemented clean on
main(post-#399) — the pre-#399 POC was the spec, not the source. Every commit was reviewed with Isaac Review before commit.Commits
PuPr new-items tail
feat(kernel): advanced HTTP proxy via WithKernelProxy(PECOBLR-3607) — fill the username / password / bypass_hosts args into the kernel'sset_proxysetter (the kernel path passed the URL only). NewWithKernelProxy(url, user, pass, bypassHosts)connector option; explicit proxy takes precedence over the env-var path.test(kernel): cover Geography as the WKT sibling of Geometry(PECOBLR-3620) — verify-and-close: GEOGRAPHY maps to the same Arrow Utf8/WKT shape as GEOMETRY (no renderer needed). Parity + live-e2e (skip-on-reject) coverage.feat(kernel): emit connection-config telemetry + fix connection-scoped error drop(PECOBLR-3627 / 3628) — emitDriverConnectionParametersat connect (kernel-path only;mode=SEA,auth_mech/auth_flowclosed enums, proxy/arrow/query-tags/metric-view). Additive on top of feat(kernel): consolidated DAIS gap-closure + PuPr feature set + nightly workflows for e2e tests #399's per-statement telemetry (not duplicated). The aggregator's"error"metric branch is routed correctly (a connection-scoped, no-statement error flushes standalone rather than being dropped), but it is defensive: no exported API emits a standalone"error"metric today, so the branch guards routing for a future producer rather than changing observable behavior now (clarified in review — see below).docs(kernel): note token caching + client reuse are inherited from the kernel(PECOBLR-3606 / 3626) — verify-and-close, doc-only: both are handled inside the kernel below the C ABI.feat(kernel): configurable HTTP retry / backoff(PECOBLR-3598) — wireWithRetries(RetryWaitMin/Max/RetryMax, incl. the disable form) into the kernel's newset_retry_configC-ABI setter, plus a kernel-onlyWithKernelRetryOverallTimeout.Datatype parity + decimal perf + RSS
feat(kernel): report column-type metadata on the kernel Rows path(PECOBLR-3692) —kernelRowsimplemented onlydriver.Rows, sodatabase/sqlfell back to""DatabaseTypeName /interface{}ScanType for every column vs Thrift's typed metadata (a silent regression for ORMs / typed-scan tooling that value-level tests can't see). New shared pure-Goarrowscan.ColumnTypeInfoFormaps Arrow →{DatabaseTypeName, ScanType, Length}matching Thrift for all 21 types;kernelRowsnow implements the 4 optionalRowsColumnType*interfaces. Pure-Go guard + liveTestKernelThriftColumnTypeParity.perf(decimal): alloc-free exact decimal renderer (shared Thrift + kernel)(PECOBLR-3691) — rewritedecimalfmt.ExactString(on the shared render path) to allocate only the returned string: uint64 fast path renders into a stack scratch,big.Intonly for true 128-bit magnitudes. Byte-identical output, pinned by an oracle-parity test (old impl kept verbatim as reference); 38.9ns/18B/1alloc → ~12ns/0B/0alloc. Live 50k-row multi-batch parity vs Thrift.feat(kernel): tune CloudFetch in-memory chunks via WithKernelMaxChunksInMemory(PECOBLR-3691, RSS) — bound how many decompressed CloudFetch chunks the kernel holds in memory (default 16), the knob that trades throughput for peak RSS on large results (measured 265 MB @ chunks=4 vs 856 MB @ chunks=32 on a 3M-row query). Kernel-only option forwarded as the client-onlycloudfetch_max_chunks_in_memorysession conf, which the kernel folds into its result config and strips before the SEA wire — never reaching the server or the Thrift path. No new C-ABI symbol.Kernel dependency
Two commits need new kernel behavior on the same kernel branch (PR #178):
set_retry_configC-ABI symbol (backoff commit), andapply_client_result_overridesreader that appliescloudfetch_max_chunks_in_memory(chunk-knob commit).KERNEL_REVis pinned to that branch's head (ffa73c0), so the tagged CI links both. Re-pin to the squash-merge SHA once the kernel PR lands. The other commits are driver-only and need no kernel change.Testing
Both build paths green (
CGO_ENABLED=0 go test ./...+CGO_ENABLED=1 go test -tags databricks_kernel ./...),go vetclean, gofmt clean,golangci-lint run ./...0 issues. Pure-Go tests (option wiring, field-classification guards, ColumnType mapper, decimal oracle-parity, retry/proxy/telemetry resolvers, telemetry full-loop) run underCGO_ENABLED=0; tagged tests drive the real cgo setters; live e2e tests (proxy-rejects-bad-URL, proxy-credential-forwarding, geography, retry config/disable/overall-timeout, ColumnType parity, 50k-row decimal parity, CloudFetch chunk-knob drain) run against the staging warehouse. Each commit was reviewed with Isaac Review and its findings addressed before commit.Review findings addressed
fix(kernel): address code-review findings(29edbba) resolves all 9 findings from the code review; a follow-upisaac reviewcame back clean (0 posted comments). All threads are resolved.WithRetries(N, 0, 0)silently reverted to the kernel default —kernelRetryConfignow forwards the caller'sRetryMax(with placeholder waits) on a degenerate range instead of returningnil. BecauseWithDefaults()runs before options,WithRetries(n, 0, 0)zeroed the waits and dropped the attempt count to the kernel's 5-retry default — a silent cross-backend divergence from Thrift. Only a zero-value range with no attempt count still falls back to the kernel default. New regression subtest.WithKernel*options trip the same gate; generalized to name theWithKernel*family, and the test now guards against a stale subset."error"branch and its test as defensive (no current producer), rather than an active behavior change.UINT8/16/32/64 → BIGINT/int64toColumnTypeInfoFor(matching the scanner's int64 widening) and to both guard tests.WithKernelMaxChunksInMemoryabsent from thedoc.gocatalog — added the bullet.TestColumnTypeInfoMatchesThriftMappingcross-checksColumnTypeInfoForagainst the ThriftgetScanType/GetDBTypeName/ length logic, so drift fails inCGO_ENABLED=0PR CI (not just the warehouse-gated nightly).TestKernelE2EProxyForwardsCredentialscaptures theProxy-Authorizationa local CONNECT proxy sees and asserts it decodes to exactlyuser:pass(a slot swap would showpass:user).This pull request and its description were written by Isaac.