sadpandajoe commented on code in PR #45546:
URL: https://github.com/apache/superset/pull/45546#discussion_r4458678002
##########
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:
This only exercises the id-keyed fallback with the name unchanged, so a
lookup keyed on `column_name` instead of `id` would pass it too. A rename in
the same PUT is the case that separates them: stored column `id=2` named
`dt_epoch` holds the Jinja, the payload sends `{"id": 2, "column_name":
"dt_renamed"}` with `partition_column="dt_renamed"` and no `expression`, and a
name-based lookup finds no stored row and returns `None`, so the templated
column is saved. Could you add a case like that asserting a `partition_column`
error mentioning Jinja?
--
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]
