feat(kernel): SEA-via-kernel data types, params, errors & query-id telemetry#397
Open
mani-mathur-arch wants to merge 8 commits into
Open
feat(kernel): SEA-via-kernel data types, params, errors & query-id telemetry#397mani-mathur-arch wants to merge 8 commits into
mani-mathur-arch wants to merge 8 commits into
Conversation
mani-mathur-arch
changed the title
mani-mathur-arch
force-pushed
the
mani/sea-kernel-dais-gaps
branch
from
2207471 to
1ebcaa7
Compare
mani-mathur-arch
force-pushed
the
mani/sea-kernel-pupr-features
branch
from
beef094 to
9c23d91
Compare
The kernel backend returns INTERVAL columns as native arrow duration (day-time) and month-interval (year-month) values, whereas the Thrift path receives them pre-formatted from the server (its native-interval config is off in prod, so it never scans a duration/month-interval array). Format them Go-side in the shared untagged arrowscan package to the same strings the Thrift path returns — "D HH:MM:SS.nnnnnnnnn" and "years-months", negatives signed — so a query's result is identical across backends. Replaces the fail-loud "intervals are not yet handled" default arm with the two type arms; golden-string unit tests (day/day-to-sec/seconds-unit/negative, year/year-month/months/negative) run in the default CGO_ENABLED=0 build. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Over Arrow the kernel delivers TIMESTAMP with a tz ("UTC") and TIMESTAMP_NTZ
with an empty tz, but — like the Thrift path — the driver ignores that field
and renders both via ToTime + .In(loc). The LTZ-vs-NTZ difference is carried
entirely by the instant the server sends, not by the client inspecting the
tz, so no arrowscan change is needed: the existing code already matches Thrift.
Verified live on both backends (America/New_York + Asia/Kolkata, including a
DST spring-forward literal, and nested/null shapes): kernel == Thrift
byte-for-byte for both types. Add an untagged parity case (TimeZone "UTC" vs
"") so a future "don't shift NTZ" change — which looks correct in isolation
but would diverge from Thrift, which shifts NTZ too — fails default CI, plus a
live e2e pinning the round-trip.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
VARIANT and GEOMETRY need no special rendering on the kernel path: verified
live on both backends, both arrive over Arrow as plain STRING columns — a
top-level VARIANT is its JSON text ({"a":1,"b":[2,3]}), a scalar VARIANT is
"42", and GEOMETRY is its WKT "POINT(1 2)". Nested inside a container the
variant/geometry element is a string leaf, rendered as a quoted, JSON-escaped
string (the variant's own JSON is escaped as text, NOT re-parsed) — identical
on both backends.
Add untagged parity cases: a top-level string equivalence (variant object /
scalar / geometry WKT) and nested string-leaf cases in an array, so the string
arm's handling of these types can't silently drift between backends. GEOGRAPHY
is intentionally excluded — not enabled on the benchmark warehouse and no
consumer has asked for it.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
A failed kernel query returned the raw *KernelError, so consumers doing errors.As(err, &DBExecutionError) — the way they inspect Thrift failures — didn't get SqlState()/QueryId()/IsRetryable() through the standard interface. kernelOp.ExecutionError now digs the sqlstate out of the underlying *KernelError and wraps the cause via NewExecutionErrorWithState, so kernel query failures surface with the same DBExecutionError shape as Thrift. Adds the neutral NewExecutionErrorWithState to the untagged internal/errors package (Thrift's NewExecutionError needs a TGetOperationStatusResp the kernel backend can't produce), unit-tested in the default CGO_ENABLED=0 build. Parity is type + SQLSTATE, not byte-identical text — kernel messages are richer (they carry the SQL error class + suggestions). Verified live: unknown table → 42P01, unknown column → 42703, byte-identical sqlstate to Thrift. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…al caveat Record what the kernel backend inherits unchanged above the backend seam — the database/sql connection pool (each conn wraps one kernel session), per-connection CREATE_SESSION / DELETE_SESSION telemetry (recorded unconditionally in connector.go, backend-agnostic), and the telemetry exporter's circuit breaker — and that result types render byte-for-byte with Thrift (scalars, exact DECIMAL, TIMESTAMP / TIMESTAMP_NTZ, INTERVAL, nested + VARIANT as JSON, GEOMETRY as WKT). Remove the now-stale "INTERVAL types are not yet handled by the kernel scanner" caveat (intervals render now), and narrow the telemetry caveat to what is actually missing: only EXECUTE_STATEMENT telemetry (gated on a per-statement query id the kernel C ABI doesn't yet surface) — CREATE_SESSION / DELETE_SESSION are unaffected. Add a live-verified connection-pool e2e (40 concurrent queries over pool cap 8) backing the inherited-pool claim. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The kernel backend rejected bound parameters at execute time. Now it binds them:
the driver's backend.Param{Name, Type, Value} maps 1:1 onto the kernel's
kernel_statement_bind_parameter (K1) — value already stringified, Type the
Databricks SQL type name, empty Name → positional, nil Value → SQL NULL ("VOID").
bindParams runs after set_sql (which clears any prior binds), using the existing
newCStr/newCStrOrNull helpers and the call() FFI-safety wrapper; a bind failure
closes the statement and surfaces via toStatementError.
Removes the fail-loud reject in Execute (and its now-unused errors import). The
old TestExecuteRejectsParams is repurposed as TestExecuteHandleLessOpContract
(the non-nil handle-less Operation contract, now driven by a nil-session failure
since params no longer reject). Live parity: 10 cases (positional/named, each
scalar type, NULL, multi-param, predicate) produce byte-identical output on the
kernel and Thrift backends.
Requires a kernel build carrying kernel_statement_bind_parameter; the KERNEL_REV
pin is bumped to the K1 merge SHA when it lands.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
kernelOp.StatementID() returned "", so the kernel backend emitted no EXECUTE_STATEMENT telemetry and QueryIdCallback fired with an empty id (connection.go gates both on a non-empty statement id). Wire StatementID() to the server query id via kernel_executed_statement_query_id (K1), captured at execute time into a cached field — the same lifetime discipline as affectedRows, since the C accessor returns a pointer borrowed from the exec handle and the op is closed (nulling exec) before StatementID() is read on some paths. C.GoString deep-copies out of the borrowed string. Live e2e: a registered QueryIdCallback fires with a non-empty server id after a kernel query. Updates doc.go — bound parameters (c6) and EXECUTE_STATEMENT telemetry are now supported; the remaining kernel-backend limitation is batch-boundary (not mid-fetch) read cancellation. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Bumps the kernel pin from the #163 canceller rev to the tip of the PuPr statement-surface branch (databricks-sql-kernel#165), which adds kernel_statement_bind_parameter and kernel_executed_statement_query_id — the two C-ABI symbols the bound-parameter (c6) and EXECUTE_STATEMENT-telemetry (c7) commits link against. Re-pin to the squash-merge SHA once #165 lands. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
force-pushed
the
mani/sea-kernel-dais-gaps
branch
from
89ce8fc to
024053d
Compare
mani-mathur-arch
force-pushed
the
mani/sea-kernel-pupr-features
branch
from
9c23d91 to
717a95f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #396 (DAIS gap-closure). The PuPr feature set for the SEA-via-kernel backend, as a feature-commit stack. Depends on the kernel PuPr statement surface (databricks-sql-kernel#165) for the two tagged commits.
Commits
Untagged / no kernel dependency (run in the default
CGO_ENABLED=0CI):feat(arrowscan): render INTERVAL day-time & year-month— Duration / MonthInterval → Thrift's server-string form, in the shared untagged scanner.test(kernel): pin TIMESTAMP vs TIMESTAMP_NTZ parity— both backends shift NTZ via.In(loc)(verified live); a parity case guards against a one-sided change.test(kernel): pin VARIANT & GEOMETRY render parity— both arrive as strings (verified live); nested string-leaf + top-level cases.fix(kernel): surface kernel errors as DBExecutionError with sqlstate—errors.As→DBExecutionError/SqlState()parity with Thrift (42P01/42703), via a neutral untagged constructor.docs(kernel): document verified inherited features— pool / telemetry / circuit-breaking inherited above the seam; drops the now-stale interval caveat.Tagged (
cgo && databricks_kernel), depend on #165:feat(kernel): bind query parameters via the C ABI—backend.Param{Name,Type,Value}→kernel_statement_bind_parameter; 10-case live parity (positional/named, each scalar, NULL, multi-param, predicate).feat(kernel): surface server query id for EXECUTE_STATEMENT telemetry—StatementID()→kernel_executed_statement_query_id, ungating EXECUTE_STATEMENT telemetry + QueryIdCallback.build(kernel): pin KERNEL_REV to the PuPr statement-surface rev— points the build recipe at [PECO-1083] Update Go Thrift files to reflect V8 and add version checking for parameterized queries #165's tip so the tagged CI links the new symbols; re-pin to [PECO-1083] Update Go Thrift files to reflect V8 and add version checking for parameterized queries #165's squash-merge SHA once it lands.Testing
Default
CGO_ENABLED=0suite green; tagged build + unit tests green; every live behavior (intervals, NTZ, variant/geometry, error sqlstate, connection pool, param binding, query-id telemetry) verified against a staging warehouse.golangci-lintclean on all touched files.Isaac Review: two batched passes, both landing clean.
Long.MaxValueregression case.)StatementID()doc-comment the reviewer flagged.Several behaviors were pinned by live-probing both backends first rather than coding to assumption: TIMESTAMP_NTZ (both shift via
.In(loc)— the "don't-shift NTZ" plan premise was wrong and would have broken parity) and VARIANT/GEOMETRY (both arrive as strings).Merge order
Kernel #163 → #165, then driver #393 → #395 → #396 → this PR; re-pin
KERNEL_REVto #165's merge SHA at that point. The tagged/cgo CI links #165's branch tip in the meantime.This pull request and its description were written by Isaac.