fix: protect SQLAlchemy reflection from String bytes format - #935
fix: protect SQLAlchemy reflection from String bytes format#935sankalpsthakur wants to merge 8 commits into
Conversation
4ce8e1c to
dabed9a
Compare
joe-clickhouse
left a comment
There was a problem hiding this comment.
Hey @sankalpsthakur. Thanks for this contribution as well. Again, I applied the follow-up review fixes directly on this branch, including:
- Statement query formats now take precedence over broader connection and engine wildcard patterns.
- Alembic's startup current database lookup now uses the internal String format override.
Worth noting that the remaining Alembic version-table read limitation is clearly in the changelog and remains outside this PR's scope.
I also merged the latest main in and reconciled conflicts. Thanks again!
There was a problem hiding this comment.
Pull request overview
This PR fixes a ClickHouse SQLAlchemy reflection failure mode when users enable a global String -> bytes read format by ensuring dialect/inspector metadata queries always apply the driver’s internal {"String": "string"} query-format override, while keeping ordinary user queries honoring the global bytes format.
Changes:
- Thread
query_formatsthrough the DB-APICursor.execute/executemanypath and SQLAlchemy dialectdo_execute*methods viaexecution_options. - Wrap SQLAlchemy reflection and other metadata queries (
DESCRIBE,system.tables,SHOW TABLES/DBS,EXISTS ..., etc.) with an internal query-format override helper. - Add unit + integration test coverage and update docs/CHANGELOG to document the behavior and new knobs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_tests/test_sqlalchemy/test_query_formats.py | Adds unit tests for query_formats plumbing and internal-format application on metadata queries. |
| tests/unit_tests/test_sqlalchemy/test_alembic.py | Verifies Alembic’s current-database lookup uses internal query formats. |
| tests/integration_tests/test_sqlalchemy/test_reflect.py | Regression test: reflection remains str under global String -> bytes, while user SELECT still yields bytes. |
| docs/sqlalchemy.mdx | Documents SQLAlchemy execution_options(query_formats=...) usage. |
| docs/driver-api.mdx | Documents new DB-API cursor query_formats keyword argument. |
| clickhouse_connect/dbapi/cursor.py | Adds query_formats passthrough for execute/executemany and the LIMIT 0 metadata probe. |
| clickhouse_connect/cc_sqlalchemy/inspector.py | Introduces with_internal_query_formats() and applies it to inspector metadata queries. |
| clickhouse_connect/cc_sqlalchemy/dialect.py | Adds _ch_query_formats() merge logic and forwards formats into cursor execution; wraps dialect metadata queries. |
| clickhouse_connect/cc_sqlalchemy/alembic/impl.py | Applies internal query formats to Alembic startup currentDatabase() lookup. |
| CHANGELOG.md | Adds a bug-fix entry describing the reflection behavior under global bytes read format. |
Summary
Supersedes #923 (same change, rebased onto current
main— resolves thecursor.pyconflict with #927'sCursor.descriptionprobe hardening by keeping_leading_keywordand the best-effortDatabaseErrorguard while threadingquery_formatsthrough both the primary query and the introspection re-query).Fixes #920.
set_default_formats("String", "bytes")(orset_read_format("String", "bytes")) is a global read format that makesStringcolumns decode asbytes. The core driver already protects orchestration queries via_INTERNAL_QUERY_FORMATS = {"String": "string"}, but the SQLAlchemy reflection path (cc_sqlalchemyinspector + dialect metadata methods) issued ordinary DB-API queries and was unprotected.Under the global bytes format this caused:
Inspector.get_columns()/Table(..., autoload_with=...)raisedTypeError: a bytes-like object is required, not 'str'onrow.type.replace("\n", "").get_table_names()returned[b'my_table']instead of['my_table'], and engine/comment comparisons likerow.engine == "Dictionary"failed.Changes
query_formatsthroughdbapi.Cursor.execute/executemanyand the SQLAlchemy dialectdo_execute*methods, mirroring the existingsettings/execution_optionsplumbing._INTERNAL_QUERY_FORMATSoverride to all SQLAlchemy metadata queries (DESCRIBE TABLE,system.tables,SHOW TABLES/SHOW DATABASES, dictionary create SQL, etc.) viawith_internal_query_formats(...).CHANGELOG entry (for
UNRELEASED → Bug Fixes)(The entry is deliberately not in the diff — the
UNRELEASEDsection conflicts on every upstream merge; happy to add it toCHANGELOG.mdon request.)Checklist
AI disclosure
This change was drafted with AI assistance and reviewed by the author before submission.
AI/LLM disclosure