aminghadersohi commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4288257702
##########
superset/commands/dataset/importers/v0.py:
##########
@@ -291,7 +319,151 @@ 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.database_id == database.id,
Review Comment:
When two catalogs contain the same schema/table, this lookup matches both
and aborts v0 imports with `MultipleResultsFound`, even with the feature flag
off. Include the supplied catalog in the lookup.
```suggestion
SqlaTable.schema == dataset.get("schema"),
SqlaTable.catalog == dataset["catalog"]
if dataset.get("catalog") is not None
else True,
SqlaTable.database_id == database.id,
```
--
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]
