-
Notifications
You must be signed in to change notification settings - Fork 64
fix(kernel): Thrift-parity fixes + GetArrowBatches + decimal-as-float opt-in + U2M scopes/telemetry + SPOG warehouse-id routing #416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
465c68c
21ad6fa
c077271
8815db9
0163bcd
dec28fe
531f6c5
5c25584
ba072af
e4dbfb5
c9e7581
4fde591
aac47ca
f8df111
77c54ec
9257e65
0d33408
c82f6c2
e62be8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,13 @@ type connector struct { | |
| client *http.Client | ||
| } | ||
|
|
||
| // interactiveU2MAuthenticator is satisfied only by the browser-based U2M | ||
| // authenticator (U2MClientID is unique to it; PAT/M2M lack it). Matches the | ||
| // structural check the kernel backend uses to detect U2M. | ||
| type interactiveU2MAuthenticator interface { | ||
| U2MClientID() string | ||
| } | ||
|
|
||
| // Connect returns a connection to the Databricks database from a connection pool. | ||
| func (c *connector) Connect(ctx context.Context) (driver.Conn, error) { | ||
| defer debuglog.Track(ctx, "connector.Connect", "host=%s", c.cfg.Host)() | ||
|
|
@@ -86,16 +93,31 @@ func (c *connector) Connect(ctx context.Context) (driver.Conn, error) { | |
| telemetryClient = withSpogHeaders(c.client, spogHeaders) | ||
| } | ||
|
|
||
| // Skip telemetry on the kernel U2M path: the kernel owns the interactive browser | ||
| // flow, so the telemetry/feature-flag call through the interactive authenticator | ||
| // would launch a second, redundant browser (and can block connect on its | ||
| // callback). Telemetry is best-effort, so it's dropped here rather than made to | ||
| // prompt. Unauthenticated telemetry (Python/Node parity) is tracked in PECOBLR-3839. | ||
| skipTelemetry := false | ||
| if c.cfg.UseKernel { | ||
| if _, isU2M := c.cfg.Authenticator.(interactiveU2MAuthenticator); isU2M { | ||
| skipTelemetry = true | ||
| log.Debug().Msg("telemetry skipped: kernel U2M owns the interactive auth flow") | ||
| } | ||
| } | ||
|
|
||
| // Initialize telemetry: client config overlay decides; if unset, feature flags decide | ||
| conn.telemetry = telemetry.InitializeForConnection(ctx, telemetry.TelemetryInitOptions{ | ||
| Host: c.cfg.Host, | ||
| DriverVersion: c.cfg.DriverVersion, | ||
| UserAgent: client.BuildUserAgent(c.cfg), | ||
| HTTPClient: telemetryClient, | ||
| EnableTelemetry: c.cfg.EnableTelemetry, | ||
| BatchSize: c.cfg.TelemetryBatchSize, | ||
| FlushInterval: c.cfg.TelemetryFlushInterval, | ||
| }) | ||
| if !skipTelemetry { | ||
| conn.telemetry = telemetry.InitializeForConnection(ctx, telemetry.TelemetryInitOptions{ | ||
| Host: c.cfg.Host, | ||
| DriverVersion: c.cfg.DriverVersion, | ||
| UserAgent: client.BuildUserAgent(c.cfg), | ||
| HTTPClient: telemetryClient, | ||
| EnableTelemetry: c.cfg.EnableTelemetry, | ||
| BatchSize: c.cfg.TelemetryBatchSize, | ||
| FlushInterval: c.cfg.TelemetryFlushInterval, | ||
| }) | ||
| } | ||
| if conn.telemetry != nil { | ||
| log.Debug().Msg("telemetry initialized for connection") | ||
| conn.telemetry.RecordOperation(ctx, conn.id, "", telemetry.OperationTypeCreateSession, sessionLatencyMs, nil) | ||
|
|
@@ -580,6 +602,21 @@ func kernelExperimental(c *config.Config) *config.KernelExperimentalConfig { | |
| return c.KernelExperimental | ||
| } | ||
|
|
||
| // WithKernelDecimalAsFloat makes the kernel path scan top-level DECIMAL columns to | ||
| // a lossy float64 instead of the exact fixed-point string. The kernel still | ||
| // receives native Arrow Decimal128; this only changes how the Go scanner | ||
| // materializes each cell, skipping per-cell string formatting for a cheap scalar. | ||
| // Precision beyond ~15-17 digits is lost, so it is opt-in and off by default; | ||
| // it mirrors the Thrift driver's pre-UseArrowNativeDecimal behavior. | ||
| // | ||
| // EXPERIMENTAL, kernel-only: the default (Thrift) backend rejects this at connect | ||
| // (use WithArrowNativeDecimal there instead). | ||
| func WithKernelDecimalAsFloat(asFloat bool) ConnOption { | ||
| return func(c *config.Config) { | ||
| kernelExperimental(c).DecimalAsFloat = asFloat | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (P3, test gap) Add this option to the Thrift-reject test.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 0d33408 — |
||
| } | ||
| } | ||
|
|
||
| // WithKernelTrustedCerts adds a PEM CA-certificate bundle to the kernel's TLS | ||
| // trust store on top of the system roots — for a corporate re-signing proxy or an | ||
| // on-prem CA. Required (rather than relying on SSL_CERT_FILE) because the kernel's | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| package dbsql | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/databricks/databricks-sql-go/auth/oauth/m2m" | ||
| "github.com/databricks/databricks-sql-go/auth/pat" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| // u2mShaped stands in for the browser-based U2M authenticator, which exposes | ||
| // U2MClientID() (its real constructor does live OIDC discovery, unusable in a unit | ||
| // test). The kernel-U2M telemetry guard keys off exactly this method. | ||
| type u2mShaped struct{} | ||
|
|
||
| func (u2mShaped) U2MClientID() string { return "databricks-sql-connector" } | ||
|
|
||
| // TestInteractiveU2MAuthenticatorDetection pins the structural check the kernel-U2M | ||
| // telemetry guard relies on: only a U2M-shaped authenticator satisfies it, so | ||
| // PAT/M2M keep authenticated telemetry while U2M does not trigger a 2nd browser. | ||
| func TestInteractiveU2MAuthenticatorDetection(t *testing.T) { | ||
| _, isU2M := interface{}(u2mShaped{}).(interactiveU2MAuthenticator) | ||
| assert.True(t, isU2M, "a U2M-shaped authenticator must satisfy interactiveU2MAuthenticator") | ||
|
|
||
| _, patIsU2M := interface{}(&pat.PATAuth{AccessToken: "dapi-x"}).(interactiveU2MAuthenticator) | ||
| assert.False(t, patIsU2M, "PAT must NOT satisfy interactiveU2MAuthenticator") | ||
|
|
||
| m2mAuth := m2m.NewAuthenticator("cid", "secret", "dbc-1234.cloud.databricks.com") | ||
| _, m2mIsU2M := m2mAuth.(interactiveU2MAuthenticator) | ||
| assert.False(t, m2mIsU2M, "M2M must NOT satisfy interactiveU2MAuthenticator") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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)."WithKernelDecimalAsFloatintroduces exactly such a config where it does not — a top-level kernel DECIMAL comes back asfloat64(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0d33408 — updated the
TestTopLevelDecimalRenderingcomment in arrowscan_parity_test.go to carve outWithKernelDecimalAsFloat(off by default, so the default-config invariant still holds), so it no longer reads as violated.