hughhhh commented on code in PR #45546:
URL: https://github.com/apache/superset/pull/45546#discussion_r4459780617
##########
tests/unit_tests/commands/dataset/update_test.py:
##########
@@ -2132,3 +2134,178 @@ def
test_a_self_mapping_is_still_rejected_alongside_a_column_payload(
command._validate_partition_mapping(exceptions)
assert [exc.field_name for exc in exceptions] == ["partition_column"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_templated_partition_column_blocks_the_save(
+ mocker: MockerFixture,
+) -> None:
+ """
+ The mirrored predicate is built outside the template processor, so a
+ Jinja-bearing partition key would reach every chart's SQL unrendered.
+ Read from the stored column when the request carries no column payload.
+ """
+ command = _mapping_command(
+ mocker,
+ "unix_timestamp(:value)",
+ partition_expression="{{ 'dt_' ~ 'epoch' }}",
+ )
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] == ["partition_column"]
+ assert "Jinja" in str(exceptions[0])
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_templated_partition_column_in_the_payload_blocks_the_save(
+ mocker: MockerFixture,
+) -> None:
+ """The expression this request writes is the one that counts, not
storage's."""
+ command = _mapping_command(mocker, "unix_timestamp(:value)")
+ command._properties = {
+ "columns": [
+ {
+ "column_name": "event_time",
+ "partition_value_transform": "unix_timestamp(:value)",
+ },
+ {"column_name": "dt_epoch", "expression": "{% if 1 %}dt{% endif
%}"},
+ ]
+ }
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] == ["partition_column"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_partial_entry_keeps_the_stored_templated_expression(
+ mocker: MockerFixture,
+) -> None:
+ """
+ An entry with an `id` and no `expression` leaves the stored expression in
+ place, so the stored Jinja is what the save has to be refused over.
+ """
+ command = _mapping_command(
+ mocker,
+ "unix_timestamp(:value)",
+ partition_expression="{{ 'dt_' ~ 'epoch' }}",
+ )
+ mapped_column, partition_column = cast(Any, command._model).columns
+ mapped_column.id = 1
+ partition_column.id = 2
+ command._properties = {
+ "columns": [
+ {"id": 1, "column_name": "event_time"},
+ {"id": 2, "column_name": "dt_epoch", "description": "partition
key"},
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
Added in 7685947d5f.
`test_a_renamed_partial_entry_keeps_the_stored_template_of_its_id` is your
case: stored `id=2` named `dt_epoch` holds the Jinja, the payload sends `{"id":
2, "column_name": "dt_renamed"}` with `partition_column="dt_renamed"`, and the
test asserts a `partition_column` error mentioning Jinja.
It passes against the current lookup, so `_effective_column_attribute`
didn't need changing. To check that it separates the two, I switched the lookup
to `column_name`, and the test failed while the existing partial-entry test
still passed. 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]
