Skip Dag params with no value when forwarding them to Databricks - #71782
Skip Dag params with no value when forwarding them to Databricks#71782rjgoyln wants to merge 2 commits into
Conversation
A Dag that declares a nullable Param and leaves it unset could no longer submit a Databricks run: the auto-forwarding added for job parameters put the None into the payload, where it either failed local payload validation or reached the API as null. A param without a value is nothing to forward, so leaving it out is what the author asked for.
|
cc @moomindani for review |
moomindani
left a comment
There was a problem hiding this comment.
LGTM — approving. The fix is minimal, applied consistently to all three forwarding paths, and the
tests pin exactly the behaviour that changes. Verified rather than read:
- The six new tests all fail on the pre-fix code and pass on
e2a90690. On
DatabricksSubmitRunOperatorthe pre-fix failure is verbatim the report in #71776:
AirflowException: Type <class 'NoneType'> used for parameter json[python_wheel_task][named_parameters][start_date_str] is not a number or a string.
All 250 tests in the operators file pass on this head. - The asymmetry the description describes is real:
DatabricksCreateJobsOperator.executenormalises
at:582and injects at:586, andDatabricksRunNowOperator._build_run_now_payloadnormalises at
:1375and injects at:1390, so in those two theNoneskipped local validation entirely, while
DatabricksSubmitRunOperatorinjects at:932ahead of thenormalise_json_contentat:945.
One correction for the PR body, because the case is stronger than it currently reads. "The None
reached the Databricks API as null instead of failing locally" can be read as "and Databricks
accepted it". It does not. I sent both pre-fix shapes at a real workspace (throwaway job, deleted
afterwards):
POST /api/2.2/jobs/createwithparameters: [{"name": "probe_null", "default": null}]→
Job–level parameters 'probe_null' is missing default value.POST /api/2.2/jobs/run-nowwithjob_parameters: {"probe_null": null}→
Could not parse request object: Expected both 'key' and 'value' to be set on elements in the field 'job_parameters'- The same two calls with a string value succeed (control).
So all three operators were genuinely broken by an unset nullable Param; the only difference was
whether the user got the local AirflowException or an opaque API error. Worth a sentence, since it
also settles that nobody could have been relying on the null being forwarded.
The fixed side is checked against the same workspace, not only against mocks: I had the post-fix
operators build the payloads (_prepare_submit_json and _build_run_now_payload, real code, hook
mocked only as transport) and sent those exact payloads to the live API.
runs/submitaccepted it, and the run came back withbase_parameters: {"env": "prod"}— the null
param gone, the other one forwarded.run-nowaccepted it, and the run'sjob_parameterscame back as
[{"default": "job_default_env", "name": "env", "value": "prod"}, {"default": "job_default_date", "name": "start_date_str"}]
— the skipped param carries novalue, so the job-level default applies to it. That is the behaviour
I would want from "there is no value to forward", now observed rather than assumed.
On whether this loosens payload validation: it does not, and I think the split is the right one.
Measured on this head, an explicit json={"notebook_task": {"base_parameters": {"x": None}}} still
raises AirflowException; only auto-forwarded params are dropped. Auto-forwarding is something the
operator does on the user's behalf, so it should degrade quietly, whereas a null the user wrote
themselves is a request Databricks cannot express and deserves the error. Dropping None while
building a provider payload is also an established Airflow idiom — core's prune_dict
(utils/helpers.py:259), amazon's trim_none_values, and this provider already does the same inline
in hooks/databricks_sql.py:282.
Three line-level notes left inline: one to protect the helper from a future "simplification", one
question about the jobs/create case, and one doc-wording nit.
Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting
The docs described the skipped params as nullable Params left unset, but a param the user explicitly passes as null in the trigger conf resolves the same way and is skipped too. The comment on the helper now also records why the params have to be read through dict(), since reading them any other way would silently restore the bug this guards against.
Thanks for the thorough review, and especially for testing the payloads against the actual Databricks API. That was very helpful! Here’s how I addressed the points:
That said, I’m happy to switch to |
Summary
A Dag that declares a nullable
Paramand leaves it unset can no longer submit a Databricks run: the params are auto-forwarded into each task's dict-shaped parameter slot, and theNonefails payload validation before any API call. A param with no value is nothing to forward, so it is now skipped.DatabricksRunNowOperatorandDatabricksCreateJobsOperatorforward Dag params the same way but inject after validation, so there theNonereached the API, which rejects it outright. All three were broken by an unset nullableParam— only the shape of the failure differed — and all three now filter identically.closes: #71776
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines