Deprecate the SEA backend; steer users to the kernel path - #920
Deprecate the SEA backend; steer users to the kernel path#920vikrantpuppala wants to merge 1 commit into
Conversation
2553f40 to
dde49ce
Compare
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
🔵 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.
dde49ce to
11aae78
Compare
There was a problem hiding this comment.
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.
11aae78 to
a06ef46
Compare
There was a problem hiding this comment.
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.
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>
a06ef46 to
d01ddf4
Compare
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
🔵 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.
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-requiredordinalfield, socur.execute("... WHERE col = ?", ("value",))fails with HTTP 400 against RT/Lakehouse warehouses (which requireuse_sea=Trueand 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
logger.warninginSeaDatabricksClient.__init__(fires once per SEA session, not per query) pointing users touse_kernel=True+ the[kernel]extra.connect()docstring now marksuse_seadeprecated/incomplete, and the staleuse_kerneldocstring is refreshed (it ships on PyPI via the[kernel]extra and supports parameter binding — the old text wrongly said neither).examples/experimental/SEA harness and the CONTRIBUTING backend table are flagged as deprecated.What this does / doesn't do
use_seacannot simply be rerouted to Thrift without breaking the exact warehouses it targets.pyarrowanddatabricks-sql-kernelremain optional extras (the kernel wheel is a platform-specific PyO3 binary; forcing it on every install would break unsupported platforms).Verification
tests/unit/test_sea_backend.pypass, including a newtest_initialization_warns_backend_incomplete.?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.