You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/09/26 12:17:53 UTC
[PR] fix(databricks): stop the string-type patch writing SQLAlchemy's shared colspecs [superset]
aminghadersohi opened a new pull request, #44707:
URL: https://github.com/apache/superset/pull/44707
### SUMMARY
`superset.db_engine_specs.databricks.monkeypatch_dialect()` runs when the engine spec module is imported. It stores `ContextAwareStringType` with `HiveDialect.colspecs[types.String] = ...`. PyHive's `HiveDialect` does not define `colspecs`, so this writes into SQLAlchemy's `DefaultDialect.colspecs` dict, which is shared by every dialect that does not define its own.
Consequences for those unrelated dialects, once any code path has imported the Databricks spec (`load_engine_specs()` does):
- `String` and every subclass are adapted to the decorator, because `adapt_type` walks the MRO. `Unicode` and `Text` lose their type.
- `sa.Enum(...).dialect_impl(dialect)` raises `TypeError: String.__init__() got an unexpected keyword argument '_enums'`.
- Dialects that unwrap `TypeDecorator` binds in a loop can stop terminating. clickhouse-connect 1.9.0's DateTime64 bind planning spins forever on every String bind parameter.
In one environment with the Databricks spec imported, 38 installed dialect entry points shared that dict and all of them were affected. Examples: pyhive Hive/Presto, pydruid, pinotdb, elasticsearch-dbapi, sqlalchemy-drill, impyla, kylinpy, sqlalchemy-dremio flight, firebolt, clickhouse-connect.
The fix gives `HiveDialect` its own copy (`{**HiveDialect.colspecs, String: ContextAwareStringType, Enum: Enum}`), so the patch keeps its intended Hive-family scope and never touches the shared dict. `Enum` maps to itself so it is not adapted to the string decorator, which cannot take Enum's arguments.
### TESTING INSTRUCTIONS
`pytest tests/unit_tests/db_engine_specs/test_databricks.py -k shared_colspecs`. It fails before the change (`String` present in `DefaultDialect.colspecs`) and passes after. The rest of the file is unchanged by this PR.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(databricks): stop the string-type patch writing SQLAlchemy's shared colspecs [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44707:
URL: https://github.com/apache/superset/pull/44707#issuecomment-5903983382
@rebenitez1802 since your approval: merged master and addressed EnxDev's two notes in c30fdbdf73. The docstring now scopes the patch to HiveDialect-based Databricks dialects, and a new test checks that a Hive-based `DatabricksDialect` still compiles `'O\'Hara'` with the patch on the copied colspecs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(databricks): stop the string-type patch writing SQLAlchemy's shared colspecs [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44707:
URL: https://github.com/apache/superset/pull/44707#issuecomment-5846205130
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(databricks): stop the string-type patch writing SQLAlchemy's shared colspecs [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #44707:
URL: https://github.com/apache/superset/pull/44707#discussion_r4137496751
##########
tests/unit_tests/db_engine_specs/test_databricks.py:
##########
@@ -1198,3 +1198,41 @@ def test_get_engine_spec_unrecognized_driver_prefers_python_connector() -> None:
get_engine_spec("databricks", "databricks-sql-python")
is DatabricksPythonConnectorEngineSpec
)
+
+
+def test_monkeypatch_dialect_leaves_shared_colspecs_alone() -> None:
+ """
+ The Databricks string patch must not reach dialects outside the Hive family.
+
+ PyHive's HiveDialect inherits SQLAlchemy's shared ``DefaultDialect.colspecs``;
+ writing to it changed String/Unicode/Text/Enum handling for every dialect that
+ does not define its own ``colspecs``.
+ """
+ import enum
+
+ import sqlalchemy as sa
+ from sqlalchemy.engine.default import DefaultDialect
+
+ pyhive = pytest.importorskip("pyhive.sqlalchemy_hive")
+
+ from superset.db_engine_specs.databricks import monkeypatch_dialect
+
+ class Color(enum.Enum):
+ red = "red"
+
+ class OtherDialect(DefaultDialect):
+ """A dialect that relies on the shared colspecs."""
+
+ monkeypatch_dialect() # also ran when the module was imported
+
+ assert sa.String not in DefaultDialect.colspecs
+ other = OtherDialect()
+ assert type(sa.Unicode(5).dialect_impl(other)) is sa.Unicode
+ assert type(sa.Text().dialect_impl(other)) is sa.Text
+
+ hive = pyhive.HiveDialect
+ assert hive.colspecs is not DefaultDialect.colspecs
+ assert hive.colspecs[sa.String].__name__ == "ContextAwareStringType"
Review Comment:
This proves the key is set, but nothing checks that a HiveDialect-based `DatabricksDialect` still gets backslash escaping now that the patch lives on a copy.
Worth compiling `sa.literal("O'Hara")` with `literal_binds` against a local `class DatabricksDialect(HiveDialect)` and asserting the SQL contains `'O\'Hara'`? It needs `importorskip("databricks.sql")` for `ParamEscaper`, and it's the actual behavior this function exists for.
##########
superset/db_engine_specs/databricks.py:
##########
@@ -103,6 +103,12 @@ def monkeypatch_dialect() -> None:
Since the DatabricksDialect.colspecs points to the base class (HiveDialect.colspecs)
Review Comment:
The connector's own `DatabricksDialect` from the <3.0 era this docstring describes subclasses `DefaultDialect`, not `HiveDialect`. So the only reason the patch ever reached it was the shared-dict write, and after this change it won't. That's fine on the pinned `databricks-sqlalchemy>=2.0.1`, since that dialect defines its own `colspecs` with `DatabricksStringType`.
The docstring still reads as if the old connector is the target, though. Could we say the patch only covers HiveDialect-based Databricks dialects (`sqlalchemy-databricks`), or is it worth asking whether we still need it at all?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(databricks): stop the string-type patch writing SQLAlchemy's shared colspecs [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44707:
URL: https://github.com/apache/superset/pull/44707#issuecomment-5906110496
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(databricks): stop the string-type patch writing SQLAlchemy's shared colspecs [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44707:
URL: https://github.com/apache/superset/pull/44707#discussion_r4140677538
##########
superset/db_engine_specs/databricks.py:
##########
@@ -103,6 +103,12 @@ def monkeypatch_dialect() -> None:
Since the DatabricksDialect.colspecs points to the base class (HiveDialect.colspecs)
Review Comment:
Reworded in c30fdbdf73: the docstring now says the patch covers HiveDialect-based Databricks dialects from `sqlalchemy-databricks`, and that the supported `databricks-sqlalchemy` dialect defines its own colspecs and escaping, so it isn't a target. I kept the patch itself since removing it would change behavior for anyone still on the Hive-based dialect; that's better as a separate deprecation.
##########
tests/unit_tests/db_engine_specs/test_databricks.py:
##########
@@ -1198,3 +1198,41 @@ def test_get_engine_spec_unrecognized_driver_prefers_python_connector() -> None:
get_engine_spec("databricks", "databricks-sql-python")
is DatabricksPythonConnectorEngineSpec
)
+
+
+def test_monkeypatch_dialect_leaves_shared_colspecs_alone() -> None:
+ """
+ The Databricks string patch must not reach dialects outside the Hive family.
+
+ PyHive's HiveDialect inherits SQLAlchemy's shared ``DefaultDialect.colspecs``;
+ writing to it changed String/Unicode/Text/Enum handling for every dialect that
+ does not define its own ``colspecs``.
+ """
+ import enum
+
+ import sqlalchemy as sa
+ from sqlalchemy.engine.default import DefaultDialect
+
+ pyhive = pytest.importorskip("pyhive.sqlalchemy_hive")
+
+ from superset.db_engine_specs.databricks import monkeypatch_dialect
+
+ class Color(enum.Enum):
+ red = "red"
+
+ class OtherDialect(DefaultDialect):
+ """A dialect that relies on the shared colspecs."""
+
+ monkeypatch_dialect() # also ran when the module was imported
+
+ assert sa.String not in DefaultDialect.colspecs
+ other = OtherDialect()
+ assert type(sa.Unicode(5).dialect_impl(other)) is sa.Unicode
+ assert type(sa.Text().dialect_impl(other)) is sa.Text
+
+ hive = pyhive.HiveDialect
+ assert hive.colspecs is not DefaultDialect.colspecs
+ assert hive.colspecs[sa.String].__name__ == "ContextAwareStringType"
Review Comment:
Added in c30fdbdf73: `test_monkeypatch_dialect_preserves_hive_databricks_escaping` compiles `sa.literal("O'Hara")` with `literal_binds` against a local `class DatabricksDialect(HiveDialect)` and asserts `'O\'Hara'` (with `importorskip("databricks.sql")`). Both tests also restore `HiveDialect.colspecs` via `monkeypatch` so they don't leak state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(databricks): stop the string-type patch writing SQLAlchemy's shared colspecs [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44707:
URL: https://github.com/apache/superset/pull/44707#issuecomment-5846235341
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44707?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 57.14%. Comparing base ([`0501796`](https://app.codecov.io/gh/apache/superset/commit/05017968378f81e44071f44aa16796ec5d2e0b9a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`71c51cb`](https://app.codecov.io/gh/apache/superset/commit/71c51cbef469220b259c6b0942047ddca5e706a1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (0501796) and HEAD (71c51cb). Click for more details.
>
> <details><summary>HEAD has 28 uploads less than BASE</summary>
>
>| Flag | BASE (0501796) | HEAD (71c51cb) |
>|------|------|------|
>|hive|4|1|
>|python|24|2|
>|presto|4|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44707 +/- ##
===========================================
- Coverage 81.12% 57.14% -23.99%
===========================================
Files 2956 2956
Lines 178436 178436
Branches 41341 41341
===========================================
- Hits 144762 101961 -42801
- Misses 30969 75492 +44523
+ Partials 2705 983 -1722
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44707/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44707/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.78% <100.00%> (ø)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44707/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44707/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44707/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.68% <0.00%> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/44707/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.73% <100.00%> (-46.68%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44707/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44707/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44707?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(databricks): stop the string-type patch writing SQLAlchemy's shared colspecs [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44707:
URL: https://github.com/apache/superset/pull/44707
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org