kaxil commented on PR #73273:
URL: https://github.com/apache/airflow/pull/73273#issuecomment-5714529233
Ran this end to end against a real Postgres backend in breeze: two real
tables, a real Airflow connection, and each consumer of a plain-database
`DataSourceConfig`.
The feature itself works. `LLMSchemaCompareOperator` now introspects a plain
table through `DbApiHook` and returns real schema text:
```
Source: pg_e2e (postgresql)
Table: e2e_orders_v1
Columns: order_id INTEGER, customer VARCHAR(64), amount NUMERIC(10, 2),
created_at TIMESTAMP
Index: e2e_orders_v1_customer_idx (customer)
```
That path was unreachable before, since the config could not be constructed,
so the `DbApiHook` branch in `_introspect_datasource_schema` was dead code.
Worth correcting in the description though: the operator does not build the
config, the caller passes it in `data_sources`, and nothing in the repo
constructs a plain-database one yet.
Where it needs another look is `table_name` validation. Same script, before
and after moving the check above the new early return:
| case | before | after |
| --- | --- | --- |
| plain DB, valid `table_name` | OK, `storage_type=None` | OK,
`storage_type=None` |
| plain DB, blank `table_name` | accepted | raises |
| plain DB, whitespace `table_name` | accepted | raises |
| explicit `storage_type=S3`, blank `table_name`, no `uri` | accepted |
raises |
| `uri` set, blank `table_name` | raises | raises |
Row three is a behaviour change against current main, which raises there
today. Row two costs a worse error at task runtime: `hook.get_table_schema("")`
surfaces a bare `NoSuchTableError` with an empty message, where the check would
have said `Table name must be provided` at construction. I left a committable
suggestion on the diff for it.
With the check hoisted, the full `common.sql` and `common.ai` unit suites
pass (2084 tests), and both existing assertions on that message, in
`test_config.py` and `test_format_handlers.py`, keep working.
On docs: `llm_schema_compare.rst` still scopes `data_sources` to
object-storage and catalog-managed sources, so the new shape has no example
anywhere. I have a short section written that frames it as the way to compare
differently named tables, since `db_conn_ids` and `table_names` form a
cross-product and cannot pair `orders` with `orders_v2`. Happy to hand it over
for this PR or push it as a follow-up, whichever you prefer.
Separately, and for a follow-up rather than this PR: three of the four
`register_datasource` callers do not gate on connection kind, so a
plain-database config reaching `LLMSQLOperator`, `DataFusionToolset` or
`AnalyticsOperator` fails with `ValueError: Unknown connection type postgres`
from `_get_credentials`. Only `LLMSchemaCompareOperator` checks
`_is_dbapi_connection` first.
--
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]
