Skip to content

Reject a DataSourceConfig with no uri or format in DataFusionEngine - #73287

Merged
potiuk merged 1 commit into
apache:mainfrom
pankajastro:datafusion-register-datasource-guard-afl-214
Sep 20, 2026
Merged

potiuk merged 1 commit into
apache:mainfrom
pankajastro:datafusion-register-datasource-guard-afl-214

Conversation

@pankajastro

@pankajastro pankajastro commented Sep 17, 2026 •

Copy link
Copy Markdown
Member

Follow-up per review feedback on #73273.

LLMSQLOperator, DataFusionToolset, and AnalyticsOperator hand DataFusionEngine.register_datasource whatever DataSourceConfig the caller supplies, unlike LLMSchemaCompareOperator which checks the connection kind first. A plain-database-table config reaching any of them failed deep inside credential resolution with a confusing Unknown connection type error instead of a clear message. register_datasource now rejects that shape immediately.


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Sonnet 5)

Generated-by: Claude Code (Sonnet 5) following the guidelines

LLMSQLOperator, DataFusionToolset, and AnalyticsOperator hand
DataFusionEngine.register_datasource whatever DataSourceConfig the
caller supplies, unlike LLMSchemaCompareOperator which checks the
connection kind first. A plain-database-table config reaching any of
them failed deep inside credential resolution with a confusing
"Unknown connection type" error instead of a clear message.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@pankajastro
pankajastro force-pushed the datafusion-register-datasource-guard-afl-214 branch from 793bcac to 8f3a1c3 Compare September 19, 2026 04:30
@pankajastro
pankajastro marked this pull request as ready for review September 19, 2026 04:59
@pankajastro
pankajastro requested a review from kaxil September 19, 2026 04:59

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guard is correct and lands exactly where it should: storage_type is None on a non-table-provider config is, by DataSourceConfig.__post_init__'s invariants, exactly the "neither uri nor format" shape, so the check turns the misleading Unknown connection type failure from _get_credentials into a clear error before any connection lookup, and the new test fails without it.

I traced the three pass-through callers (LLMSQLOperator, DataFusionToolset, AnalyticsOperator) and every DataSourceConfig(...) in the common-ai / common-sql tests; none hands a plain-table config to a real engine, and the existing docs already describe uri/format as required for those consumers, so nothing else changes.

Smaller observations

  • The commit carries a Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> trailer. AGENTS.md ("Commits and PRs") says "Never list an agent as a commit co-author"; the Generated-by: line in the description is the sanctioned disclosure. I'll drop the trailer from the squash message at merge time.
  • For LLMSchemaCompareOperator, a plain-table config whose conn_id is mistyped now reports "has no uri or format" instead of "conn_id isn't defined" (because _is_dbapi_connection swallows the not-found error and falls through to DataFusion). Probably the right layering; just noting the message no longer points at the real cause in that one case.

This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.

More on how Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.

@potiuk
potiuk merged commit aa4aeef into apache:main Sep 20, 2026
172 of 185 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants