moomindani commented on code in PR #71782:
URL: https://github.com/apache/airflow/pull/71782#discussion_r3836020963
##########
providers/databricks/docs/operators/submit_run.rst:
##########
@@ -125,6 +125,9 @@ Tasks whose only parameter slot is ``List[str]``
(``spark_jar_task``, ``spark_py
dict to a positional argument list — pass those parameters explicitly via the
``json``
or ``tasks`` argument.
+Params whose value is ``None`` — a nullable ``Param(default=None)`` left unset
— are
Review Comment:
Small wording nit: this is narrower than what the code does. `ParamsDict`
resolves both "explicitly
null" and "never set" (`NOTSET`) to `None`, so the filter also drops a param
a user deliberately passes
as `null` in the trigger conf, not just a nullable `Param` left unset.
"Params whose value resolves to
``None``" would cover both cases — the operator docstrings already put it
that way.
Same phrasing in `run_now.rst:64` and `jobs_create.rst:75`.
---
Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -329,6 +329,13 @@ def
_handle_deferrable_databricks_operator_completion(event: dict, log: Logger)
)
+def _get_forwardable_dag_params(params: Mapping[str, Any]) -> dict[str, Any]:
+ """Return the Dag params that can be forwarded to Databricks."""
+ # A nullable Param left unset has no value to forward, and the Databricks
payload has no
+ # slot for null.
+ return {key: value for key, value in dict(params).items() if value is not
None}
Review Comment:
The `dict(params)` round-trip here is load-bearing, so this is a note for
whoever is tempted to
"simplify" it later rather than a change request: `ParamsDict` is a
`MutableMapping` whose `items()`
yields the raw `Param` objects while `__getitem__` resolves them, so
`dict(params)` is precisely what
turns `Param(None)` into `None`. Iterating `params.items()` directly would
compare `Param` instances
against `None`, keep everything, and silently restore the bug. Measured on
`ParamsDict({"set": Param("v"), "unset": Param(None, type=["null",
"string"])})`: `dict(pd)` →
`{'set': 'v', 'unset': None}`, while `pd.items()` → `[('set', Param),
('unset', Param)]`.
Also worth knowing, though I would not widen this PR for it: the filter is
top-level only, so a param
whose *value* contains a null — `Param({"a": None})` — still reaches the
payload and still fails the
same way (`Type <class 'NoneType'> used for parameter json[cfg][a] is not a
number or a string`). That
is outside what #71776 reports, so leaving it here seems right; flagging it
in case it returns as a
follow-up.
---
Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -575,8 +583,8 @@ def execute(self, context: Context) -> int:
if "name" not in json:
raise AirflowException("Missing required parameter: name")
job_id = self._hook.find_job_id_by_name(json["name"])
- if not json.get("parameters") and self.params:
- json["parameters"] = [{"name": k, "default": v} for k, v in
dict(self.params).items()]
+ if not json.get("parameters") and (forwardable_params :=
_get_forwardable_dag_params(self.params)):
Review Comment:
A question rather than an objection, because this is the one sink where
"skip" answers a different kind
of question than it does in the other two.
For `run-now` and for the `submit-run` task slots, what is forwarded is a
*value*, and omitting a key
means "fall back to the job-level default" (or the notebook widget / wheel
argparse default). Skipping
is then the closest possible match for "there is no value", and strictly
better than sending `""`,
which would override the job's own default with an empty string. Confirmed
on a live run: the payload
this PR produces yields `job_parameters: [{"default": "job_default_env",
"name": "env", "value":
"prod"}, {"default": "job_default_date", "name": "start_date_str"}]`, i.e.
the skipped param falls
through to the job default.
Here what is forwarded is the parameter *definition* — `{"name",
"default"}`, with `default`
mandatory (confirmed: the API rejects `"default": null` with `Job–level
parameters '…' is missing
default value.`). Skipping means the parameter is not defined on the job at
all, so the job's parameter
*schema* ends up depending on whether that Dag param happened to be `None`
at that moment: a run with
it unset resets the job without the parameter, a later run with it set adds
it back. `default: ""`
would keep the schema stable instead.
I could not find a practical consequence — `run-now` accepts a
`job_parameters` key the job does not
define and materialises it with an empty default, verified on a throwaway
job whose run came back with
`[{"default": "d", "name": "defined_param"}, {"default": "", "name":
"not_defined_on_job", "value": "v"}]`
— so I am happy either way. Was the uniform "skip" deliberate for the
definition case too, or would
`default: ""` be a better fit here?
---
Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting
--
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]
