rjgoyln commented on PR #71782:
URL: https://github.com/apache/airflow/pull/71782#issuecomment-5380889189
> 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
> `DatabricksSubmitRunOperator` the 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.execute` normalises
> at `:582` and injects at `:586`, and
`DatabricksRunNowOperator._build_run_now_payload` normalises at
> `:1375` and injects at `:1390`, so in those two the `None` skipped local
validation entirely, while
> `DatabricksSubmitRunOperator` injects at `:932` ahead of the
`normalise_json_content` at `: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/create` with `parameters: [{"name": "probe_null",
"default": null}]` →
> `Job–level parameters 'probe_null' is missing default value.`
> * `POST /api/2.2/jobs/run-now` with `job_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/submit` accepted it, and the run came back with `base_parameters:
{"env": "prod"}` — the null
> param gone, the other one forwarded.
> * `run-now` accepted it, and the run's `job_parameters` came back as
> `[{"default": "job_default_env", "name": "env", "value": "prod"},
{"default": "job_default_date", "name": "start_date_str"}]`
> — the skipped param carries no `value`, 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
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:
* **Docs/Wording** — Fixed in 350955d across all three pages. `resolves to
None` is more accurate for both explicit `null` and `NOTSET`.
* **`dict(params)` & nested `None`** — Added a comment explaining the
`dict()` round-trip. I agree that handling nested `None` is better left out of
this PR, since Databricks expects `Dict[str, str]` here.
* **CreateJobs** — I kept the `skip` behavior to avoid silently turning an
unset value into an empty string. Since the job definition is rebuilt from JSON
on each run, I also prefer keeping the behavior consistent across all three
operators.
That said, I’m happy to switch to `default: ""` if you think that would be
safer. Thanks again for the great review!
--
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]
