pankajastro commented on code in PR #73273:
URL: https://github.com/apache/airflow/pull/73273#discussion_r4047093849
##########
providers/common/sql/src/airflow/providers/common/sql/config.py:
##########
@@ -88,16 +91,23 @@ def is_table_provider(self) -> bool:
return bool(self.format and self.format.lower() in TABLE_PROVIDERS)
def __post_init__(self):
+ if not self.table_name or not self.table_name.strip():
+ raise ValueError("Table name must be provided for storage type")
+
if self.is_table_provider:
if self.db_name is None:
raise ValueError(f"Database name must be provided for table
providers {TABLE_PROVIDERS}")
return
+ if not self.format and not self.uri and self.storage_type is None:
+ # Plain database table: no object store involved, so storage_type
stays unset.
+ return
+
if self.storage_type is None:
self.storage_type = self._extract_storage_type
- if self.storage_type is not None and (not self.table_name or not
self.table_name.strip()):
- raise ValueError("Table name must be provided for storage type")
+ if not self.uri:
+ raise ValueError("URI must be provided when storage_type is set")
Review Comment:
Good catch — applied your suggestion (keyed on `format`, moved above the
inference, dropped `storage_type is None` from the early return). Restores the
explicit-`storage_type` workaround and fixes the unreachable message. Updated
`test_format_handlers.py` and two of my own tests to match. On the
iceberg-blank-`table_name` point: intentional, not incidental — closes the same
class of gap, covered by `test_iceberg_with_blank_table_name_raises_error`.
---
Drafted-by: Claude Code (Sonnet 5); reviewed by @pankajastro before posting
##########
providers/common/ai/docs/operators/llm_schema_compare.rst:
##########
@@ -63,7 +63,10 @@ With Object Storage
Use ``data_sources`` with
:class:`~airflow.providers.common.sql.config.DataSourceConfig` to include
object-storage sources (S3 Parquet, CSV, Iceberg, etc.) in the comparison.
-These can be freely combined with ``db_conn_ids``:
+These can be freely combined with ``db_conn_ids``. A ``DataSourceConfig``
+with neither ``uri`` nor ``format`` set is introspected via ``DbApiHook``
Review Comment:
Fair point — reworded to state the actual rule (routing depends on what
`conn_id` resolves to, not on `uri`/`format`), updated the heading, the
`data_sources` bullet, and the example DAG's comment to match.
---
Drafted-by: Claude Code (Sonnet 5); reviewed by @pankajastro before posting
--
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]
