bito-code-review[bot] commented on code in PR #43757:
URL: https://github.com/apache/superset/pull/43757#discussion_r4160487056
##########
superset/datasets/schemas.py:
##########
@@ -347,6 +367,10 @@ def fix_extra(self, data: dict[str, Any], **kwargs: Any)
-> dict[str, Any]:
datetime_format = fields.String(
allow_none=True, validate=[Length(1, 100), validate_python_date_format]
)
+ partition_value_transform = fields.String(allow_none=True)
+ # Bundles predating the field must not claim their transform preserves
+ # ordering, which would silently enable range mirroring on import.
+ partition_transform_is_monotonic = fields.Boolean(load_default=False)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Import rejects NULL monotonic flag</b></div>
<div id="fix">
`partition_transform_is_monotonic` is nullable in the DB (migration
`a7f3c2e91d84`), and the legacy datasource editor writes NULL for fields its
payload omits (see `TableColumn.partition_transform_is_monotonic` comment in
`connectors/sqla/models.py`). Export round-trips that NULL, and
`fields.Boolean` without `allow_none=True` rejects it with "Field may not be
null", failing the whole dataset import. Siblings `is_dttm`/`is_active` set
`allow_none=True`. ([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #aa7c0e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -0,0 +1,350 @@
+# 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.
+"""
+
+from __future__ import annotations
+
+import logging
+import re
+from dataclasses import dataclass
+from functools import lru_cache
+
+from flask_babel import lazy_gettext as _
+
+from superset.constants import LRU_CACHE_MAX_SIZE
+from superset.exceptions import SupersetParseError
+from superset.sql.parse import SQLStatement
+
+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"}
+
+
+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})
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant per-name AST walks</b></div>
<div id="fix">
`check_functions_present` wraps `get_disallowed_functions`, which walks the
whole AST and rebuilds the dialect parser-alias map on every call
(`superset/sql/parse.py:1798-1841`); the comprehension runs it once per name —
seven walks for one verdict. One
`statement.get_disallowed_functions(NON_DETERMINISTIC_FUNCTIONS)` call returns
the same matched subset in a single walk.
</div>
</div>
<small><i>Code Review Run #aa7c0e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/daos/dataset.py:
##########
@@ -592,9 +642,11 @@ def update_columns(
"""
cls._validate_column_date_formats(property_columns)
if override_columns:
- cls._override_columns(model, property_columns)
+ surviving_column_names = cls._override_columns(model,
property_columns)
else:
- cls._upsert_columns(model, property_columns)
+ surviving_column_names = cls._upsert_columns(model,
property_columns)
+
+ cls.clear_dangling_partition_mapping(model, surviving_column_names)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Mapping clear misses fetch_metadata path</b></div>
<div id="fix">
`clear_dangling_partition_mapping` runs only from `update_columns`, but
`SqlaTable.fetch_metadata` (RefreshDatasetCommand, the 'Sync columns from
source' action, and the MCP `update_dataset` sync_columns flow) also deletes
physical columns that vanished from the source (models.py drops non-calculated
leftovers) without touching `partition_column`/`partition_mapped_column`. A
source-side drop of the partition column still leaves the stored mapping
dangling on that path — the exact state this change sets out to prevent.
</div>
</div>
<small><i>Code Review Run #aa7c0e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/connectors/sqla/models.py:
##########
@@ -1924,8 +1956,48 @@ 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_
+ @property
+ def partition_filter_mapping_summary(self) -> dict[str, Any] | None:
+ """
+ Self-contained summary of the mapping for the Explore indicator.
+
+ Deliberately not a lookup into `columns`: `data_for_slices` prunes
+ columns no chart references, and the partition column is typically
+ referenced by none of them, so anything reading it out of
+ `datasource.columns` would work in Explore and break on dashboards.
+
+ `active` is the save path's own verdict rather than an approximation of
+ it. A transform that fails validation -- one missing `:value`, one that
+ does not parse -- is saved inactive on purpose, so a cheaper signal
here
+ would advertise a mapping that never mirrors a filter. The parse this
+ costs is memoized on `(transform, engine)` in `is_transform_active`,
and
+ datasets without a partition column never reach it.
+ """
+ if not self.partition_column:
+ return None
+
+ columns_by_name = {column.column_name: column for column in
self.columns}
+ mapped_column_name = self.partition_mapped_column or self.main_dttm_col
+ mapped_column = columns_by_name.get(mapped_column_name or "")
+ active = bool(
+ self.partition_column in columns_by_name
+ and mapped_column is not None
+ and mapped_column_name != self.partition_column
+ and is_transform_active(
+ mapped_column.partition_value_transform, self.database.backend
+ )
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated mapping validity checks</b></div>
<div id="fix">
`active` re-implements the three tier-1 structural checks of
`validate_partition_mapping` (partition column exists, mapped column exists,
not self-mapped) inline, sharing only the transform verdict via
`is_transform_active`. The docstring claims this is 'the save path's own
verdict', but the structural half is a parallel copy that can drift. Extract
the shared checks into a helper in `partition_mapping.py`.
</div>
</div>
<small><i>Code Review Run #aa7c0e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
