codeant-ai-for-open-source[bot] commented on code in PR #44816:
URL: https://github.com/apache/superset/pull/44816#discussion_r4140630659
##########
superset/connectors/sqla/models.py:
##########
@@ -1924,8 +1987,84 @@ def data(self) -> ExplorableData:
data_["extra"] = self.extra
data_["always_filter_main_dttm"] = self.always_filter_main_dttm
data_["normalize_columns"] = self.normalize_columns
+ data_["partition_column"] = self.partition_column
+ data_["partition_mapped_column"] = self.partition_mapped_column
+ data_["partition_filter_mapping"] =
self.partition_filter_mapping_summary
return data_
+ # Partition filter mapping (ยง PARTITION_FILTER_MAPPING). Read-only
+ # properties over `PARTITION_MAPPING_STORE` rather than columns, for the
+ # reasons spelled out on `TableColumn.partition_value_transform` and in
+ # `superset.connectors.sqla.partition_mapping_storage`. Writes go through
+ # `set_partition_mapping`, or `DatasetDAO`'s funnel on the API path.
+ @property
+ def partition_column(self) -> str | None:
+ """Physical column the engine partitions on."""
+ return load_partition_mapping(self).partition_column
+
+ @property
+ def partition_mapped_column(self) -> str | None:
+ """
+ Explicit override for the column whose filters are mirrored.
+
+ ``None`` means "follow ``main_dttm_col``", so re-pointing the default
+ datetime column moves the mapping with it.
+ """
+ return load_partition_mapping(self).mapped_column
+
+ def update_from_object(self, obj: dict[str, Any]) -> None:
+ """
+ Sync from the legacy datasource editor's payload, mapping intact.
+
+ `super()` writes `obj.get(attr)` for every name in
+ `update_from_object_fields`, and `extra` is one of them -- so a payload
+ that omits it writes NULL and takes the partition mapping (and the
+ dataset's certification) down with it. Re-applying afterwards preserves
+ the mapping when the payload is silent about `extra`, and leaves an
+ `extra` that *does* carry one exactly as sent.
+ """
+ mapping = load_partition_mapping(self)
+ super().update_from_object(obj)
+ if "extra" not in obj and not mapping.is_empty:
+ partition_mapping_store().save(self, mapping)
Review Comment:
**Suggestion:** When the editor payload omits `extra`, `super()` first
clears it, then saving only the mapping recreates the blob without
certification, timezone, or other existing extra settings.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=84693cf6cd0d47dfb8117499dd9e8183&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=84693cf6cd0d47dfb8117499dd9e8183&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 2027:2029
**Comment:**
*Logic Error: When the editor payload omits `extra`, `super()` first
clears it, then saving only the mapping recreates the blob without
certification, timezone, or other existing extra settings.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=edb0f6fe3c0b5a292f698433cee3907af97e9bd6bd8e08a1b2bf2444c2f597db&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=edb0f6fe3c0b5a292f698433cee3907af97e9bd6bd8e08a1b2bf2444c2f597db&reaction=dislike'>๐</a>
##########
superset/connectors/sqla/partition_mapping_storage.py:
##########
@@ -0,0 +1,606 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Where a dataset's partition filter mapping is persisted.
+
+A mapping is four values -- see `superset.connectors.sqla.partition_mapping`
for
+what they mean. This module owns the question of where those four values live,
+and nothing else. Every read goes through `PartitionMappingStore.load` and
every
+write through `set_partition_mapping`, so the answer can change without
touching
+the model, the API, validation or the query rewriter.
+
+Two backings
+------------
+The feature ships stored as one key in ``tables.extra``
+(`ExtraJsonPartitionMappingStore`) and will move to four real columns
+(`ColumnPartitionMappingStore`) in the migration that adds them. Selected by
+``PARTITION_MAPPING_STORE``.
+
+``tables.extra`` is not merely the option that avoids a migration; it is a
+reasonable home. It is already in `SqlaTable.export_fields` and already listed
in
+`ExportDatasetsCommand`'s ``JSON_KEYS``, so a mapping travels through
+export/import with no bundle-format change. SQLAlchemy-Continuum already
+versions the column, so dataset version history and restore work untouched. And
+there is precedent for reading typed settings back out of it:
+``certification``, ``warning_markdown`` and ``timezone`` all live there, the
+first two surfaced through model properties exactly as the mapping now is.
+
+It has to be the *dataset*-level blob, though, never ``table_columns.extra``,
+even for the two values that are per-column after the migration. The dataset
+editor's ``buildExtraJsonObject`` rebuilds every column's and metric's
``extra``
+from ``certification`` and ``warning_markdown`` alone on each save, so anything
+else parked there is destroyed the first time an owner opens the editor.
+Dataset-level ``extra`` is round-tripped verbatim, and its one mutating path
+(``setDatasetCertification``) parses and *merges*, so an unknown key survives.
+
+Reads are cheap; writes are funnelled
+-------------------------------------
+`load` is on the hot path -- `get_extra_cache_keys`, every `get_sqla_query`,
and
+once per column in `TableColumn.data` -- so it memoizes on the instance, keyed
on
+the raw ``extra`` string, the same self-invalidating trick
+`CertificationMixin.get_extra_dict` uses. (`SqlaTable.extra_dict` is an
uncached
+``json.loads``; don't route through it.)
+
+Writes cannot be attribute assignment. Under the extra-JSON store the mapping
+and ``extra`` are the same bytes, so ``table.partition_column = x`` followed by
+``table.extra = <payload>`` loses the first write with no exception and no log
--
+and which of the two runs last is a property of marshmallow's field ordering.
+The model's properties are therefore read-only and `set_partition_mapping` is
the
+only supported write.
+"""
+
+from __future__ import annotations
+
+import logging
+from dataclasses import dataclass, field, replace
+from typing import Any, Mapping, Protocol, TYPE_CHECKING
+
+from flask import current_app as app
+
+from superset.utils import json
+
+if TYPE_CHECKING:
+ from superset.connectors.sqla.models import SqlaTable
+
+logger = logging.getLogger(__name__)
+
+#: Top-level key in ``tables.extra``. Joins ``certification``,
+#: ``warning_markdown``, ``timezone`` and ``disallow_adhoc_metrics``.
+EXTRA_KEY = "partition_filter_mapping"
+
+#: Bounds enforced on read as well as on write. ``DatasetPutSchema`` validates
+#: the typed fields, but the free-text Extra box reaches the same storage
without
+#: passing through it, so the store re-checks rather than trusting its
contents.
+#: The two lengths are the widths the columns get after the migration.
+MAX_COLUMN_NAME_LENGTH = 250
+MAX_TRANSFORM_LENGTH = 4096
+
+#: Sentinel for "this keyword was not passed", distinct from ``None``, which
+#: means "clear this field".
+UNSET: Any = object()
+
+_MEMO_UNSET: Any = object()
+_MEMO_VALUE_ATTR = "_partition_mapping_memo"
+_MEMO_RAW_ATTR = "_partition_mapping_memo_raw"
+
+
+@dataclass(frozen=True)
+class ColumnTransform:
+ """
+ The two per-column halves of a mapping: the ``:value`` expression and
+ whether it preserves ordering.
+ """
+
+ value_transform: str | None = None
+ is_monotonic: bool = False
+
+ @property
+ def is_empty(self) -> bool:
+ return self == ColumnTransform()
+
+
+@dataclass(frozen=True)
+class StoredPartitionMapping:
+ """
+ A dataset's mapping exactly as persisted -- unresolved and unvalidated.
+
+ ``mapped_column is None`` means "follow ``main_dttm_col``", so re-pointing
+ the dataset's default datetime column moves the mapping with it unless the
+ owner overrides it. Deliberately not resolved at save time: that behaviour
+ then falls out of the model rather than needing code on every write path.
+
+ Transforms are keyed by column name rather than flattened onto the single
+ effective mapped column, because flattening is unsound. An owner who sets a
+ transform on ``A`` (then ``main_dttm_col``) and later re-points
+ ``main_dttm_col`` to ``B`` would, flattened, have ``B``'s values silently
+ mirrored through ``A``'s expression. Keyed, ``B`` has no transform, the
+ mapping goes inactive, and the behaviour matches what four real columns
+ would do -- which is the property that makes the two stores swappable.
+ """
+
+ partition_column: str | None = None
+ mapped_column: str | None = None
+ column_transforms: Mapping[str, ColumnTransform] =
field(default_factory=dict)
+
+ @property
+ def is_empty(self) -> bool:
+ return (
+ not self.partition_column
+ and not self.mapped_column
+ and not self.column_transforms
+ )
+
+ def transform_for(self, column_name: str | None) -> ColumnTransform:
+ """
+ The transform declared on one column, or an empty one.
+
+ Empty rather than ``None`` so callers read ``.value_transform`` /
+ ``.is_monotonic`` unconditionally and get the same "not declared"
+ answer a NULL column would give them.
+ """
+ if not column_name:
+ return ColumnTransform()
+ return self.column_transforms.get(column_name, ColumnTransform())
+
+ def effective_mapped_column(self, main_dttm_col: str | None) -> str | None:
+ """The column whose filters are mirrored, override or default."""
+ return self.mapped_column or main_dttm_col
+
+ def with_transform(
+ self, column_name: str, transform: ColumnTransform
+ ) -> StoredPartitionMapping:
+ transforms = dict(self.column_transforms)
+ if transform.is_empty:
+ transforms.pop(column_name, None)
+ else:
+ transforms[column_name] = transform
+ return replace(self, column_transforms=transforms)
+
+
+@dataclass(frozen=True)
+class PartitionMappingPatch:
+ """
+ Only the fields a request actually mentioned.
+
+ Present keys rather than four ``X | None`` attributes, because ``None`` is
a
+ meaningful value here: ``{"partition_column": None}`` clears the mapping
+ while an absent key leaves it alone. Collapsing the two is how a partial
+ column payload silently clears a monotonic flag nobody touched -- the exact
+ failure ``DatasetColumnsPutSchema`` avoids by refusing a ``load_default``.
+ """
+
+ fields: Mapping[str, Any] = field(default_factory=dict)
+ column_fields: Mapping[str, Mapping[str, Any]] =
field(default_factory=dict)
+
+ @property
+ def is_empty(self) -> bool:
+ return not self.fields and not self.column_fields
+
+ def apply_to(self, mapping: StoredPartitionMapping) ->
StoredPartitionMapping:
+ if self.fields:
+ mapping = replace(mapping, **dict(self.fields))
+ for column_name, values in self.column_fields.items():
+ current = mapping.transform_for(column_name)
+ mapping = mapping.with_transform(
+ column_name, replace(current, **dict(values))
+ )
+ return mapping
+
+
+class PartitionMappingStore(Protocol):
+ """
+ Where a dataset's partition mapping lives.
+
+ Exists because the mapping ships before its migration: today it is one key
+ in ``tables.extra``, and a follow-up moves it to four real columns. Both
+ shapes answer the same two questions, so the model facade, the write
funnel,
+ validation and the query rewriter are written once against this and never
+ learn which store answered.
+ """
+
+ def load(self, table: SqlaTable) -> StoredPartitionMapping: ...
+
+ def save(self, table: SqlaTable, mapping: StoredPartitionMapping) -> None:
...
+
+
+class ExtraJsonPartitionMappingStore:
+ """One nested object under ``partition_filter_mapping`` in
``tables.extra``."""
+
+ def load(self, table: SqlaTable) -> StoredPartitionMapping:
+ raw = getattr(table, "extra", None)
+ memo_raw = getattr(table, _MEMO_RAW_ATTR, _MEMO_UNSET)
+ if memo_raw is _MEMO_UNSET or memo_raw != raw:
+ # Transient instance attributes, not mapped columns, so they never
+ # reach the database and never confuse Continuum. Safe to hand the
+ # memoized object straight out: it is frozen.
+ object.__setattr__(table, _MEMO_VALUE_ATTR, parse_mapping(raw))
+ object.__setattr__(table, _MEMO_RAW_ATTR, raw)
+ return getattr(table, _MEMO_VALUE_ATTR)
+
+ def save(self, table: SqlaTable, mapping: StoredPartitionMapping) -> None:
+ raw = getattr(table, "extra", None)
+ updated = extra_with_partition_mapping(raw, mapping)
+ if updated is raw:
+ # `extra_with_partition_mapping` hands back the identical object
when
+ # the blob already says this. Writing anyway would dirty the column
+ # and mint a Continuum version row for an edit that changed
nothing.
+ return
+ # A *new string*, never a mutation of the parsed dict. `decode_extra`
+ # parses fresh and throws its dict away, so mutating one would change
+ # nothing on the model and SQLAlchemy would never see the attribute as
+ # dirty. Assignment is also what invalidates `load`'s memo.
+ table.extra = updated
Review Comment:
**Suggestion:** Concurrent saves read the same `extra`, then each writes a
replacement, so one update can silently erase another update's unrelated
dataset settings.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7dd47d0915744e0a8e2b9d7841aeaf6e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7dd47d0915744e0a8e2b9d7841aeaf6e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/partition_mapping_storage.py
**Line:** 234:246
**Comment:**
*Race Condition: Concurrent saves read the same `extra`, then each
writes a replacement, so one update can silently erase another update's
unrelated dataset settings.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=0cfc0755c2352794a0a80126358f7fc8d80d92b8361bbc6da964ac4b5b5d6e74&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=0cfc0755c2352794a0a80126358f7fc8d80d92b8361bbc6da964ac4b5b5d6e74&reaction=dislike'>๐</a>
##########
superset/daos/dataset.py:
##########
@@ -462,7 +483,56 @@ def update(
if force_update:
attributes["changed_on"] = datetime.now()
- return super().update(item, attributes)
+ # `BaseDAO.update` returns `db.session.merge(item)` for a detached
item,
+ # so the mapping is written onto whatever came back rather than onto
what
+ # went in.
+ updated = super().update(item, attributes)
+
+ if mapping_patch is not None or surviving_column_names is not None:
+ cls.save_partition_mapping(updated, mapping_patch,
surviving_column_names)
Review Comment:
**Suggestion:** `save_partition_mapping` performs a separate read and write
after the dataset update, so concurrent edits to other `extra` keys can be
overwritten.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Race condition`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=96b842c975b74c189cfaea357155fffb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=96b842c975b74c189cfaea357155fffb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/daos/dataset.py
**Line:** 489:492
**Comment:**
*Race Condition: `save_partition_mapping` performs a separate read and
write after the dataset update, so concurrent edits to other `extra` keys can
be overwritten.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=f476254f077c323234c7675fc18fd671a50b4bde210fec99f96546cb3215e327&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=f476254f077c323234c7675fc18fd671a50b4bde210fec99f96546cb3215e327&reaction=dislike'>๐</a>
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -0,0 +1,977 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Partition filter mapping.
+
+Datasets on Hadoop-family engines are commonly partitioned on a *technical*
+column -- an epoch integer, a lowercased region key -- that no analyst would
+filter on. Unless a query carries a predicate on that column the engine scans
+every partition.
+
+A dataset owner names one partition column ``p``, one business column that
+filters are mirrored from, and a value transform ``T`` (a SQL expression
+containing a ``:value`` placeholder). Superset then appends an equivalent
+predicate on ``p`` to every query, so chart authors change nothing and queries
+prune.
+
+The load-bearing assumption
+---------------------------
+Everything here reasons about ``T(col) op T(v)``, but what is emitted is
+``p op T(v)`` -- a predicate on a *physically different column*. The step from
+one to the other is::
+
+ p = T(mapped_col) for every row in the table
+
+Superset cannot verify that; it is a property of whatever ETL populates the
+partition column. If that job lags, backfills with different logic, or writes
+the partition key in a different timezone, mirrored predicates silently drop
+real rows. The mapping is only as trustworthy as the pipeline behind it.
+
+Operator safety
+---------------
+A mirrored predicate ``P2`` may only be ``AND``-ed onto a query when the
+original predicate ``P1`` *implies* it:
+
+=========================================== =============================
+Original Safe when
+=========================================== =============================
+``col = v``, ``col IN (...)`` always -- ``T`` is a function
+``col >=|>|<|<= v``, ``TEMPORAL_RANGE`` only if ``T`` is monotonic
+``col != v``, ``NOT IN``, ``LIKE``, ... never
+=========================================== =============================
+
+Negations are never safe because ``T`` need not be injective:
+``lower(:value)`` with ``country != 'US'`` mirrors to ``region_key != 'us'``,
+which wrongly excludes rows whose ``country`` is already lowercase ``'us'`` --
+rows the original filter *keeps*.
+
+Monotonicity is a property of the transform, not of the column's data type:
+``hour(:value)``, ``date_format(:value, 'dd')`` and ``dayofweek(:value)`` are
+all reasonable transforms on a ``TIMESTAMP`` column and none of them preserve
+ordering. It is therefore declared by the owner, not inferred.
+
+Time grains
+-----------
+A grained filter -- drill-to-detail, mostly -- compares the *truncated* column,
+``trunc(col) op v``, so the raw bounds it carries do not describe the rows it
+keeps. A row in the final partial bucket satisfies ``trunc(col) < until`` while
+``col < until`` excludes it.
+
+Which direction a grain rounds is not knowable from the duration alone, and the
+obvious guess is wrong: ``WEEK_ENDING_SATURDAY`` rounds *forward* on Hive and
+Presto, and Ocient's grains are ``ROUND``, i.e. to nearest. What every grain
+does satisfy is a bound on the displacement::
+
+ |trunc(ts) - ts| < width(grain)
+
+so widening *both* bounds by one bucket width is no narrower than the real
+predicate whichever way the grain rounds -- and it needs no monotonicity of
+``trunc`` itself, which is what rescues Hive's oddly-anchored ``P1W``. Width is
+a property of the grain, not of the engine, which is what makes this
+maintainable; see `grain_bucket_width`. A grain whose SQL an operator supplied
+has no known width, and does not mirror at all.
+"""
+
+from __future__ import annotations
+
+import hashlib
+import logging
+import re
+from dataclasses import dataclass
+from functools import lru_cache
+from typing import Any, cast, TYPE_CHECKING
+
+import sqlalchemy as sa
+from dateutil.relativedelta import relativedelta
+from flask import current_app as app
+from flask_babel import lazy_gettext as _
+from sqlalchemy.engine.interfaces import Dialect
+from sqlalchemy.sql.elements import ColumnElement
+
+from superset.connectors.sqla.partition_mapping_storage import (
+ ColumnTransform,
+ load_partition_mapping,
+ StoredPartitionMapping,
+)
+from superset.constants import LRU_CACHE_MAX_SIZE, TimeGrain
+from superset.exceptions import SupersetParseError
+from superset.extensions import cache_manager, feature_flag_manager
+from superset.sql.parse import SQLStatement
+from superset.utils import json
+from superset.utils.core import FilterOperator
+
+if TYPE_CHECKING:
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.models.core import Database
+
+logger = logging.getLogger(__name__)
+
+FEATURE_FLAG = "PARTITION_FILTER_MAPPING"
+
+#: Placeholder the owner writes in the transform, e.g.
``unix_timestamp(:value)``.
+#: Matched with word boundaries so ``:values`` is not mistaken for it.
+VALUE_PLACEHOLDER_RE = re.compile(r":value\b")
+
+#: Balanced Jinja blocks. The probe would render these in a different context
+#: at a different time from the chart query, so they are rejected at save time.
+JINJA_BLOCK_RE = re.compile(r"\{\{.*?\}\}|\{%.*?%\}|\{#.*?#\}", re.DOTALL)
+
+#: Substituted for ``:value`` before parsing -- sqlglot rejects a bare
``:value``
+#: on most dialects. Mirrors the ``_JINJA_BLOCK_RE`` -> ``NULL`` trick used by
+#: ``validate_stored_expression``.
+_PARSE_STANDIN = "NULL"
+
+#: Functions whose value depends on wall-clock time or randomness. The probe
+#: runs in a different session at a different moment from the chart query and
+#: its result is then cached, so any of these freezes a snapshot of probe time
+#: into the emitted predicate.
+NON_DETERMINISTIC_FUNCTIONS = {
+ "CURRENT_DATE",
+ "CURRENT_TIME",
+ "CURRENT_TIMESTAMP",
+ "NOW",
+ "RAND",
+ "RANDOM",
+ "UUID",
+}
+
+#: Functions that mean "now" only in their zero-argument form. On Hive and
+#: Impala ``unix_timestamp()`` is the current time while ``unix_timestamp(x)``
+#: -- the canonical transform for this feature -- is pure.
+NON_DETERMINISTIC_WHEN_NILADIC = {"UNIX_TIMESTAMP"}
+
+#: Safe for any function ``T``.
+MIRRORABLE_ALWAYS = {FilterOperator.EQUALS, FilterOperator.IN}
+
+#: Safe only when ``T`` preserves ordering.
+MIRRORABLE_IF_MONOTONIC = {
+ FilterOperator.GREATER_THAN,
+ FilterOperator.GREATER_THAN_OR_EQUALS,
+ FilterOperator.LESS_THAN,
+ FilterOperator.LESS_THAN_OR_EQUALS,
+ FilterOperator.TEMPORAL_RANGE,
+}
+
+
+def mirrorable_operators(is_monotonic: bool) -> set[FilterOperator]:
+ """
+ The operators whose predicates may be mirrored onto the partition column.
+
+ :param is_monotonic: whether the owner declared the transform
+ order-preserving
+ """
+ if is_monotonic:
+ return MIRRORABLE_ALWAYS | MIRRORABLE_IF_MONOTONIC
+ return set(MIRRORABLE_ALWAYS)
+
+
+#: How wide one bucket of each built-in time grain is.
+#:
+#: Keyed on the ISO duration a filter carries in its ``grain``. Written out
+#: rather than parsed: four of the week grains are ISO *intervals* with an
+#: anchor (``P1W/1970-01-03T00:00:00Z``) that ``isodate.parse_duration``
+#: rejects outright, and ``PT0.5H`` / ``P0.25Y`` are fractional. A literal
+#: table is also the thing a reviewer can check a line at a time.
+#:
+#: ``relativedelta`` rather than ``timedelta`` so the calendar grains stay
+#: calendar arithmetic: a month is not 30 days.
+GRAIN_BUCKET_WIDTHS: dict[str, relativedelta] = {
+ TimeGrain.SECOND: relativedelta(seconds=1),
+ TimeGrain.FIVE_SECONDS: relativedelta(seconds=5),
+ TimeGrain.THIRTY_SECONDS: relativedelta(seconds=30),
+ TimeGrain.MINUTE: relativedelta(minutes=1),
+ TimeGrain.FIVE_MINUTES: relativedelta(minutes=5),
+ TimeGrain.TEN_MINUTES: relativedelta(minutes=10),
+ TimeGrain.FIFTEEN_MINUTES: relativedelta(minutes=15),
+ TimeGrain.THIRTY_MINUTES: relativedelta(minutes=30),
+ TimeGrain.HALF_HOUR: relativedelta(minutes=30),
+ TimeGrain.HOUR: relativedelta(hours=1),
+ TimeGrain.SIX_HOURS: relativedelta(hours=6),
+ TimeGrain.DAY: relativedelta(days=1),
+ TimeGrain.WEEK: relativedelta(days=7),
+ TimeGrain.WEEK_STARTING_SUNDAY: relativedelta(days=7),
+ TimeGrain.WEEK_STARTING_MONDAY: relativedelta(days=7),
+ TimeGrain.WEEK_ENDING_SATURDAY: relativedelta(days=7),
+ TimeGrain.WEEK_ENDING_SUNDAY: relativedelta(days=7),
+ TimeGrain.MONTH: relativedelta(months=1),
+ TimeGrain.QUARTER: relativedelta(months=3),
+ TimeGrain.QUARTER_YEAR: relativedelta(months=3),
+ TimeGrain.YEAR: relativedelta(years=1),
+}
+
+
+def grain_bucket_width(grain: str | None, engine: str) -> relativedelta | None:
+ """
+ How far a grain's truncation can move a timestamp, or ``None`` if unknown.
+
+ A grained filter compares the *truncated* column, so the raw bounds it
+ carries do not describe the rows it keeps. Widening both bounds by one
+ bucket recovers a predicate that is no narrower than the real one -- see
+ `_collect_partition_mirror_range` for the argument. That only works for a
+ grain whose bucket width Superset knows, which excludes anything an
+ operator supplied.
+
+ :param grain: the ISO duration from the filter's ``grain``
+ :param engine: the engine spec's ``engine``, to check per-engine overrides
+ """
+ if not grain:
+ return None
+
+ # An operator-declared grain carries whatever duration string they typed,
+ # and `TIME_GRAIN_ADDON_EXPRESSIONS` can redefine a *built-in* grain's SQL
+ # per engine -- `P1D` could be anything at all. Neither has a width we can
+ # claim to know, so both fall back to not mirroring.
+ if grain in app.config["TIME_GRAIN_ADDONS"]:
+ return None
+ if grain in app.config["TIME_GRAIN_ADDON_EXPRESSIONS"].get(engine, {}):
+ return None
+
+ return GRAIN_BUCKET_WIDTHS.get(grain)
+
+
+@dataclass(frozen=True)
+class PartitionMapping:
+ """A resolved, usable partition filter mapping."""
+
+ partition_column: str
+ mapped_column: str
+ value_transform: str
+ is_monotonic: bool
+
+ def mirrors(self, operator: FilterOperator) -> bool:
+ return operator in mirrorable_operators(self.is_monotonic)
+
+
+def contains_value_placeholder(transform: str | None) -> bool:
+ """Whether the transform contains the ``:value`` placeholder."""
+ return bool(transform) and VALUE_PLACEHOLDER_RE.search(transform or "") is
not None
+
+
+def contains_jinja(transform: str | None) -> bool:
+ """Whether the transform contains a balanced Jinja block."""
+ return bool(transform) and JINJA_BLOCK_RE.search(transform or "") is not
None
+
+
+def parse_skeleton(transform: str) -> str:
+ """
+ The transform with ``:value`` substituted out, ready for a SQL parser.
+
+ ``sanitize_clause`` / sqlglot choke on a bare ``:value`` on most dialects,
+ so the placeholder is swapped for a benign literal first -- the same trick
+ ``validate_stored_expression`` uses for Jinja blocks.
+ """
+ return VALUE_PLACEHOLDER_RE.sub(_PARSE_STANDIN, transform)
+
+
+def _parse_skeleton(transform: str, engine: str) -> SQLStatement | None:
+ """
+ Parse ``SELECT <transform>`` with the placeholder substituted out.
+
+ Returns ``None`` when the transform does not parse.
+ """
+ try:
+ return SQLStatement(f"SELECT {parse_skeleton(transform)}", engine)
+ except SupersetParseError:
+ return None
+
+
+def is_parseable(transform: str | None, engine: str) -> bool:
+ """Whether the transform parses as a single select expression."""
+ if not transform or not transform.strip():
+ return False
+ return _parse_skeleton(transform, engine) is not None
+
+
+def find_non_deterministic_functions(transform: str, engine: str) -> set[str]:
+ """
+ Names of non-deterministic functions the transform calls.
+
+ ``UNIX_TIMESTAMP`` is only reported in its zero-argument form, which means
+ "now" on Hive and Impala; the one-argument form is the canonical temporal
+ transform and stays allowed.
+ """
+ statement = _parse_skeleton(transform, engine)
+ if statement is None:
+ return set()
+
+ found = {
+ name
+ for name in NON_DETERMINISTIC_FUNCTIONS
+ if statement.check_functions_present({name})
+ }
+ return found | _find_niladic_calls(statement)
+
+
+def _find_niladic_calls(statement: SQLStatement) -> set[str]:
+ """
+ Names from ``NON_DETERMINISTIC_WHEN_NILADIC`` called with no arguments.
+
+ Note some dialects resolve the zero-argument form themselves -- Hive parses
+ ``unix_timestamp()`` straight to ``CURRENT_TIMESTAMP`` -- in which case the
+ name-based check above has already caught it. This is the backstop for the
+ dialects that do not.
+ """
+ return NON_DETERMINISTIC_WHEN_NILADIC & statement.get_niladic_functions()
+
+
+def resolve_partition_mapping(datasource: SqlaTable) -> PartitionMapping |
None:
+ """
+ Resolve the dataset's mapping, or ``None`` when nothing may be mirrored.
+
+ Every bail-out here is defensive as well as functional: save-time
validation
+ rejects most of these, but rows predating the validation can still violate
+ the invariants, and a column sync can invalidate a mapping that was fine
+ when it was written.
+ """
+ if not feature_flag_manager.is_feature_enabled(FEATURE_FLAG):
+ return None
+
+ stored = load_partition_mapping(datasource)
+ partition_column = stored.partition_column
+ if not partition_column:
+ return None
+
+ columns_by_name = {column.column_name: column for column in
datasource.columns}
+ if partition_column not in columns_by_name:
+ # The partition column was dropped by a column sync or at the source.
+ return None
+
+ mapped_column_name =
stored.effective_mapped_column(datasource.main_dttm_col)
+ if not mapped_column_name or mapped_column_name not in columns_by_name:
+ return None
+
+ if mapped_column_name == partition_column:
+ # Self-mapping: the mirrored predicate would duplicate the original.
+ return None
+
+ transform_spec = stored.transform_for(mapped_column_name)
+ transform = transform_spec.value_transform
+ # `is_transform_active` rather than a query-time restatement of the same
+ # rules: it *is* `validate_transform` with the messages discarded, so the
two
+ # cannot drift. The difference that matters is the non-deterministic
+ # denylist, which a Tier-2-only check omits -- and `CreateDatasetCommand`,
+ # import, and the editor's free-text Extra box all reach this storage
without
+ # passing the save-time validation, so a transform calling `now()` would
+ # otherwise be mirrored and its result cached.
+ if not is_transform_active(transform, datasource.database.backend):
+ return None
+
+ if _has_active_advanced_data_type(columns_by_name[mapped_column_name]):
+ # `translate_filter` builds its own predicate shape from *translated*
+ # values, so the `(operator, value)` pair the operator matrix reasons
+ # about does not exist and mirroring would apply the wrong values.
+ return None
+
+ return PartitionMapping(
+ partition_column=str(partition_column),
+ mapped_column=str(mapped_column_name),
+ value_transform=cast(str, transform),
+ is_monotonic=transform_spec.is_monotonic,
+ )
+
+
+def _has_active_advanced_data_type(column: TableColumn) -> bool:
+ advanced_data_type = getattr(column, "advanced_data_type", None)
+ if not advanced_data_type:
+ return False
+ if not
feature_flag_manager.is_feature_enabled("ENABLE_ADVANCED_DATA_TYPES"):
+ return False
+ return advanced_data_type in app.config.get("ADVANCED_DATA_TYPES", {})
+
+
+def build_probe_sql(
+ transform: str,
+ values: list[Any],
+ dialect: Dialect | None = None,
+) -> str:
+ """
+ Compile a single ``SELECT`` that evaluates the transform at every value.
+
+ Values are attacker-controlled (a Gamma user picks filter values), so they
+ are bound as parameters and rendered by the dialect's own literal processor
+ rather than interpolated into the SQL text.
+
+ Note this deliberately does *not* go through ``BaseEngineSpec``'s text
+ helper, which escapes ``:`` on every engine but Athena and would destroy
the
+ ``:value`` placeholder before it can be bound.
+ """
+ selections = []
+ for index, value in enumerate(values):
+ clause = sa.text(transform).bindparams(sa.bindparam("value",
value=value))
+ compiled = clause.compile(
+ dialect=dialect,
+ compile_kwargs={"literal_binds": True},
+ )
+ selections.append(f"{compiled} AS v{index}")
+ return "SELECT " + ", ".join(selections)
+
+
+def evaluate_transform(
+ database: Database,
+ catalog: str | None,
+ schema: str | None,
+ transform: str,
+ values: list[Any],
+) -> list[Any] | None:
+ """
+ Evaluate ``transform`` against the engine once per distinct value.
+
+ Returns one result per input value, positionally aligned with ``values``,
or
+ ``None`` if anything at all goes wrong. Failing open costs pruning, never
+ correctness: the chart query still runs, it just scans more partitions.
+
+ The probe is pinned to the dataset's catalog and schema so session settings
+ match the chart query as closely as the connection pool allows. It still
+ runs in a *different* session, which is why transforms calling
+ session-dependent functions are rejected at save time.
+ """
+ if not values:
+ return None
+
+ # Dedupe so a 200-value `IN` list costs one column, not 200.
+ distinct: list[Any] = []
+ seen: set[Any] = set()
+ for value in values:
+ key = _hashable(value)
+ if key not in seen:
+ seen.add(key)
+ distinct.append(value)
+
+ cache_key = _probe_cache_key(database, catalog, schema, transform,
distinct)
+ cached = _cache_get(cache_key)
+ if cached is None:
+ cached = _run_probe(database, catalog, schema, transform, distinct)
+ if cached is None:
+ # Deliberately not cached: a transient engine blip would otherwise
+ # keep the dataset pruning-free for the whole cache timeout.
+ return None
+ _cache_set(cache_key, cached)
+
+ evaluated = dict(
+ zip((_hashable(value) for value in distinct), cached, strict=False)
+ )
+ return [evaluated[_hashable(value)] for value in values]
+
+
+def _run_probe(
+ database: Database,
+ catalog: str | None,
+ schema: str | None,
+ transform: str,
+ distinct: list[Any],
+) -> list[Any] | None:
+ try:
+ sql = build_probe_sql(transform, distinct, _dialect_for(database))
+ frame = database.get_df(sql=sql, catalog=catalog, schema=schema)
+ if frame is None or frame.empty:
+ logger.warning(
+ "Partition transform probe returned no rows; skipping
mirroring"
+ )
+ return None
+ row = frame.iloc[0]
+ if len(row) < len(distinct):
+ # The results cannot be aligned back to their inputs; skipping
+ # beats guessing which value produced which column.
+ logger.warning(
+ "Partition transform probe returned %d values for %d inputs",
+ len(row),
+ len(distinct),
+ )
+ return None
+ return [row.iloc[index] for index in range(len(distinct))]
+ except Exception: # pylint: disable=broad-except
+ logger.warning(
+ "Partition transform probe failed; queries will not prune",
+ exc_info=True,
+ )
+ return None
+
+
+def _probe_cache_key(
+ database: Database,
+ catalog: str | None,
+ schema: str | None,
+ transform: str,
+ values: list[Any],
+) -> str:
+ """
+ Key on everything that can change the answer.
+
+ Note this cache is independent of the chart-data cache: it is keyed on the
+ transform and its inputs, so it is correct to share across every chart on
+ every dataset that happens to use the same transform.
+ """
+ payload = json.dumps(
+ [
+ database.id,
+ database.backend,
+ # The probe asks this connection to evaluate the transform, so the
+ # answer belongs to it. `id` outlives an edit to the URI or to
+ # `extra` (a session timezone, say), which would otherwise serve
+ # values computed against the old environment until the entry
+ # expires. `changed_on` covers the edits that do not show up here,
+ # such as a rotated password, without putting a secret in the key.
+ database.sqlalchemy_uri,
+ database.extra,
+ str(database.changed_on),
+ catalog,
+ schema,
+ transform,
+ [repr(value) for value in values],
+ ],
Review Comment:
**Suggestion:** The probe cache key omits the authenticated user, so OAuth2
users can reuse transform results computed under another user's database
permissions or session.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Security`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8b7617bfe1e04b69b3b340e7e61a9b51&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8b7617bfe1e04b69b3b340e7e61a9b51&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/partition_mapping.py
**Line:** 518:535
**Comment:**
*Security: The probe cache key omits the authenticated user, so OAuth2
users can reuse transform results computed under another user's database
permissions or session.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=a21c654592df694be3567015c51198ce648adf94b048ef472d1d0e9175a94e62&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=a21c654592df694be3567015c51198ce648adf94b048ef472d1d0e9175a94e62&reaction=dislike'>๐</a>
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -1327,6 +1335,27 @@ function DatasourceEditor({
) as Column[],
});
+ // A sync can remove the partition column at the source. Leave the editor
+ // showing a mapping that points at a column the table no longer has and
+ // the owner has no way to tell why pruning stopped.
+ const clearedMapping = clearDanglingPartitionMapping(
+ datasource,
+ columnChanges.finalColumns,
+ );
+ if (clearedMapping) {
+ onDatasourcePropChange(
+ 'partition_column',
+ clearedMapping.partition_column,
+ );
+ onDatasourcePropChange(
+ 'partition_mapped_column',
+ clearedMapping.partition_mapped_column,
+ );
Review Comment:
**Suggestion:** A completed metadata request uses its captured datasource
and can clear a newer mapping changed while that request was still pending.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Stale reference`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4d040ed7a4e146d593ca2e93554a8b7d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4d040ed7a4e146d593ca2e93554a8b7d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 1341:1353
**Comment:**
*Stale Reference: A completed metadata request uses its captured
datasource and can clear a newer mapping changed while that request was still
pending.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=2694a629e718295358bdb3c2b66589070bc42b6ce2f421a121a93bb513051234&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=2694a629e718295358bdb3c2b66589070bc42b6ce2f421a121a93bb513051234&reaction=dislike'>๐</a>
##########
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx:
##########
@@ -183,6 +183,8 @@ const DatasourceModal:
FunctionComponent<DatasourceModalProps> = ({
currency_code_column: datasource.currency_code_column ?? null,
normalize_columns: datasource.normalize_columns,
always_filter_main_dttm: datasource.always_filter_main_dttm,
+ partition_column: datasource.partition_column ?? null,
+ partition_mapped_column: datasource.partition_mapped_column ?? null,
Review Comment:
**Suggestion:** The modal always sends stale typed mapping fields, so
editing the mapping only in Extra is overwritten by the previously loaded
values.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=62961c850fab44bc9ba635e5a3d9ec3b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=62961c850fab44bc9ba635e5a3d9ec3b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/DatasourceModal/index.tsx
**Line:** 186:187
**Comment:**
*Api Mismatch: The modal always sends stale typed mapping fields, so
editing the mapping only in Extra is overwritten by the previously loaded
values.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=b9ea612f778beb4d9e550ac192158455d696f7d4c302ef086d886f719a075361&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44816&comment_hash=b9ea612f778beb4d9e550ac192158455d696f7d4c302ef086d886f719a075361&reaction=dislike'>๐</a>
--
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]
