kaxil commented on code in PR #73273:
URL: https://github.com/apache/airflow/pull/73273#discussion_r4036626629
##########
providers/common/sql/src/airflow/providers/common/sql/config.py:
##########
@@ -93,6 +96,10 @@ def __post_init__(self):
raise ValueError(f"Database name must be provided for table
providers {TABLE_PROVIDERS}")
return
+ if not self.format and not self.uri:
Review Comment:
This return sits above the `table_name` check on line 106, so anything
reaching it skips that validation. `DataSourceConfig(conn_id="pg",
table_name="", storage_type=StorageType.S3)` raises `Table name must be
provided for storage type` on main and is accepted here, and a plain DB table
with a blank or whitespace `table_name` gets no check at all, even though that
is the value `_introspect_db_schema` hands to `hook.get_table_schema()`.
Hoisting the `table_name` check above this branch covers both cases; its
`storage_type is not None` guard is already vacuous, since
`_extract_storage_type` either returns a type or raises.
##########
providers/common/sql/src/airflow/providers/common/sql/config.py:
##########
@@ -93,6 +96,10 @@ def __post_init__(self):
raise ValueError(f"Database name must be provided for table
providers {TABLE_PROVIDERS}")
return
+ if not self.format and not self.uri:
+ # Plain database table: no object store involved, so storage_type
stays unset.
Review Comment:
Four call sites hand a user-supplied config to
`DataFusionEngine.register_datasource`, and only `LLMSchemaCompareOperator`
reaches it conditionally, after `_is_dbapi_connection` says no.
`LLMSQLOperator._introspect_object_storage_schema`,
`DataFusionToolset._get_engine` and `AnalyticsOperator` call it
unconditionally, so a plain DB table config there takes the non-LOCAL branch
into `_get_credentials` and fails with `Unknown connection type postgres`.
Would a sibling predicate to `is_table_provider` help, so the engine can reject
the shape it cannot register instead of failing in the credential lookup?
##########
providers/common/sql/src/airflow/providers/common/sql/config.py:
##########
@@ -62,6 +62,9 @@ class DataSourceConfig:
require ``uri`` or ``storage_type``; they use ``conn_id`` and
format-specific
keys in ``options`` (e.g. ``catalog_table_name`` for Iceberg).
+ **Plain database tables** (neither ``uri`` nor ``format`` set) are not
Review Comment:
`docs/operators/llm_schema_compare.rst` still scopes `data_sources` to
object storage (the "With Object Storage" heading at line 60, and line 187's
"for object-storage or catalog-managed sources"), and none of the
`DataSourceConfig` uses in the example DAGs is a plain DB table. The class
docstring is the right place to start, but a reader would not find the new
shape where they actually look.
--
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]
