fix(d1): support SQLAlchemy 2 with sqlalchemy-d1 0.2.0 - #44505
danielalyoshin wants to merge 11 commits into
Conversation
The d1 extra installs sqlalchemy-d1>=0.2.0 only. That release supports SQLAlchemy 2 and is built on sqlalchemy-cloudflare-d1. superset-engine-d1 and dbapi-d1 are retired, and their only releases cap Python below 3.12. The engine spec metadata points at sqlalchemy-d1 and drops the SQLAlchemy 2.0 known incompatibility. The spec also hides D1's internal _cf_* tables and views.
- Set allows_sql_comments = False. The DBAPI in sqlalchemy-cloudflare-d1 only reports column names when the statement starts with SELECT, PRAGMA or WITH, so a query with a leading comment returned rows without a cursor description. - Point the install text at the d1 extra and add a version_requirements note, so an environment with sqlalchemy-d1 0.1.0 is not told the requirement is met. - Cap the d1 extra at <1. - Drop the _cf_ table and view overrides; the sqlalchemy-d1 dialect already filters internal names. - Drop tests that copied the SQLite convert_dttm test or only asserted constants, and trim the metadata test to the package and install text.
Code Review Agent Run #4cdb4cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44505 +/- ##
==========================================
- Coverage 81.11% 81.11% -0.01%
==========================================
Files 2955 2955
Lines 178330 178357 +27
Branches 41311 41307 -4
==========================================
+ Hits 144651 144671 +20
- Misses 30974 30980 +6
- Partials 2705 2706 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Replace allows_sql_comments = False with an execute override that drops comments ahead of a statement. The flag is only read by the SQL Lab executor, so a dataset made from SQL, an alert, or a comment added by SQL_QUERY_MUTATOR still reached the driver with a leading comment and came back as rows without a cursor description. Every query path goes through execute. - Refuse base_url in the connection URI, the way MySQL refuses local_infile. The driver sends the API token to the host that base_url names.
- Map the d1 engine to sqlglot's SQLite dialect. The generic dialect fails on bracket identifiers and blob literals, and rewrites `->>` into JSON_EXTRACT_SCALAR, which D1 does not have. - Show D1's own error message instead of the whole JSON reply the DBAPI puts in the exception, and stop the "no such column" pattern from taking the offset and result code as part of the column name. - Set default_driver to httpx, the driver name of the d1 dialect in sqlalchemy-d1 0.2.0. - Turn off file upload. D1 has no transactions, so a failed upload could leave a half-written table.
Strings set when the module loads must be lazy, as tests/unit_tests/views/test_i18n_constants.py checks.
D1 keeps a DATE as text such as 2026-09-20 and compares it as text. The inherited literal '2026-09-20 00:00:00' sorts after that day, so a time range on a DATE column started and ended one day late. sqlalchemy-d1 0.2.0 marks DATE columns temporal on its own, so these filters apply by default.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| SQL Lab, datasets made from SQL, charts and alerts. | ||
| """ | ||
| stripped = LEADING_COMMENTS_REGEX.sub("", query, count=1) | ||
| super().execute(cursor, stripped or query, database, **kwargs) |
There was a problem hiding this comment.
Suggestion: Leading comments added by SQL_QUERY_MUTATOR, including query-attribution or audit tags, are removed before execution, so D1 loses the mutator's intended database-visible tagging.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Logic error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/db_engine_specs/d1.py
**Line:** 114:114
**Comment:**
*Logic Error: Leading comments added by `SQL_QUERY_MUTATOR`, including query-attribution or audit tags, are removed before execution, so D1 loses the mutator's intended database-visible tagging.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
This is on purpose. The DBAPI in sqlalchemy-cloudflare-d1 only reports column names when the statement text starts with SELECT, PRAGMA or WITH. A query with a leading comment comes back as rows without a cursor description, and Superset fails on it (reported upstream as CollierKing/sqlalchemy-cloudflare-d1#32).
So the choice is between losing a leading tag and the query failing. Dropping the comment keeps the query working. Comments elsewhere in the statement are kept, and SQL_QUERY_MUTATOR is off by default, so this only affects deployments that prepend tags.
Once the driver reports column names for every statement, this override can go and leading comments will reach D1 again. Alternatively I can move leading comments to the end of the statement instead of dropping them.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag removing leading SQL comments before D1 execution; the sqlalchemy-cloudflare-d1 driver requires statements to start with SELECT, PRAGMA, or WITH to provide cursor column names, and preserving the comments would break query execution.
Applied to:
superset/db_engine_specs/d1.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
|
The flagged issue is correct. The |
|
@rusackas, you wrote @villebro, since you are picking fixes into |
Code Review Agent Run #cf8dc1Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
The shared parse.py line changes no security verdict over 50 statement shapes (mutation detection strictly improves, no table reference lost), and all four master merges are byte-identical to a clean auto-merge. Four inlines; the base_url guard's only test bypasses its own lookup.
| default_driver = "httpx" | ||
|
|
||
| # The driver sends the API token to the host named by ``base_url`` | ||
| disallow_uri_query_params = {"httpx": {"base_url"}} |
There was a problem hiding this comment.
get_driver_name() splits on + only for d1+httpx://; for the bare d1:// form that metadata["connection_string"] documents it imports the dialect and returns dialect.driver. Measured: with a dialect reporting any other name, ?base_url= is allowed while this +httpx test still passes.
| rows without a cursor description. Every query path goes through here: | ||
| SQL Lab, datasets made from SQL, charts and alerts. | ||
| """ | ||
| stripped = LEADING_COMMENTS_REGEX.sub("", query, count=1) |
There was a problem hiding this comment.
base.py:2658 get_column_description_retry_sql documents this exact driver limitation, and clickhouse.py:658 answers it by wrapping the untouched SQL rather than deleting the comment. That hook only covers get_columns_description, but your offered reposition would keep the tag on every path.
| COLUMN_DOES_NOT_EXIST_REGEX = re.compile( | ||
| r"no such column: (?P<column_name>.+?)(?: at offset \d+)?(?:: SQLITE_\w+)?$" | ||
| ) |
There was a problem hiding this comment.
$ without re.MULTILINE makes this narrower than the parent it overrides: SqliteEngineSpec's pattern maps no such column: x\nsecond line to x, this one does not match at all, so a multi-line body falls back to the generic error.
| COLUMN_DOES_NOT_EXIST_REGEX = re.compile( | |
| r"no such column: (?P<column_name>.+?)(?: at offset \d+)?(?:: SQLITE_\w+)?$" | |
| ) | |
| COLUMN_DOES_NOT_EXIST_REGEX = re.compile( | |
| r"no such column: (?P<column_name>.+?)(?: at offset \d+)?(?:: SQLITE_\w+)?$", | |
| re.MULTILINE, | |
| ) |
| return message | ||
|
|
||
| errors = reply.get("errors") if isinstance(reply, dict) else None | ||
| if isinstance(errors, list) and errors and isinstance(errors[0], dict): |
There was a problem hiding this comment.
Mutation over the 35 new tests: dropping isinstance(errors, list), the errors truthiness, isinstance(errors[0], dict), or or message each leaves the suite at 35 passed. The first three make this method raise on a malformed reply, inside error handling.
This branch has not been deployed
3 participants
SUMMARY
The
d1extra is one of the connector extras still held back from SQLAlchemy 2.UPDATING.mdlists it under "the upstream dialect package has no SQLAlchemy 2.0 support yet at all", andsuperset/db_engine_specs/d1.pycarries aknown_incompatibilitiesentry whosetracking_urlpoints at our repo. This PR lifts that block. I am one of the maintainers of thesqlalchemy-cf-d1packages.What changed on our side:
sqlalchemy-d10.2.0 supports SQLAlchemy 2. It keeps thed1://scheme and is a thin layer over the community dialectsqlalchemy-cloudflare-d1. It is on PyPI: sqlalchemy-d1 0.2.0.superset-engine-d1anddbapi-d1are retired. The engine spec of record is the one in this repo. It has taken priority over the external one ford1://since 6.1.0.sqlalchemy-d10.2.0 gets its DBAPI fromsqlalchemy-cloudflare-d1, so nothing needsdbapi-d1any more.What this PR changes:
pyproject.toml: thed1extra installssqlalchemy-d1>=0.2.0, <1only.superset/db_engine_specs/d1.py:pypi_packagesnamessqlalchemy-d1, and theknown_incompatibilitiesentry is removed.install_instructionsispip install "apache-superset[d1]"with aversion_requirementsnote, as fix(presto): require PyHive 0.7 for SQLAlchemy 2 #44285 did for Presto. A barepip install sqlalchemy-d1would call the requirement met on an environment that still has 0.1.0.executeoverride drops comments ahead of a statement. The DBAPI insqlalchemy-cloudflare-d1only reports column names when the statement text starts withSELECT,PRAGMAorWITH, so a query with a leading comment came back as rows without a cursor description and Superset failed on it.allows_sql_comments = Falsewas not enough, because only the SQL Lab executor reads it. Every query path goes throughexecute: SQL Lab, datasets made from SQL, charts, alerts, and comments added bySQL_QUERY_MUTATOR. Comments elsewhere in the statement are left alone.disallow_uri_query_paramsrefusesbase_urlin the connection URI, the waymysql.pyrefuseslocal_infile. The driver sends the API token to the host thatbase_urlnames.We can't seem to resolve the column "nope at offset 9: SQLITE_ERROR"}]}". It now readsWe can't seem to resolve the column "nope". The message text is the one SQLite already uses, somessages.potdoes not change.supports_file_upload = False. D1 has no transactions, so an upload that fails partway leaves the rows already written, and nothing undoes them.default_driverishttpx, the driver name of thed1dialect insqlalchemy-d10.2.0. It wasd1.convert_dttmwrites midnight as a bare date forDATEcolumns. D1 keeps aDATEas text such as2026-09-20and compares it as text, and the inherited literal'2026-09-20 00:00:00.000000'sorts after that day, so a time range on aDATEcolumn started and ended one day late: 09-20 to 09-22 returned 09-21 and 09-22.sqlalchemy-d10.2.0 marksDATEcolumns temporal on its own, so these filters apply by default. A time other than midnight keeps its time part, which sorts between two days, so each day still counts as its midnight.SqliteEngineSpechas the same problem on plain SQLite; that is left for a separate PR.superset/sql/parse.py:d1maps to sqlglot's SQLite dialect. With the generic dialect, bracket identifiers and blob literals fail to parse, anddata ->> '$.a'is rewritten toJSON_EXTRACT_SCALAR(data, '$.a'), which D1 does not have.UPDATING.md:d1is taken off the list of blocked extras, and a note tells existing installs to uninstall the two retired packages and that file upload is now off for D1.docs/src/data/databases.json: the matching lines for D1, edited by hand as fix(docs): use modern driver package for CrateDB #42125 and fix(presto): require PyHive 0.7 for SQLAlchemy 2 #44285 did. The driver, file upload and score lines match whatgenerate_yaml_docs()gives for the new spec.tests/unit_tests/db_engine_specs/test_d1.py: new. Covers theexecuteoverride, thebase_urlcheck, the metadata, the error messages, the driver and file upload settings, SQL parsed as SQLite, time filters onDATEcolumns (run on an in-memory SQLite), and that the type names the dialect reports (DATETIME,TIMESTAMP,DATE,TIME,BOOLEAN) map to temporal and boolean columns.D1 keeps internal
_cf_*tables (for example_cf_KV) in every database. Thesqlalchemy-d1dialect hides them, so the spec does not need to.Existing
d1://connections keep working. The connection string does not change.Request: please consider this for 7.0. The
7.0branch is the first on SQLAlchemy 2 and still lists all three packages in thed1extra.superset-engine-d10.1.0 anddbapi-d10.1.0 both require Python below 3.12, so without this changepip install apache-superset[d1]on 7.0 only works on Python 3.11. The change is small and cherry-picks onto7.0with one trivial conflict insuperset/sql/parse.py: 7.0 has# "databend": ???on the line after the new"d1"entry, where master has the Databend dialect. Keep 7.0's line and add"d1": Dialects.SQLITE,above it. With that, the D1 tests pass on7.0(checked against78611f6c0b). If a committer agrees, av7.0label would be appreciated.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
1. Test connection succeeds for a
d1://database (connection string blurred).2. SQL Lab lists the tables and the view but not
_cf_KV, and aDATETIMEcolumn holding a number, an empty string and09/21/2026still previews.3. A dataset from a D1 table marks its
DATETIMEandDATEcolumns temporal on its own.4. A line chart with a daily time grain.
TESTING INSTRUCTIONS
Unit tests:
By hand, against a real D1 database:
pip install "apache-superset[d1]", orpip install "sqlalchemy-d1>=0.2.0"into an existing Superset environment. Do not installsuperset-engine-d1ordbapi-d1.d1://{cloudflare_account_id}:{cloudflare_api_token}@{cloudflare_d1_database_id}. Test connection succeeds.SELECT * FROM <table> WHERE 1=0returns the column headers and no rows.DATETIMEandDATEcolumns. Both are marked temporal. A line chart with a daily time grain renders._cf_KVis not in the table list.-- commentline runs and shows its columns.SELECT nope FROM <table>saysWe can't seem to resolve the column "nope".DATEcolumn returns the days in the range: 09-20 to 09-22 gives 09-20 and 09-21.Results. I reran the list on 2026-09-24 at the last commit (
8072d71) against a real D1 database, withsqlalchemy-d10.2.0 installed from PyPI (SQLAlchemy 2.0.52,sqlalchemy-cloudflare-d10.4.0) and neither retired package, on Python 3.12, driving Superset through its REST API. The first run, on 2026-09-21 at3838685, went through the UI in thedocker-compose-light.ymlstack on Python 3.11.14; the screenshots above are from it.get_engine_spec("d1")returnsCloudflareD1EngineSpec, no spec load warnings in the loghttpxand file upload not supported?base_url=in the URI (HTTP 422)SELECT 1--or a/* */comment runs and returns its columnsSELECT nope FROM <table>saysWe can't seem to resolve the column "nope"SELECT * FROM nopesaysd1 error: no such table: nope: SQLITE_ERROR, without the JSON reply->>,[bracket]identifiers andIIFrunDATETIMEcolumn holds a number, an empty string and09/21/2026still previews_cf_KVDATETIMEandDATEcolumns temporal on its ownDATETIMEcolumnDATEcolumn, from midnight to midnight and from noon to noonDATEcolumnADDITIONAL INFORMATION