Yanislava opened a new pull request, #76804:
URL: https://github.com/apache/airflow/pull/76804

   ## Problem
   
   `VerticaHook.sqlalchemy_url` (added in #55928) builds the URL with the 
`vertica-python` drivername, but no Vertica SQLAlchemy dialect registers that 
name: `vertica-sqlalchemy-dialect`, `sqlalchemy-vertica` and 
`sqlalchemy-vertica-python` all declare the `sqlalchemy.dialects` entry point 
`vertica.vertica_python` (scheme `vertica+vertica_python`). As a result:
   
   - `get_sqlalchemy_engine()` raises `NoSuchModuleError: Can't load plugin: 
sqlalchemy.dialects:vertica-python` even with a dialect installed, so 
`DataFrame.to_sql` with the hook's engine, `inspector` and `get_table_schema()` 
don't work;
   - `get_uri()` returns `vertica-python://...`, which `make_url()` cannot 
parse (`-` is not allowed in the scheme), so `dialect_name` silently falls back 
to `"default"`.
   
   The existing test mocks `create_engine` and only asserts the drivername 
string, so it didn't catch this. #60177 (closing #59908) made SQLAlchemy an 
optional extra, but the engine still could not be created.
   
   ## Changes
   
   - Use `vertica+vertica_python` in `sqlalchemy_url` (and so in `get_uri()`).
   - Regression tests without mocking `create_engine`: `get_uri()` parses with 
`make_url()`, `get_sqlalchemy_engine()` creates an engine through a dialect 
registered as `vertica.vertica_python`, and `dialect_name` is `"vertica"`.
   - Document in the connection docs which dialect works with SQLAlchemy 2.x.
   
   ## Verification on a real Vertica
   
   Airflow 3, provider 4.4.0, SQLAlchemy 2.0.49, vertica-python 1.4.0, pandas 
2.3.3, everything through `VerticaHook.get_sqlalchemy_engine()`:
   
   | check | current provider | with this change + `sqlalchemy-vertica` 1.0.2 |
   |---|---|---|
   | create engine, `SELECT 1` | `NoSuchModuleError` | ok |
   | `dialect_name` | `"default"` | `"vertica"` |
   | `inspector.get_schema_names()` | fails | ok |
   | `DataFrame.to_sql` + read back | fails | ok |
   | `session_label` extra reaches the server (`GET_CLIENT_LABEL()`) | fails | 
ok |
   
   Which dialect works with SQLAlchemy 2.x (checked with 
`vertica+vertica_python` on the same Vertica):
   
   | dialect | SELECT 1 | inspector | `DataFrame.to_sql` |
   |---|---|---|---|
   | `vertica-sqlalchemy-dialect` 0.0.8.2 (official) | fails: pins 
`sqlalchemy<=1.4.44`, `ObjectNotExecutableError` on SQLAlchemy 2 | – | – |
   | `sqlalchemy-vertica` 1.0.2 | ok | ok | ok |
   | `sqlalchemy-vertica-python` 0.6.3 | ok | ok | fails: `has_table() got an 
unexpected keyword argument 'info_cache'` |
   
   ## Notes for reviewers
   
   - **Dependency question:** should `sqlalchemy-vertica` be added to the 
`sqlalchemy` extra? It is the only dialect that worked with SQLAlchemy 2.x, but 
it requires `sqlalchemy>=2.0`, while the extra allows SQLAlchemy 1.4 and the 
provider still supports Airflow 2.11. I left the extra unchanged and documented 
the dialect instead; happy to change it either way.
   - **Not in this PR:** connection extras are passed as URL query strings, and 
the dialect converts only some of them (`connection_timeout`, 
`connection_load_balance`), so e.g. `{"log_level": "error"}` still fails 
through the engine with `ValueError: Unknown level: 'error'` (`get_conn()` is 
not affected). I can follow up by passing only driver-supported extras with 
proper types, reusing the `get_conn()` mapping.
   
   related: #55928, #59908, #60177
   
   ---
   
   ##### Was generative AI tooling used to co-author this PR?
   
   - [X] Yes (please specify the tool below)
   
   Generated-by: Claude Code following [the 
guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to