hughhhh commented on code in PR #45546:
URL: https://github.com/apache/superset/pull/45546#discussion_r4458082669
##########
superset/commands/dataset/update.py:
##########
@@ -608,29 +613,31 @@ def _validate_partition_mapping(self, exceptions:
list[ValidationError]) -> None
ValidationError(reason,
field_name="partition_value_transform")
)
- def _effective_transform(
+ def _effective_column_attribute(
self,
columns: list[dict[str, Any]] | None,
- mapped_column: str | None,
+ column_name: str | None,
+ attribute: str,
) -> str | None:
"""
- The value transform on the effective mapped column.
+ One attribute of a named column, as this request will leave it.
Reads from the payload when the request carries columns, and from the
persisted model otherwise -- a PUT that changes only `partition_column`
- still has to be validated against the transform already stored.
+ still has to be validated against the transform already stored, and
+ against the expression of the column it now names.
"""
- if not mapped_column:
+ if not column_name:
return None
if columns is not None:
for column in columns:
- if column.get("column_name") == mapped_column:
- return column.get("partition_value_transform")
+ if column.get("column_name") == column_name:
+ return column.get(attribute)
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
Fixed: a partial column entry can no longer skip the Jinja check.
`_effective_column_attribute` (`superset/commands/dataset/update.py`) now reads
an omitted attribute from the row the write will actually touch, and an
explicit `None` still counts as a clear.
- **`id` with no `expression` (4d0e16bd1e):** this falls back to the column
stored under that `id`, as you suggested.
`test_a_partial_entry_keeps_the_stored_templated_expression` covers the block,
and `test_an_explicit_null_expression_clears_the_stored_template` covers the
clear.
- **`override_columns=true` (1360f15137):** the fallback above still missed
this path, because `_override_columns` matches by `column_name` and ignores
`id`. An entry with neither `id` nor `expression` kept the stored Jinja and
passed validation, and a renamed entry with a stale `id` was refused over an
expression its new row wouldn't get. Both now follow the name.
`test_an_override_entry_keeps_the_stored_template_of_its_name` and
`test_an_override_entry_ignores_a_stale_id` failed before the fix and pass now.
The value transform goes through the same helper. The PUT schema requires
`column_name`, so the id-only entry CodeAnt mentions can't reach this code.
I've left this thread open for you to check.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
