Validate DatabricksSQLStatementsSensor exclusivity at __init__

[ahilashsasidharan]

Description

This PR moves template_field exclusivity check back to __init__ from execute() while keeping the not statement and not statement_id sibling in execute() since it is a required-argument check (see #70503) for the DatabricksSQLStatementsSensor operators in the providers/databricks/src/airflow/providers/databricks/sensors/databricks.py.

The check is updated to use is not None polarity rather than a truthiness check, so this is not a straight revert of the original code.

These are pure provision checks ("was this argument passed?") which belong in __init__ per the updated guidance in #70296, as moving them to execute() risks false positives when render_template_as_native_obj=True renders a provided field to None.

Following the logic of "you're already touching this function" argument the PR also converts 3 AirflowExceptions to ValueErrors under both __init__ and execute() to increase consistency and follow guidance to narrow when changing a line.

Tests

• Separated tests under providers/databricks/tests/unit/databricks/sensors/test_databricks.py added as part of the previous PR Validate DatabricksSQLStatementsSensor statement fields after rendering #70340 to validate the new behaviour.
• Added parameterised testcases to cover the specific reasons the is not None polarity is used in __init__ while the is None polarity is not used in execute() for the sibling testcase.

related: #70296 and #70503

---

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Generated-by: [Antigravity IDE] following the guidelines

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[ahilashsasidharan]

Question: Could the AirflowException for both checks mentioned in this PR be converted to ValueError raises as part of this PR or should they remain as is?

[potiuk]

This gets the split right, which is worth saying explicitly because several PRs in this series have not.

The "both provided" case is a provision check — passing both statement and statement_id is a Dag-authoring error whatever they render to — so __init__ with is not None polarity is the correct home, and that polarity also catches statement_id="", which a truthiness check would let through.

Leaving the "neither resolves" case in execute() is right too, and I checked rather than assumed: template_fields includes both statement and statement_id, so statement="{{ params.sql }}" rendering to an empty string is only visible after rendering. __init__ genuinely cannot catch that one. The asymmetry is correct, not an oversight.

Tests are split to match, which reads well.

One thing before merge. The relocated line still raises AirflowException:

ifstatementisnotNoneandstatement_idisnotNone:
raiseAirflowException("Cannot provide both statement and statement_id.")

Moving one verbatim during a refactor is explicitly allowed, and the file's ratchet entry stays at ::4, so nothing is violated as it stands. But the guidance is to narrow when you're already touching the line, and this is an argument-combination error, so ValueError fits — exactly what you did in #70634:

ifstatementisnotNoneandstatement_idisnotNone:
raiseValueError("Cannot provide both statement and statement_id.")

That needs the test's pytest.raises(AirflowException, ...) updated to ValueError, and generated/known_airflow_exceptions.txt dropped from providers/databricks/src/airflow/providers/databricks/sensors/databricks.py::4 to ::3.

Happy to merge once that's in.

---

Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

[shahar1]

The split is right and the follow-up commit lands everything that was asked for — ValueError, the updated pytest.raises, and the ratchet drop to ::3. Two observations inline and one process note below; none are blockers.

I checked a few things rather than assuming them, and they hold up:

• #70340 (which moved the check into execute()) landed after the last provider release prep, so it is unreleased. Against released 7.18.0 this PR is a net no-op on placement — the genuinely new behaviour is the polarity and the exception type, which matches how the description frames it.
• Pre-#70340 the __init__ check was if statement and statement_id raising AirflowException. So this really isn't a straight revert, as stated.
• DatabricksSQLStatementsOperator takes statement: str as required and does not list it in template_fields, so there is no parallel site needing the same treatment. Nothing sibling is left half-fixed.

Process — the PR title ships into the changelog verbatim

The title currently ends in a literal ellipsis:

Rewrite DatabricksSQLStatementsSensor exclusivity check and move back…

Databricks changelogs are regenerated from git log by the release manager rather than from newsfragments (see providers/AGENTS.md), so the truncation would land as-is in providers/databricks/docs/changelog.rst. Worth retitling to something that stands on its own and stays under 70 characters — for example:

Validate DatabricksSQLStatementsSensor exclusivity at __init__

Please retitle the commit as well as the PR, since the commit message is what the changelog is generated from.

On your question about the remaining AirflowExceptions

Could the AirflowException for both checks mentioned in this PR be converted to ValueError raises as part of this PR or should they remain as is?

Both readings are defensible, and the ratchet permits either. Neither warehouse_id must be provided. nor One of either statement or statement_id must be provided. is modified by this diff, so leaving them is not a violation — the check-no-new-airflow-exceptions hook only guards new usages.

That said, I'd convert them here. The same "you're already touching this function" argument that applied to the relocated line applies to its two neighbours, and the current state leaves an odd surface for anyone writing an except block: passing both statement and statement_id raises ValueError, while passing neither raises AirflowException, for what a Dag author experiences as the same class of mistake. Both are argument-provision errors and ValueError fits both. That would take the ratchet entry from ::3 to ::1.

If you'd rather keep this PR tightly scoped to what was asked for, a follow-up is fine too — just say which way you're going so the ratchet line doesn't get churned twice.

---

This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The findings
below are observations, not blockers; an Apache Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.

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

---

Drafted-by: Claude Code (Opus 5); reviewed by @shahar1 before posting

[ahilashsasidharan]

Please retitle the commit as well as the PR, since the commit message is what the changelog is generated from.

I’ve updated the PR title and rewritten the first commit message to the suggested form. My understanding is that this is the commit whose message is used when generating the provider changelog, so that’s the one I corrected.

If you'd rather keep this PR tightly scoped to what was asked for, a follow-up is fine too — just say which way you're going so the ratchet line doesn't get churned twice.

I’ve opted to make the additional changes in this PR. Since the diff already touches this function, it felt cleaner to apply the consistent ValueError treatment now rather than opening a follow‑up PR.

[ahilashsasidharan]

Follow up question for some of my related commits in this series:

#70874 & #70634

1. Both make similar changes to different provider files but do not have testcases that fail without the PR and emphasise the specific logic for each check being written as it is (i.e. with is not None or without is None). Is it worth going back to add additional parameterised testcases for these?

2. Should these also have a comments to highlight the split between sibling validation to avoid future confusion by readers?

[ahilashsasidharan]

@shahar1 pinging since this should be ready for a re-review when you have the chance

[eladkal]

cc @moomindani for review

[moomindani]

Thanks for the clear write-up — the reasoning matches the criterion in #70503 exactly, and it follows the same restoration already merged for the Repos operators in #70551.

I ran a differential against a live SQL warehouse. Provider 7.18.1 already contains #70340, so the unpatched wheel reproduces current main and the patched one carries your two edits (Airflow 3.2.2, real warehouse_id, real statements executed):

case	current main	with this PR
statement + templated statement_id rendering to None (render_template_as_native_obj=True)	task succeeds — the contradictory pair is silently ignored and the statement runs	Dag import error
statement="SELECT 1", statement_id=""	task succeeds against the warehouse	Dag import error
statement="", statement_id=""	execute(): "One of either statement or statement_id must be provided."	__init__: "Cannot provide both statement and statement_id."
statement only / statement_id only	constructed	constructed

The first row is an argument for this change that the description does not make: on main, supplying both arguments produces no error at all once one of them renders to None, and the sensor quietly runs the other. Catching that at parse time is a real improvement rather than a relocation — worth saying in the description.

The second row is a user-visible behaviour change: statement_id="" alongside a statement works today (confirmed end to end) and becomes a Dag import error here. Your parametrization shows it is deliberate, so this is only about making it visible to whoever reads the changelog later.

Nits inline, none blocking.

---

Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting

[moomindani]

With both fields "" this raises "Cannot provide both" even though nothing was really provided — measured on the patched build, and main reports the accurate "One of either statement or statement_id must be provided." from execute() for the same input.

The is not None polarity is right and I would not weaken it; the cheapest fix is a message that holds for either failure, e.g. "Provide exactly one of statement or statement_id."

[moomindani]

Worth adding ("", "") here so whichever behaviour you settle on for the both-empty case is pinned down — right now nothing covers it.

[moomindani]

The case that actually motivates the polarity change is not covered anywhere: a provided field that renders to None under render_template_as_native_obj=True, which must not slip past the exclusivity check. Without it, a later well-meaning move back to a truthiness check in execute() would pass CI.

[eladkal]

@ahilashsasidharan can you fix the open items?

[ahilashsasidharan]

@ahilashsasidharan can you fix the open items?

Sorry for the delay. Will work on these today. running late will take a look tomorrow.