hughhhh commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4319662866
##########
superset/commands/dataset/create.py:
##########
@@ -248,5 +253,52 @@ def validate(self) -> None: # noqa: C901
# so a ``viewers`` key would be dropped by the DAO's ``setattr`` loop.
populate_subjects(self._properties, exceptions, include_viewers=False)
+ self._validate_partition_mapping(exceptions)
+
if exceptions:
raise DatasetInvalidError(exceptions=exceptions)
+
+ def _validate_partition_mapping(self, exceptions: list[ValidationError])
-> None:
+ """
+ Reject a partition mapping that cannot work, at create time too.
+
+ `UpdateDatasetCommand` has validated this since the mapping landed, but
+ create did not, so the same mapping accepted on POST and rejected on
PUT
+ was reachable -- and once stored it is only reported as broken by the
+ editor, which an API-only caller never opens.
+
+ What create can check is narrower than what update can. The dataset's
+ columns are synced by `fetch_metadata` *after* this runs, so "is that a
+ real column?" has no answer yet; both submitted names are therefore
+ passed as known so the existence checks pass trivially and the checks
+ that do not need a column list still run. There is no column payload on
+ POST either, so no transform can arrive here to validate.
+ """
+ if not is_feature_enabled(PARTITION_FILTER_MAPPING):
+ return
+
+ partition_column = self._properties.get("partition_column")
+ database = self._properties.get("database")
+ if not partition_column or not database:
+ # No database means the caller already has a
+ # `DatabaseNotFoundValidationError`; there is no engine to validate
+ # a mapping against and no value in a second error about it.
+ return
+
+ partition_mapped_column =
self._properties.get("partition_mapped_column")
+ for issue in validate_partition_mapping(
+ column_names={
+ name
+ for name in (partition_column, partition_mapped_column)
+ if name is not None
+ },
+ partition_column=partition_column,
+ partition_mapped_column=partition_mapped_column,
+ main_dttm_col=None,
Review Comment:
Fixed. `_validate_partition_mapping_after_sync` runs after `fetch_metadata`,
against the real columns and the real `main_dttm_col`, filtered to
`issue.blocking`. The implicit self-mapping stays Tier 2 there on purpose —
blocking it would recreate the unsaveable dataset the check exists to prevent —
and the rejection rolls the partial create back.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -267,6 +272,72 @@ def _get_template_params(dataset: SqlaTable) -> dict[str,
Any]:
return params if isinstance(params, dict) else {}
+def drop_unusable_partition_transforms(config: dict[str, Any]) -> None:
+ """
+ Drop a partition value transform the save path would have rejected.
+
+ Import is the one door into this field that does not go through
+ `UpdateDatasetCommand`, so a bundle can carry a transform holding Jinja or
+ calling a non-deterministic function -- one that will never mirror a
filter,
+ and that the editor can only report as broken after the fact. Dropping it
on
+ the way in leaves the column with no transform, a state an owner can see
and
+ fix, rather than a stored expression that looks configured and is not.
+
+ Deliberately sanitizes rather than raises. A bundle is imported as a whole,
+ and failing someone's entire dataset over one unusable expression is a
worse
+ trade than importing the dataset without it. This is the same bargain
+ `DatasetDAO.clear_dangling_partition_mapping` already strikes for a mapping
+ whose column went away.
+
+ Two kinds of check, and only one of them answers to the feature flag. The
+ usability checks ask whether a transform will ever mirror a filter, which
+ is a question about a live feature; with the flag off nothing mirrors, so
+ rewriting the bundle would discard configuration for no gain. The
+ structural check asks whether the expression is one this product stores at
+ all, and that answer does not change with a flag -- an import written while
+ the flag was off would otherwise sit in the metadata DB fully armed,
+ waiting for an operator to turn the flag on.
+ """
+ columns = config.get("columns")
+ if not columns:
+ return
+ if not any(column.get("partition_value_transform") for column in columns):
+ return
+
+ database =
db.session.query(Database).filter_by(id=config["database_id"]).first()
+ if database is None:
+ return
+
+ check_usability = is_feature_enabled(PARTITION_FILTER_MAPPING)
+ catalog = config.get("catalog")
+ schema = config.get("schema")
+
+ for column in columns:
+ transform = column.get("partition_value_transform")
+ if not transform:
+ continue
+
+ reasons: list[str] = []
+ if reason := stored_expression_error(database, catalog, schema,
transform):
Review Comment:
Fixed. `stored_expression_error` is now asked only when
`is_unfinished(transform, database.backend)` is false — the same condition
`UpdateDatasetCommand` puts in front of that gate — so a half-typed transform
imports in the inactive state PUT stores it in. The structural check still runs
with the flag off, deliberately, so a bundle written flag-off cannot sit armed
in the metadata DB.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -582,6 +668,12 @@ def import_dataset( # noqa: C901
if dataset.id is None:
db.session.flush()
+ # A bundle can name a mapped column and still carry transforms on other
+ # columns; the editor's client-side guard never runs here. Left in place, a
+ # transform on an unmirrored column is invisible and still stored, ready to
+ # go live the moment the mapped column resolves back to it.
+ DatasetDAO.clear_unmapped_partition_transforms(dataset)
Review Comment:
Fixed. The cleanup runs against both the old and the new mapping before the
import is applied — resolving `partition_column` / `partition_mapped_column`
from the config falling back to the stored values — so a parked transform
cannot be activated by an overwrite that clears the override.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -582,6 +734,27 @@ def import_dataset( # noqa: C901
if dataset.id is None:
db.session.flush()
+ # A bundle can name a mapped column and still carry transforms on other
+ # columns; the editor's client-side guard never runs here. Left in place, a
+ # transform on an unmirrored column is invisible and still stored, ready to
+ # go live the moment the mapped column resolves back to it.
+ DatasetDAO.clear_unmapped_partition_transforms(dataset)
+
+ # And repair a reference the import itself broke: a `sync` deletion can
+ # remove the column `partition_column` names, which otherwise leaves the
+ # dataset failing `UpdateDatasetCommand`'s validation on every later edit
--
+ # including a description-only PUT, which carries no columns payload and so
+ # cannot reach the forgiveness branch that tolerates a stored-only dangle.
+ #
+ # Flushed and expired first: the `sync` deletion is still pending here, and
+ # reading the relationship without flushing loads the removed rows back and
+ # un-deletes them.
+ db.session.flush()
+ db.session.expire(dataset, ["columns"])
+ DatasetDAO.clear_dangling_partition_mapping(
Review Comment:
Fixed. An explicit self-mapping — `partition_mapped_column` equal to
`partition_column` — is cleared on import, in line with the dangling-reference
repair, so a bundle setting both to the same column no longer produces a
dataset that fails its next description-only PUT.
--
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]
