Skip to content

Deprecate the SEA backend; steer users to the kernel path - #920

Open
vikrantpuppala wants to merge 1 commit into
mainfrom
sea-backend-deprecation-warning
Open

Deprecate the SEA backend; steer users to the kernel path#920
vikrantpuppala wants to merge 1 commit into
mainfrom
sea-backend-deprecation-warning

Conversation

@vikrantpuppala

@vikrantpuppala vikrantpuppala commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

The SEA backend (use_sea=True) has feature gaps — notably it does not support positional (?) parameter binding. The connector emits SEA parameters without the API-required ordinal field, so cur.execute("... WHERE col = ?", ("value",)) fails with HTTP 400 against RT/Lakehouse warehouses (which require use_sea=True and refuse Thrift).

Rather than partially patch SEA, this PR marks it deprecated and steers users to the supported kernel backend (use_kernel=True + the [kernel] extra) — a SEA-native client that already handles positional and named parameter binding correctly.

Changes

  • Runtime warning: a one-time logger.warning in SeaDatabricksClient.__init__ (fires once per SEA session, not per query) pointing users to use_kernel=True + the [kernel] extra.
  • Docs: connect() docstring now marks use_sea deprecated/incomplete, and the stale use_kernel docstring is refreshed (it ships on PyPI via the [kernel] extra and supports parameter binding — the old text wrongly said neither).
  • Examples / contributor docs: the examples/experimental/ SEA harness and the CONTRIBUTING backend table are flagged as deprecated.

What this does / doesn't do

  • Does not change SEA's runtime behavior — SEA stays functional. RT/Lakehouse warehouses refuse Thrift, so use_sea cannot simply be rerouted to Thrift without breaking the exact warehouses it targets.
  • Does not change packaging — pyarrow and databricks-sql-kernel remain optional extras (the kernel wheel is a platform-specific PyO3 binary; forcing it on every install would break unsupported platforms).

Verification

  • 23/23 tests/unit/test_sea_backend.py pass, including a new test_initialization_warns_backend_incomplete.
  • Confirmed the kernel path handles positional ? params correctly — ran the kernel parameterized e2e tests live against a real warehouse: 7/7 passed (positional, named, NULL, decimal, timestamp, scientific-notation).

This pull request and its description were written by Isaac.

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — a minimal, well-tested logger.warning steering SEA users toward the kernel backend. Verified the guidance is accurate: use_kernel=True, the databricks-sql-connector dist name, and the [kernel] extra all match the codebase/pyproject.toml, and the new unit test correctly asserts the warning content. One low note on the blanket "should not be used in production" wording given RT/Lakehouse warehouses require SEA.


# The SEA backend is incomplete (e.g. it does not support positional
# parameter binding — see ES-2127451) and is slated for deprecation.
# Steer users to the Rust kernel backend, which is the supported path.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low — The warning is emitted unconditionally on every SeaDatabricksClient.__init__, and its wording ("should not be used in production") is a blanket statement. Per this PR's own description, RT/Lakehouse warehouses require use_sea=True and refuse Thrift — for those users SEA is the only available path, so "should not be used in production; use the kernel backend instead" may be misleading if the kernel backend isn't a drop-in substitute for their warehouse type. Consider softening to something like "has known feature gaps (e.g. positional parameter binding)" and noting the kernel backend as the recommended path where supported, rather than an unqualified do-not-use. Non-blocking — the message is factually grounded and the steer is correct for the parameter-binding case.

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — a one-time construction warning plus a matching unit test. Verified use_kernel=True and the [kernel] extra referenced in the message are real. One low: the message recommends the kernel path unconditionally, but the kernel wheel needs Python >= 3.10, so the advice misfires on 3.8/3.9.

Comment thread src/databricks/sql/backend/sea/backend.py Outdated
@vikrantpuppala
vikrantpuppala force-pushed the sea-backend-deprecation-warning branch from 11aae78 to a06ef46 Compare August 19, 2026 23:37
@vikrantpuppala vikrantpuppala changed the title Warn that the SEA backend is incomplete; steer users to the kernel path Deprecate the SEA backend; steer users to the kernel path Aug 19, 2026

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — a low-risk deprecation PR (runtime warning + docstring/doc flags) whose one behavior change is covered by a new unit test. One low-severity note on deprecation-message consistency and the choice of logger.warning vs DeprecationWarning.

Comment thread src/databricks/sql/backend/sea/backend.py Outdated
The SEA backend (use_sea=True) has feature gaps — notably it does not
support positional (`?`) parameter binding, which causes HTTP 400s
against RT/Lakehouse warehouses. Rather than partially patch SEA, mark
it deprecated and steer users to the supported kernel backend
(use_kernel=True), which is SEA-native and handles positional/named
parameter binding.

- Emit a warning at SeaDatabricksClient construction pointing users to
  use_kernel=True + the `[kernel]` extra.
- Document use_sea as deprecated/incomplete in the connect() docstring,
  and refresh the stale use_kernel docstring (it now ships on PyPI via
  the `[kernel]` extra and supports parameter binding).
- Flag the SEA example harness and the CONTRIBUTING backend table as
  deprecated.

SEA stays functional (RT warehouses refuse Thrift, so it can't simply
be rerouted) and is slated for eventual removal.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — a low-risk deprecation/docs PR. The runtime warning is correctly placed in SeaDatabricksClient.__init__ (once per session as described), SEA runtime behavior is unchanged, and the new unit test genuinely exercises the warning. One low note on the deprecation mechanism (logger.warning vs DeprecationWarning) and per-session log volume.

# The SEA backend is deprecated and incomplete (e.g. it does not
# support positional parameter binding) and is slated for removal.
# Steer users to the Rust kernel backend, which is the supported path.
logger.warning(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low — This deprecation is signalled via logger.warning, whereas the connector elsewhere signals API deprecations with warnings.warn(..., DeprecationWarning) (see src/databricks/sql/auth/thrift_http_client.py:45). A DeprecationWarning is the more conventional, programmatically-filterable signal for a deprecated public kwarg and integrates with -W/filterwarnings in test suites. That said, logger.warning is a defensible deliberate choice here since DeprecationWarning is suppressed by default and wouldn't reach end users — if that's the intent, this is fine as-is. Also note that because the warning fires on every SeaDatabricksClient.__init__, applications that open many short-lived SEA sessions (e.g. connection churn) will see the line repeated per session rather than once per process; consider a module-level _warned guard if that log volume is a concern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants