sadpandajoe commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4320971029
##########
superset/commands/dataset/importers/v0.py:
##########
@@ -291,7 +319,154 @@ def run(self) -> None:
.one()
)
dataset["database_id"] = database.id
- SqlaTable.import_from_dict(dataset, sync=self.sync)
+ self._prepare_partition_mapping(dataset, database)
+ table = SqlaTable.import_from_dict(dataset, sync=self.sync)
+ if table is not None:
+ imported.append(table)
+
+ self._repair_partition_mappings(imported)
+
+ @classmethod
+ def _default_omitted_monotonic_declarations(cls, config: Any) -> None:
+ """
+ Materialize `partition_transform_is_monotonic` wherever a transform is
+ set, anywhere in a v0 config.
+
+ The v1 importer does this on its own raw config, and for the same
+ reason: an overwrite keeps whatever the bundle omits, including a
+ ``True`` left behind by the transform this bundle is replacing. A
+ non-monotonic transform declared monotonic enables range mirrors that
+ exclude rows the filter keeps -- ``abs(:value)`` inheriting a stored
+ ``True`` turns ``n >= -1`` into ``p >= 1`` and misses ``n = 0``.
+
+ Walked rather than addressed directly because a v0 bundle comes in two
+ shapes: a CLI export nests datasets under ``databases[].tables[]``,
+ while a UI export is a bare list of datasets. Only columns that supply
+ a transform are touched, so a re-import that says nothing about a
+ column stays idempotent.
+ """
+ if isinstance(config, list):
+ for entry in config:
+ cls._default_omitted_monotonic_declarations(entry)
+ return
+ if not isinstance(config, dict):
+ return
+ _default_omitted_monotonic_declarations(config.get("columns") or [])
+ for key in (DATABASES_KEY, "tables"):
+ for child in config.get(key) or []:
+ cls._default_omitted_monotonic_declarations(child)
+
+ @staticmethod
+ def _prepare_partition_mapping(
+ dataset: dict[str, Any], database: Optional[Database]
+ ) -> None:
+ """
+ Run the two repairs that have to happen *before* the bundle is applied.
+
+ `_repair_partition_mappings` below runs after `import_from_dict`, which
+ is too late for either of these.
+
+ Sanitizing the bundle. The v1 importer drops a transform the save path
+ would refuse (`drop_unusable_partition_transforms`) on its way in, and
+ v0 did not -- so a legacy bundle naming ``partition_mapped_column: n``
+ and a transform of ``random() + :value`` on ``n`` imported happily and
+ then failed *every* later save: a description-only
+ ``PUT /api/v1/dataset/<id>`` returns 422 on the stored expression, and
+ the mapping never mirrors anything. Applied to the config rather than
+ the model because that is where the transform still is.
+
+ Disarming the old mapping.
`DatasetDAO.clear_unmapped_partition_transforms`
+ reads the mapping as it stands, so the post-import call can only
enforce
+ the one-transform invariant against the *new* references. An overwrite
+ that re-points the mapping makes a previously unmapped column the
+ effective one, and a transform parked there -- which nothing in the
+ bundle authored -- is found already mapped by the later call and kept.
+ A dataset mapping ``a -> p`` with a parked ``:value + 100`` on ``b``,
+ overwritten by a bundle setting ``partition_mapped_column: b`` and
+ omitting the columns, would mirror ``b = 5`` as ``p = 105`` and drop
the
+ row ``(a=1, b=5, p=1)``. The same pre-change call `DatasetDAO.update`
+ and the v1 importer both make, and for the same reason.
+
+ ``database`` is ``None`` only for a CLI bundle importing a connection
+ that does not exist yet, which by definition has no stored dataset to
+ disarm and no dialect to validate a transform against -- the same case
+ `drop_unusable_partition_transforms` already declines to act on.
+ """
+ if database is None:
+ return
+
+ # Deferred import: `daos.dataset` pulls in `views.base`, which
+ # circularly imports back into the commands package at app init.
+ from superset.daos.dataset import DatasetDAO # noqa: PLC0415
+
+ drop_unusable_partition_transforms(dataset, database)
+
+ if not (table_name := dataset.get("table_name")):
+ return
+ existing = (
+ db.session.query(SqlaTable)
+ .filter(
+ SqlaTable.table_name == table_name,
+ SqlaTable.schema == dataset.get("schema"),
+ SqlaTable.catalog == dataset["catalog"]
+ if dataset.get("catalog") is not None
+ else True,
+ SqlaTable.database_id == database.id,
+ )
+ .one_or_none()
+ )
+ if existing is not None:
+ DatasetDAO.clear_unmapped_partition_transforms(existing)
+
+ @staticmethod
+ def _repair_partition_mappings(datasets: list[SqlaTable]) -> None:
+ """
+ Keep the partition filter mapping honest across a v0 import.
+
+ The mapping's columns ride in `SqlaTable.export_fields`, so this path
+ carries them -- but it calls `SqlaTable.import_from_dict` directly and
+ goes through neither `DatasetDAO.update` nor the v1 importer, so
+ neither of the mapping's repairs ran. An older bundle imported with
+ ``--sync columns`` that omits the partition column deletes it and keeps
+ `partition_column` naming it: pruning silently stops, and every later
+ edit of that dataset fails the partition-column-not-found validation,
+ including a description-only PUT, which carries no columns payload and
+ so cannot reach the branch that forgives a stored-only dangle.
+
+ The same pair, in the same order, as `RefreshDatasetCommand` and the
+ v1 importer. Run after the import rather than around it because
+ ``import_from_dict`` is what discovers which datasets are involved.
+ """
+ if not datasets:
+ return
+ # Deferred import: `daos.dataset` pulls in `views.base`, which
+ # circularly imports back into the commands package at app init.
+ from superset.daos.dataset import DatasetDAO # noqa: PLC0415
+
+ db.session.flush()
+ for dataset in datasets:
+ DatasetDAO.clear_dangling_partition_mapping(
Review Comment:
`dataset.columns` is still the pre-import collection here:
`import_from_dict` removes omitted columns with `session.delete(...)`, and
`flush()` doesn't drop them from an already-loaded relationship. With
`sync=["columns"]` and a bundle that omits the partition column `p`, `p` still
counts as a survivor, so `partition_column` keeps naming a column that no
longer exists. Pruning stops and every later PUT (even description-only) fails
the column-not-found validation. Should this expire `columns` before reading
the surviving names, the way the v1 importer does?
--
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]
