hughhhh commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4319676081
##########
superset/models/helpers.py:
##########
@@ -5623,6 +6025,33 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
)
having_clause_and += [Grouping(self.text(having))]
+ # The mirrors go on last, so the snapshot taken here is everything the
+ # outer query and the series-limit ranking subquery agree about: row
+ # level security, the extras `where`, every filter. The two disagree
+ # about exactly the mirrors, and subtracting them again later is not an
+ # option -- `==` on a SQLAlchemy expression builds a new expression
+ # rather than answering a question, so the list cannot be filtered by
+ # identity.
+ where_clause_and_without_mirrors = list(where_clause_and)
+ inner_partition_mirror_predicates: list[Any] = []
+ if partition_mapping is not None:
+ partition_mirror_predicates =
self._build_partition_mirror_predicates(
Review Comment:
Fixed. `get_sqla_query` takes `mirror_partition_filters`, which
`get_extra_cache_keys` passes as `False` — it wants the SQL's shape, not the
SQL a chart would run, and the mirrors contribute nothing to
`extra_cache_keys`. It is not in `SQLA_QUERY_KEYS`, so no request payload can
set it.
##########
superset/models/helpers.py:
##########
@@ -5689,7 +6106,20 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
)
if _inner_filter is not None:
inner_time_filter = [_inner_filter]
- subq = subq.where(and_(*(where_clause_and +
inner_time_filter)))
+ # The mirrors swapped for the inner window's own, since the
+ # time filter just built describes that window: an outer-window
+ # mirror here would demand partition keys from a period this
+ # subquery excludes, and the empty ranking would drop the
+ # comparison series from the join below.
+ subq = subq.where(
+ and_(
+ *(
+ where_clause_and_without_mirrors
+ + inner_partition_mirror_predicates
Review Comment:
Fixed. The inner mirror is added only `if not
db_engine_spec.time_groupby_inline`, so on ClickHouse (and Databend,
Elasticsearch, Kusto) the ranking subquery is no longer narrowed by a window
nothing else in that query mentions. Window-independent mirrors still go to
both sinks, since those correspond to `where_clause_and`, which the subquery
does receive.
##########
superset/views/datasource/views.py:
##########
@@ -87,6 +88,51 @@ def _load_dataset_for_samples(
return dataset, None
+def _partition_transform_error(
+ orm_datasource: Any,
+ datasource_dict: dict[str, Any],
+) -> str | None:
+ """
+ The first column whose incoming value transform may not be stored, if any.
+
+ Only datasets carry a partition mapping, and only a transform the request
+ actually changes is worth refusing over -- re-sending a value already in
+ storage is what a GET-then-PUT client does, and a transform stored before
+ this check existed is disarmed at probe time instead.
+ """
+ incoming = [
+ column
+ for column in datasource_dict.get("columns") or []
+ if column.get("partition_value_transform")
+ ]
+ database = getattr(orm_datasource, "database", None)
+ if not incoming or database is None:
+ return None
+
+ stored = {
+ column.column_name: column.partition_value_transform
+ for column in orm_datasource.columns
+ }
+ for column in incoming:
Review Comment:
Fixed. The legacy save calls `stored_expression_error`, which checks
`len(transform) > MAX_TRANSFORM_LENGTH` ahead of the parse — both because it is
the cheaper question and because the parse is the cost the bound exists to
contain.
##########
superset/views/datasource/views.py:
##########
@@ -87,6 +95,109 @@ def _load_dataset_for_samples(
return dataset, None
+def _partition_transform_error(
+ orm_datasource: Any,
+ datasource_dict: dict[str, Any],
+) -> str | None:
+ """
+ The first column whose incoming value transform may not be stored, if any.
+
+ Only datasets carry a partition mapping, and only a transform the request
+ actually changes is worth refusing over -- re-sending a value already in
+ storage is what a GET-then-PUT client does, and a transform stored before
+ this check existed is disarmed at probe time instead.
+ """
+ incoming = [
+ column
+ for column in datasource_dict.get("columns") or []
+ if column.get("partition_value_transform")
+ ]
+ database = getattr(orm_datasource, "database", None)
+ if not incoming or database is None:
+ return None
+
+ stored = {
+ column.column_name: column.partition_value_transform
+ for column in orm_datasource.columns
+ }
+ for column in incoming:
+ transform = column["partition_value_transform"]
+ if transform == stored.get(column.get("column_name")):
+ continue
+ if reason := stored_expression_error(
Review Comment:
Fixed. The legacy save reaches `validate_transform`'s blocking tier as well
as `stored_expression_error`, so `random() + :value` is refused here instead of
saving with a 200 and then failing every later PUT.
##########
superset/views/datasource/views.py:
##########
@@ -157,6 +203,18 @@ def save(self) -> FlaskResponse:
),
status=409,
)
+ # `partition_value_transform` rides in on `update_from_object`, which
+ # writes every field in `update_from_object_fields` straight onto the
+ # column. That bypasses `UpdateDatasetCommand`, and with it the gate
+ # the REST PUT and the mapping preview both apply -- so without this
+ # the deprecated endpoint is a way to store arbitrary SQL that the
+ # partition probe later runs. Refused rather than dropped, unlike the
+ # importer: this is one interactive edit whose author is present to
+ # correct it, not a bundle where failing the whole dataset over one
+ # expression is the worse trade.
+ if error := _partition_transform_error(orm_datasource,
datasource_dict):
Review Comment:
Fixed. `_partition_mapping_reference_error` runs the same
`validate_partition_mapping` reference check as `UpdateDatasetCommand` before
`update_from_object`, so the legacy endpoint can no longer store a
`partition_column` naming a column that does not exist.
--
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]
