Rewrite DatabricksSQLStatementsSensor exclusivity check and move back…

[ahilashsasidharan]

… to init

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.

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.

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:

if statement is not None and statement_id is not None:
    raise AirflowException("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:

if statement is not None and statement_id is not None:
    raise ValueError("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

[shahar1]

The is not None polarity is the headline of this PR, and nothing here exercises it. STATEMENT and STATEMENT_ID are both non-empty strings, so a plain if statement and statement_id: truthiness check passes this test identically — meaning the test does not fail without the change it is meant to cover, which is the bar in Testing Standards ("every test must fail without the PR's change").

The distinguishing case is the empty string — exactly the one called out as the reason for the polarity in the first place. Adding it back as a parametrize keeps the split-by-lifecycle naming intact:

Suggested change

	def test_both_statements_included_validated_at_init(self):
	with pytest.raises(ValueError, match="Cannot provide both"):
	DatabricksSQLStatementsSensor(
	statement=STATEMENT,
	statement_id=STATEMENT_ID,
	task_id=TASK_ID,
	warehouse_id=WAREHOUSE_ID,
	)
	@pytest.mark.parametrize(
	("statement", "statement_id"),
	[
	(STATEMENT, STATEMENT_ID),
	(STATEMENT, ""),
	],
	)
	def test_both_statements_included_validated_at_init(self, statement, statement_id):
	with pytest.raises(ValueError, match="Cannot provide both"):
	DatabricksSQLStatementsSensor(
	statement=statement,
	statement_id=statement_id,
	task_id=TASK_ID,
	warehouse_id=WAREHOUSE_ID,
	)

The second case fails on main and passes with this PR, which is the property that was missing.

[shahar1]

Minor, non-blocking. This comment and its counterpart above the __init__ check both restate the condition on the line beneath them, which AGENTS.md asks us to avoid ("do not write narrating comments that restate the next line").

The thing that genuinely warrants a comment here is the part the code cannot say for itself: why one half of this validation lives in __init__ and the other half lives in execute(). That asymmetry is deliberate and load-bearing, and it is precisely what has been getting reverted back and forth across this series — a reader arriving cold will otherwise read it as an oversight and "fix" it again.

Suggested change

	# Handle the scenario where both statement and statement_id are not set
	# Both fields are templated, so "neither resolves to a value" is only knowable
	# after rendering — __init__ cannot catch it. The both-provided case is a pure
	# provision error and is checked there instead.

Dropping the # Handle the scenario where both statement and statement_id are set line above the __init__ check would round this out — with the rationale recorded here, that one carries nothing the if does not already say.