feat(sea): per-statement metricViewMetadata knob on ExecuteStatementOptions - #397
feat(sea): per-statement metricViewMetadata knob on ExecuteStatementOptions#397msrathore-db wants to merge 3 commits into
Conversation
Sorry, something went wrong.
Independent devils-advocate self-review (cold-eye, no author context)Verdict: no HIGH or CRITICAL findings; merge-ready pending human reviewer sign-off. Two LOW notes captured for transparency: L1 (semantic — accept-as-designed): metricViewMetadata: false is indistinguishable from omitting the option — both leave confOverlay untouched. This is intentional (the knob is opt-in only; there is no use case for explicitly clearing a per-statement conf since the default is unset). Worth a one-line JSDoc clarification if a follow-up touches this option, but not a merge blocker. L2 (forward-reference): The JSDoc mentions "this knob is honored only on the Thrift backend", which forward-references the SEA backend that does not exist on main yet (it's on the abstraction stack). A reviewer who greps lib/ for SEA will find nothing. The phrasing is correct once the SEA stack lands and is the only way to document the gap pre-emptively, so it stays as-is. Verified against PR-A diff at head: pattern existence, test count (4/4 new tests pass), order-of-operations w.r.t. existing query_tags block, protocol-version gating parity with the query_tags precedent, additive-only public-surface change. No backward-compat breakage. Hardcoded conf-key string matches existing project style (same as query_tags). Cross-checks performed: lint clean, type-check clean (pre-existing failures in examples/tokenFederation/ only), the single failing test on tests/unit/DBSQLSession.test.ts:235 is a pre-existing LZ4 cloud-fetch issue on origin/main unrelated to this change. This pull request and its description were written by Isaac. |
Sorry, something went wrong.
Address DA review note N4 on PR #397: the public type is `boolean` (not `true`-literal), but the wiring treats `false` and omitted identically. Spell that out in the JSDoc so callers don't expect `false` to clear a server-side default. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…ntOptions Add `metricViewMetadata?: boolean` to `ExecuteStatementOptions`. When true, the Thrift backend forwards `spark.databricks.optimizer.enableMetricViewMetadata=true` via the request `confOverlay`, alongside any `query_tags` already serialized there. The option is per-statement only and does not persist across queries. The SEA backend will route the same key through napi `statementConf` once the kernel statement-options surface lands; until then the option is honored only on Thrift. Documented in the public option JSDoc so users do not silently lose the conf on SEA. Unit tests assert the option appears in the outgoing `TExecuteStatementReq` when set, is omitted when unset or `false`, and coexists with `queryTags` in the same `confOverlay`. Audit refs: rows 1.17 and 2.18 of sea-workflow/audits/2026-05-28-cross-driver-audit.md (F12 in the PR #347 audit). Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Address DA review note N4 on PR #397: the public type is `boolean` (not `true`-literal), but the wiring treats `false` and omitted identically. Spell that out in the JSDoc so callers don't expect `false` to clear a server-side default. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
…LSession Rebasing onto main brought in the #378 backend-abstraction, which changed the DBSQLSession constructor from `{ handle, context }` to `{ backend }`. The metricViewMetadata tests still assert the right thing (the driver's confOverlay carries the metric-view conf key, now built in ThriftSessionBackend), so switch their construction to the `createSessionForTest({ handle, context })` helper the rest of the suite uses post-abstraction. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
Summary
Adds an explicit metricViewMetadata?: boolean knob to ExecuteStatementOptions. When true, the Thrift backend forwards spark.databricks.optimizer.enableMetricViewMetadata=true via the outgoing TExecuteStatementReq.confOverlay, scoped to a single statement.
This is the C7 (metric-view-metadata) cluster of the autonomous Thrift↔SEA parity drive — audit refs:
What changed
SEA wiring
The SEA backend will route the same key through napi statementConf once the kernel statement-options surface lands (kernel PR #75 + NodeJS PR #393). Until then the option is honored only on Thrift; the public JSDoc states this so users do not silently lose the conf on SEA. Cross-driver round-trip is exercised in the companion driver-test PR.
Test plan
Notes