sadpandajoe commented on code in PR #43870: URL: https://github.com/apache/superset/pull/43870#discussion_r4350197811
########## superset/connectors/sqla/partition_mapping.py: ########## @@ -0,0 +1,3583 @@ +# 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 (...)`` ``T`` is a function *and* the + engine compares ``col`` + byte-exactly +``col >=|>|<|<= v``, ``TEMPORAL_RANGE`` only if ``T`` is monotonic +``col != v``, ``NOT IN``, ``LIKE``, ... never +=========================================== ============================= + +Monotonic here means non-decreasing, not strictly increasing: a bucketing +transform such as a day key is monotonic and maps a whole day of timestamps +onto one value. ``col < v`` then only implies ``T(col) <= T(v)``, so a strict +bound is mirrored non-strictly -- see `mirror_operator`. + +Equality under the engine's own rules +------------------------------------- +"``T`` is a function" gives ``col = v`` implies ``T(col) = T(v)`` for *value* +equality. The engine compares with *SQL* equality, and the two part company +wherever that comparison is not byte-exact: under a case-insensitive collation +a stored ``country`` of ``'us'`` satisfies a filter for ``'US'``, while the +mirror ``region_key = hex('US')`` excludes the row and the chart loses it with +nothing to indicate why. Trailing-space-insensitive ``CHAR`` comparison says +the same thing about padding. + +So equality and ``IN`` are withdrawn for a *string* mapped column unless the +engine spec declares ``binary_string_comparison`` -- `equality_mirrors_safely`. +Numeric and temporal comparison is exact everywhere and is untouched, and range +mirroring is untouched too: it already rests on the owner declaring ``T`` +order-preserving *with respect to the column's own order*, which is an +assertion about the very semantics this is checking for. + +What the gate cannot see is a column declaring its own collation +(``country COLLATE NOCASE``) on an otherwise byte-exact engine. Nothing in +SQLAlchemy's reflection or the engine specs exposes that, so there it remains +the owner's assumption, like ``p = T(mapped_col)`` itself. + +Resolution of the value the engine compares +------------------------------------------- +Every row of the table above assumes the engine compares the value the mirror +was built from. It does not always. A ``DATE`` column compared against +``'2026-07-06 10:08:11'`` is compared on the date part alone -- whether the +engine writes the literal itself (``TO_DATE('2026-07-06', 'YYYY-MM-DD')`` on +Postgres, ``DATE '2026-07-06'`` on Presto, Trino, BigQuery and some two dozen +others) or applies an implicit cast to a bound string. The filter therefore +keeps the whole of that day while ``p = T(2026-07-06 10:08:11)`` keeps one +instant of it: narrower than the predicate it stands in for, which is the one +thing a mirror may never be. + +What an engine throws away is detected from the column's type rather than +enumerated per engine -- `ExploreMixin._engine_literal_resolution` -- and what +to do about it is a property of the *operator*, not of the value: + +* a bound rounds outward, backward for a lower one and forward for an upper one, + through `ExploreMixin._round_bounds_outward`. The mirror then reads one extra + bucket at the edge, and the original predicate is still in the query to + exclude the rows that bucket adds. +* an equality, or one member of an ``IN`` list, has nowhere to widen to: these + predicates are ``AND``-ed onto the query, so "or the day either side" is not + something the mirror can express. They decline, which costs the query its + pruning and nothing else. + +So a ``DATE``-typed mapped column mirrors a range at day granularity, and does +not mirror an equality whose value carries a time of day at all. Map a +``TIMESTAMP`` column instead and every operator is available again. + +Writing the value into the probe +-------------------------------- +The probe evaluates the transform at a *literal*, because it runs with no bound +parameters at all -- so every value is written into SQL text by the driver's +literal renderer and read back by the engine's lexer, and the two do not always +agree. SQLAlchemy's default renderer doubles apostrophes and leaves backslashes +alone; Hive honours backslash escapes and not doubling, so ``'O''Brien'`` is +read there as two adjacent literals and ``'x\\''`` ends four characters into a +value the filter carries whole. A disagreement is either a mirror built from a +value nobody filtered on -- dropping rows the filter keeps -- or, where the +value closes its own quote, an expression of the filterer's choosing running in +the probe. + +Which side is right is not knowable from the driver: it is a per-session server +setting (``standard_conforming_strings``, ``NO_BACKSLASH_ESCAPES``, +``spark.sql.parser.escapedStringLiterals``), and SQLAlchemy's own view of it is +only correct on a dialect that has connected. So escaping is not what makes +this safe. `literal_round_trips` renders the literal and reads it back in the +engine's own dialect instead, and a value that does not survive the trip is not +mirrored -- the usual trade, pruning for correctness. `probe_sql_is_evaluable` +re-reads the finished statement behind it. + +The probe's *answers* make the same trip, in the other direction. The chart +query is compiled with ``literal_binds``, so a probed partition key is rendered +into its SQL by the same renderer and read by the same lexer -- and +``concat(:value, chr(92), 'b')`` turns the unremarkable input ``a`` into the key +``a\\b``, which Hive reads back as ``a`` followed by a backspace. So +`_predicates_from_probe` asks `literal_round_trips` about each result too, and +declines the request rather than emitting a predicate no row can satisfy: the +mirror is ``AND``-ed onto the query, so a mirror that matches nothing costs the +chart its rows rather than its pruning. + +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 numbers +import re +from collections.abc import Sequence +from dataclasses import dataclass +from datetime import date, datetime +from functools import lru_cache +from typing import Any, cast, TYPE_CHECKING + +import numpy as np +import pandas as pd +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.constants import LRU_CACHE_MAX_SIZE, TimeGrain +from superset.exceptions import ( + QueryClauseValidationException, + SupersetParseError, + SupersetSecurityException, +) +from superset.extensions import cache_manager, feature_flag_manager +from superset.sql.parse import SQLScript, SQLStatement +from superset.superset_typing import FilterValues +from superset.utils import core as utils, json +from superset.utils.core import FilterOperator, get_username + +if TYPE_CHECKING: + from superset.connectors.sqla.models import SqlaTable, TableColumn + from superset.db_engine_specs.base import BaseEngineSpec + from superset.models.core import Database + +logger = logging.getLogger(__name__) + +FEATURE_FLAG = "PARTITION_FILTER_MAPPING" + +#: Longest transform accepted anywhere. A transform is one SQL expression an +#: owner types by hand, so this is generous; the point is that it is bounded. +#: The bound matters because `is_transform_active` parses the stored value on +#: each Explore load, and an unbounded string would make that parse the +#: expensive part of rendering a chart. +#: +#: The typed column field, the import schema and the preview request each +#: enforce it through a marshmallow `Length` validator, which is what produces +#: the per-field message those three callers want. `stored_expression_error` +#: enforces it too, for the paths that load no schema -- the deprecated +#: `POST /datasource/save/` is one, and it writes straight through +#: `update_from_object`. +MAX_TRANSFORM_LENGTH = 1024 + +#: 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") + +#: The opening and closing delimiters of each kind of Jinja block. The probe +#: would render these in a different context at a different time from the chart +#: query, so a transform holding one is rejected at save time. +JINJA_BLOCK_DELIMITERS = (("{{", "}}"), ("{%", "%}"), ("{#", "#}")) + +#: MySQL and MariaDB "executable comments": ``/*!50000 ... */`` and +#: ``/*M!50000 ... */`` are comments to every other reader and *statement text* +#: to those two engines. +#: +#: This is the one construct that breaks the premise the probe's shape gates +#: rest on -- that the parse this module inspects and the statement the engine +#: runs are the same statement. ``1 /*!50000 + (SELECT secret FROM vault) */`` +#: parses with no sub-query and no FROM, clears `stored_expression_error` and +#: `probe_sql_is_evaluable` alike, and then reads a table on MySQL. The result +#: comes back to a dataset owner through the preview's ``emitted_predicate``, +#: with no RLS applied and no SQL Lab access needed. +#: +#: Refused on every engine rather than only the two that honour it. A transform +#: is a scalar function of ``:value``; there is no legitimate reason for one to +#: carry a comment of any kind, let alone this one, and a per-engine gate would +#: store a transform on Postgres that detonates when the dataset is re-pointed +#: at MySQL. +EXECUTABLE_COMMENT_RE = re.compile(r"/\*M?!") + +#: A PostgreSQL Unicode-escaped identifier, ``U&"..."``. +#: +#: The second construct that breaks the premise above, and in the same way: +#: ``U&"\\0071uery_to_xml"(...)`` parses as an anonymous function whose name is +#: the *literal* ``\\0071uery_to_xml``, so the function denylist is matching a +#: name the engine never sees -- PostgreSQL decodes the escape and runs +#: ``query_to_xml``, handing a dataset owner rows from outside their RLS scope +#: through the preview's ``emitted_predicate``. +#: +#: Normalizing the escapes and re-checking would be the other repair, and is +#: worse: it means reimplementing one engine's identifier decoding well enough +#: to be a security boundary. Refusing the construct is exact, and costs +#: nothing real -- a transform is a scalar function of ``:value``, and there is +#: no reason to name a function or column through an escape sequence. +#: +#: Refused on every engine, like the executable comment, so a transform stored +#: on SQLite cannot detonate when the dataset is re-pointed at PostgreSQL. The +#: ``&`` may be separated from the ``U`` by nothing at all, but whitespace or a +#: comment between the prefix and the string is not accepted by PostgreSQL +#: either, so the strict form is the whole surface. +#: +#: Matched in raw text, so the prefix is refused inside a string literal too. +#: That is the same trade the executable comment makes and has the same +#: justification: a gate whose contract is to fire *before* the parse cannot +#: ask the parse whether the text was quoted. ``concat(:value, 'u&"x"')`` is +#: refused, which costs an owner nothing anyone writes. +UNICODE_ESCAPED_IDENTIFIER_RE = re.compile(r'\bU&"', re.IGNORECASE) + +#: 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" + +#: Substituted for ``:value`` to find out *where* the placeholder landed, which +#: `_PARSE_STANDIN` cannot answer: ``NULL`` is a keyword, so after substitution +#: the parse tree no longer records that a placeholder was ever there. An +#: identifier does leave a trace -- a column reference -- and one that is +#: missing is a placeholder the engine will never evaluate. +#: +#: Named so it cannot plausibly collide with a real column, but a transform that +#: writes it anyway is refused rather than counted. A hand-written occurrence +#: does not merely make the count disagree: it *supplies* the column reference a +#: non-executable placeholder failed to produce, and the two cancel exactly. See +#: `placeholder_is_executable`. +_PLACEHOLDER_STANDIN = "superset_pfm_value_standin" + +#: 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: a transform such as +#: ``datediff(day, :value, LOCALTIMESTAMP)`` for a relative partition key is +#: evaluated once and then keeps emitting yesterday's answer, so the chart +#: silently drops rows after midnight. +#: +#: Matched by SQL function name, so every engine's spelling of "now" has to be +#: named -- the ANSI four were not enough. Listed rather than delegated to the +#: engine specs because the risk is the same everywhere and a spec that had not +#: got round to declaring its own would fail open; a name no engine has is +#: inert, so over-listing costs nothing. +NON_DETERMINISTIC_FUNCTIONS = { + # ANSI, and the engines that spell it this way. + "CURRENT_DATE", + "CURRENT_TIME", + "CURRENT_TIMESTAMP", + "LOCALTIME", + "LOCALTIMESTAMP", + "NOW", + "RAND", + "RANDOM", + "UUID", + # PostgreSQL. `now()` is statement-stable; these two are not even that. + "CLOCK_TIMESTAMP", + "STATEMENT_TIMESTAMP", + "TIMEOFDAY", + # MySQL and MariaDB. + "CURDATE", + "CURTIME", + "SYSDATE", + "UTC_DATE", + "UTC_TIME", + "UTC_TIMESTAMP", + "UUID_SHORT", + # Oracle. + "SYSTIMESTAMP", + "SYS_GUID", + # SQL Server. + "GETDATE", + "GETUTCDATE", + "SYSDATETIME", + "SYSUTCDATETIME", + "SYSDATETIMEOFFSET", + "NEWID", + # BigQuery. + "CURRENT_DATETIME", + "GENERATE_UUID", + # Snowflake. + "RANDSTR", + "UUID_STRING", + # ClickHouse. + "NOW64", + "RANDCANONICAL", + "GENERATEUUIDV4", + # H2 and Derby, which the test fixtures use. + "RANDOM_UUID", +} + +#: The wall-clock names an engine evaluates with no parentheses at all. +#: +#: These parse as *column references*, not functions, so the name-based walk +#: below cannot see them: `datediff(UTC_DATE, :value)` on MySQL and +#: `:value - SYSDATE` on Oracle both cleared the gate and froze probe time into +#: a cached mirror. +#: +#: A subset of `NON_DETERMINISTIC_FUNCTIONS` rather than the whole of it, +#: because refusing a bare name costs an owner a column genuinely called that. +#: Only the names SQL lets you write without ``()`` are listed -- the datetime +#: keywords -- so `rand`, `uuid` and `newid`, which always need parentheses and +#: are plausible column names, stay out. +BARE_WALL_CLOCK_KEYWORDS = frozenset( + { + "current_date", + "current_time", + "current_timestamp", + "localtime", + "localtimestamp", + "utc_date", + "utc_time", + "utc_timestamp", + "sysdate", + "systimestamp", + } +) + +#: 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, +} + + +#: Every operator that can be mirrored under *some* transform. The preview +#: endpoint accepts these; whether a given one actually mirrors still depends on +#: the monotonicity declaration. +MIRRORABLE_OPERATORS = MIRRORABLE_ALWAYS | MIRRORABLE_IF_MONOTONIC + +#: Operators the preview endpoint can construct. `TEMPORAL_RANGE` is mirrored by +#: the query path, but as *two* bounds that `_collect_partition_mirror_range` +#: decomposes a time range into -- the preview request carries sample values, not +#: a since/until pair, so there is no range for it to build. The editor never +#: asks for one either: `previewOperatorFor` sends `>=`, `=` or `IN`. +PREVIEWABLE_OPERATORS = MIRRORABLE_OPERATORS - {FilterOperator.TEMPORAL_RANGE} + + +class Unmirrorable: + """ + A filter no mirror can stand in for, and why. + + Returned by `ExploreMixin.mirror_probe_request` for a value the engine + compares more coarsely than the filter carries, one it cannot render as a + literal, one whose rendered literal the engine would not read back as the + same value, one in a time frame the transform would discard, or an ``IN`` + list with a member of any of those kinds. An object rather than `None`, + because `None` is a filter value in its own right. + + The reason rides along because the preview has to explain the decline to + the owner, and the two declines want different advice: "the engine compares + less of this value than you typed, so map a column it compares in full" is + sound guidance for a `DATE` column and actively misleading for an + offset-bearing bound on a `TIMESTAMP` one, which already is such a column. + Carried rather than re-derived so the preview's explanation cannot come + apart from the chart path's decision. + + Lives here rather than beside that method so the preview can read it too: + `superset.models.helpers` imports this module, so these can only cross the + boundary in this direction. Both callers recognise the same instances -- a + preview that answered "valid" for a request the chart path declines is the + drift this is here to prevent. + """ + + __slots__ = ("reason",) + + def __init__(self, reason: str) -> None: + self.reason = reason + + def __repr__(self) -> str: + return f"<Unmirrorable {self.reason}>" + + +#: The ordinary decline: whatever the mirror cannot be built from, with the +#: engine's own comparison resolution as the reason an owner can act on. +UNMIRRORABLE = Unmirrorable("resolution") + +#: A value carrying a UTC offset, declined whatever the column's resolution. +#: A transform such as ``CAST(:value AS TIMESTAMP)`` drops the offset, so even +#: an engine that compares the whole instant would be probed in the wrong +#: frame -- which is not something mapping a different column fixes. +UNMIRRORABLE_OFFSET = Unmirrorable("offset") + +#: A value the driver and the engine would not spell the same way, declined +#: whatever the column's resolution and whatever the operator. Nothing about +#: the column or the transform fixes it -- see `literal_round_trips` -- so the +#: advice the resolution reason carries would send an owner after the wrong +#: thing entirely. +UNMIRRORABLE_ESCAPING = Unmirrorable("escaping") + + +def is_unmirrorable(value: Any) -> bool: + """Whether `mirror_probe_request` declined this request.""" + return isinstance(value, Unmirrorable) + + +#: A transform that preserves ordering need not be *strictly* increasing: a day +#: key such as ``to_char(ts, 'YYYYMMDD')`` maps a whole day of timestamps onto +#: one value. For such a ``T``, ``col < v`` only implies ``T(col) <= T(v)`` and +#: ``col > v`` only implies ``T(col) >= T(v)`` -- the rows in the bucket at the +#: boundary satisfy the non-strict form and not the strict one. A strict mirror +#: therefore drops rows the filter keeps, which is the one thing a mirror may +#: never do, so a strict bound is mirrored non-strictly. +_NON_STRICT_EQUIVALENT = { + FilterOperator.GREATER_THAN: FilterOperator.GREATER_THAN_OR_EQUALS, + FilterOperator.LESS_THAN: FilterOperator.LESS_THAN_OR_EQUALS, +} + + +def mirror_operator(operator: FilterOperator) -> FilterOperator: + """ + The operator a mirrored predicate uses to stand in for ``operator``. + + Strict bounds widen; everything else passes through. Idempotent, so it is + safe wherever a request may already have been widened. + + The widened bound reads one extra partition at the edge, and the original + predicate is still in the query to exclude the rows that partition adds -- + the same trade as the grain widening and the ``NULL``-partition escape. + Declaring the transform strictly increasing would buy that partition back, + which is not worth a second property only the owner can vouch for. + """ + return _NON_STRICT_EQUIVALENT.get(operator, operator) + + +def mirrorable_operators( + is_monotonic: bool, *, equality_is_safe: bool = True +) -> set[FilterOperator]: + """ + The operators whose predicates may be mirrored onto the partition column. + + :param is_monotonic: whether the owner declared the transform + order-preserving + :param equality_is_safe: whether the engine's ``=`` on the mapped column + compares the way ``T`` was written against -- see + `equality_mirrors_safely` + + Both call sites -- the query path through `PartitionMapping.mirrors` and the + Explore indicator through `partition_filter_mapping_summary` -- come through + here, which is what keeps the glyph from promising pruning the SQL will not + do. + """ + operators = MIRRORABLE_ALWAYS if equality_is_safe else set() + if is_monotonic: + operators = operators | MIRRORABLE_IF_MONOTONIC + return set(operators) + + +def _native_generic_type( + column: "TableColumn", +) -> utils.GenericDataType | None: + """ + What the engine spec makes of ``column``'s declared type. + + `TableColumn.type_generic` answers ``TEMPORAL`` for every column carrying + ``is_dttm``, which is the right answer for a chart's time axis and the + wrong one for a comparison: an epoch ``BIGINT`` is a number to the engine + however it is labelled here. Falls back to the flag where the spec has no + answer, so a type Superset cannot resolve is no worse off than before. + """ + try: + column_spec = column.db_engine_spec.get_column_spec( + column.type, db_extra=column.db_extra + ) + except Exception: # pylint: disable=broad-except # noqa: BLE001 + column_spec = None + if column_spec is not None and column_spec.generic_type is not None: + return column_spec.generic_type + try: + return column.type_generic + except Exception: # pylint: disable=broad-except # noqa: BLE001 + return None + + +#: A declared type naming an array: PostgreSQL's ``TEXT[]`` / ``INTEGER[][]`` +#: / ``int[3]``, and the ``ARRAY<...>`` / ``ARRAY(...)`` / ``Array(...)`` +#: spellings of Hive, Spark, Trino, BigQuery and ClickHouse. +#: The bracket body is written so no two runs can match the same blanks: +#: ``\s*\d*\s*`` backtracks polynomially on a long run of spaces. +_ARRAY_TYPE_RE = re.compile(r"(?:\[\s*(?:\d+\s*)?\])+\s*$|^\s*ARRAY\b", re.IGNORECASE) + + +def declares_array_type(column_type: str | None) -> bool: + """ + Whether a declared column type is an array, whatever the engine spec says. + + The engine spec only reports ``MULTI_VALUE`` where it has opted into + ``supports_multivalue_columns``; PostgreSQL has not, and resolves ``TEXT[]`` + to ``STRING`` and ``INTEGER[]`` to ``NUMERIC``. So an array partition column + there cleared the array refusal, previewed valid, and every equality chart + on the mapping failed with ``malformed array literal``. The declared type + is what the engine will compare, so it is read directly as well. + """ + return bool(column_type and _ARRAY_TYPE_RE.search(column_type)) + + +#: Declared types whose comparison ignores trailing blanks. +#: +#: The fixed-width character types, which the SQL standard has compare as +#: though both sides were padded to the same length -- so ``'US'`` and +#: ``'US '`` are equal in a ``CHARACTER(3)``, while ``lower()`` of the two is +#: not. Spelled out rather than matched by prefix because the variable-width +#: names start with the same letters: ``VARCHAR`` and ``CHARACTER VARYING`` +#: compare the blanks and must not land here. +PADDED_CHARACTER_TYPES = frozenset( + { + "CHAR", + "NCHAR", + # PostgreSQL's own name for ``CHARACTER``, which is what reflection + # reports for a ``CHAR(n)`` column there. + "BPCHAR", + "CHARACTER", + "NATIONAL CHAR", + "NATIONAL CHARACTER", + } +) + + +def _is_padded_character_type(column: TableColumn) -> bool: + """ + Whether ``column``'s declared type compares without its trailing blanks. + + Read off the declared type rather than the engine spec's resolved one: the + specs map ``CHAR`` and ``VARCHAR`` onto the same `sqlalchemy.String`, so + the distinction this needs is gone by then. The length suffix is dropped + and the name matched against `PADDED_CHARACTER_TYPES`, so an unfamiliar + spelling answers ``False`` and the engine-level flag decides as before -- + this gate adds a decline rather than lifting one. A column with no + declared type has nothing to read, and answers the same way. + """ + if not isinstance(column.type, str): + return False + native = re.sub(r"\s+", " ", column.type.strip().upper()) + return native.split("(")[0].strip() in PADDED_CHARACTER_TYPES + + +def equality_mirrors_safely( + column: TableColumn | None, + db_engine_spec: type[BaseEngineSpec], +) -> bool: + """ + Whether ``col = v`` on this column implies ``T(col) = T(v)`` in the engine. + + The operator matrix calls equality safe "for any function ``T``" because + ``T`` is a function, so ``col = v`` gives ``T(col) = T(v)``. That reasons + about *value* equality while the engine reasons about *SQL* equality, and + the two part company under any comparison that is not byte-exact: with a + case-insensitive collation a stored ``country`` of ``'us'`` satisfies a + filter for ``'US'``, but the mirror ``region_key = hex('US')`` excludes the + row and the chart loses it with no indication why. Trailing-space-insensitive + ``CHAR`` comparison says the same thing about padding. + + Only string columns are at risk; numeric and temporal comparison is exact + everywhere. And only equality: range mirroring already requires the owner + to declare ``T`` order-preserving *with respect to the column's own order*, + which is an assertion about the same comparison semantics this is checking + for -- so the declaration covers it where equality has nothing to cover it. + + Padding is asked about per column, because it is a property of the *type* + rather than of the engine. ``binary_string_comparison`` speaks for an + engine's default text comparison, and on PostgreSQL -- which declares it + -- a ``CHAR(2)`` still matches a filter for ``'US '`` against a stored + ``'US'``, because the standard says a padded character type compares + without its trailing blanks. The mirror ``region_key = lower('US ')`` then + excludes the row. So a fixed-width character type declines whatever the + engine says, and an engine that ignores trailing blanks on its *variable* + types says so by not declaring the flag at all (see `RedshiftEngineSpec`). + + The type asked for is the engine spec's reading of the column's declared + type, not `TableColumn.type_generic`, which answers ``TEMPORAL`` for + anything carrying ``is_dttm``. A ``VARCHAR`` flagged temporal is still text + to the engine: on MySQL a filter for ``'2026-01-01T00:00:00'`` matches a + stored ``'...t00:00:00'``, while the mirror ``p = hex('...T...')`` excludes + it -- and asking the semantic flag skipped the check entirely. + + A type the spec cannot classify declines, which is what the paragraph above + promises and what only the exception branch used to do. PostgreSQL's + ``citext`` matches no type pattern, so it answered ``None`` -- neither + string nor not-string -- and was waved through, which is precisely wrong + for a type whose whole purpose is case-insensitive comparison. Declining + costs the pruning; admitting it costs rows. + + What this cannot see is a column that declares its own collation + (``country COLLATE NOCASE``) on an otherwise byte-exact engine. Neither + SQLAlchemy's reflection nor the engine specs expose it, so on such a column + the assumption stays where ``p = T(mapped_col)`` already is: with the owner. + """ + if column is None: + return True + # Ahead of the generic-type question, which a padded type can lose: the + # specs map ``CHARACTER`` onto no generic type at all on some engines, and + # a fixed-width character type is a string whether or not Superset can say + # so from the name. + if _is_padded_character_type(column): + return False + generic_type = _native_generic_type(column) + if generic_type is None: + return False + if generic_type != utils.GenericDataType.STRING: + return True + return bool(db_engine_spec.binary_string_comparison) Review Comment: A PostgreSQL `JSONB` column maps to `GenericDataType.STRING`, so this returns the engine's `binary_string_comparison` (true) and equality mirroring is allowed. `JSONB` does not keep input spelling: the stored value `{"a":1}` reads back as `{"a": 1}`. With mapped column `j JSONB`, partition column `p TEXT`, and transform `CAST(:value AS TEXT)`, a filter `j == '{"a":1}'` matches the row, but the probe renders the compact input and adds `p = '{"a":1}' OR p IS NULL`, which excludes the row whose `p` is `{"a": 1}`. Should `JSON`/`JSONB` decline here like the padded character types do? ########## superset/models/helpers.py: ########## @@ -4449,36 +4558,777 @@ def dttm_sql_literal(self, dttm: datetime, col: "TableColumn") -> str: return f"""'{dttm.strftime("%Y-%m-%d %H:%M:%S.%f")}'""" - def get_time_filter( # pylint: disable=too-many-arguments # noqa: C901 + def _mirror_probe_value(self, dttm: datetime, col: Optional["TableColumn"]) -> Any: + """ + ``dttm`` in the mapped column's own stored representation. + + The predicate this mirror stands in for compares the column against + `dttm_sql_literal(dttm, col)`, so the transform has to be probed with + the same value. On a column stored as an epoch integer, ``T`` is a + function of an integer: probing it with a `datetime` either returns + something unrelated to the partition keys or raises, and a raise costs + the dataset its pruning silently. + + Only the ``python_date_format`` branches of `dttm_sql_literal` are + reproduced, because only those produce a *value*. `convert_dttm` yields + engine-specific SQL text, which cannot be bound as a parameter; its own + divergence from the probe is handled by widening the bounds instead + (`_engine_literal_resolution`). + + In `dttm_sql_literal`'s own order, though: it asks ``convert_dttm`` + first and reaches the format only when the engine answers nothing. A + column can carry both -- a TIMESTAMP with a ``%Y%m%d`` + ``python_date_format``, or an entry in + ``python_date_format_by_column_name`` -- and applying the format + unconditionally probed a value the real predicate never compares. + ``event_time >= '2026-01-01 00:00:00'`` mirrored as + ``partition_col >= '20260101'``, and a January 15 row keyed + ``2026-01-15`` sorts below that string, so the chart lost a row the + filter keeps. + + Where the engine does answer, the `datetime` is returned unchanged and + the widening path takes over -- which is exactly what already happens + for every temporal column that carries no format at all. + """ + if col is None: + return dttm + + if col.type and self.db_engine_spec.convert_dttm( + col.type, dttm, db_extra=self.db_extra + ): + # The format is unreachable in `dttm_sql_literal` for this column, + # so reproducing it here would describe a different bound. + return dttm + + tf = col.python_date_format + if not tf and self.db_extra: + tf = self.db_extra.get("python_date_format_by_column_name", {}).get( + col.column_name + ) + if not tf: + return dttm + + if tf in EPOCH_FORMATS: + dttm_tz_aware = dttm + if dttm_tz_aware.tzinfo is None: + dttm_tz_aware = dttm_tz_aware.replace(tzinfo=timezone.utc) + return int(dttm_tz_aware.timestamp()) * EPOCH_FORMATS[tf] + return dttm.strftime(tf) + + def _engine_literal_resolution( + self, dttm: datetime, col: Optional["TableColumn"] + ) -> LiteralResolution: + """ + How much of ``dttm`` survives into this engine's literal for ``col``. + + The real predicate compares `dttm_sql_literal(dttm, col)`, so whatever + that literal throws away is a bound the engine never sees. A mirror + derived from the full precision then describes a *narrower* range than + the predicate it stands in for -- and narrower means it drops rows the + filter keeps. + + Two things get thrown away in practice: + + - the sub-second part. SQLite renders a timestamp with + ``timespec="seconds"``. + - the time of day entirely. Every engine spec with a ``types.Date`` + branch renders ``dttm.date().isoformat()``, so Presto, Trino, + BigQuery and some two dozen others answer `DATE '2026-01-01'` for a + bound of 10:00 -- and `T(DATE '2026-01-01')` is below the mirror's + own lower bound, so the whole first day is dropped. + + Detected rather than enumerated per engine, because the branch depends + on the column's type as well as the spec: instants differing only in the + part under test render as the same text exactly when the engine has + thrown that part away. + + The day test uses two fixed times of day rather than ``dttm`` and one + alternative, so a bound that happens to sit on the hour the alternative + uses cannot read as "no time at all". Neither probe sits at midnight on + purpose: SQLite renders a DATE column at exactly midnight as a bare + date, so a midnight probe would differ from its alternative for a + reason that has nothing to do with resolution. + + Likewise both sub-second probes carry a non-zero remainder. Comparing + against ``dttm`` truncated to the second looks like the obvious test and + is wrong for the same formatting-boundary reason. + + A non-temporal column has no answer here, and asking anyway gets a wrong + one. SQLite's ``convert_dttm`` renders for ``types.String`` as well as + for the date types, so a ``VARCHAR`` mapped column reads as + second-resolution -- and a filter value that happens to *look* like an + instant then has its bound rounded as though the engine had truncated + it. On a VARCHAR ``code`` mapped onto a VARCHAR ``p`` by ``:value``, + ``code < '2026-07-06T10:08:11.500000'`` was mirrored as + ``p <= '2026-07-06 10:08:12.000000'``; a space sorts before ``T``, so a + row holding ``2026-07-06T09:00:00`` fell outside the mirror while + matching the filter. + + The guard lives here rather than in each caller so this and + `_column_literal_resolution` cannot disagree about which column is + coarse -- which is the same reason that helper delegates to this one. + + "Temporal" is the column's generic type, not just its ``is_dttm`` flag. + `get_time_filter` renders a bound through ``convert_dttm`` for any + ``DATE`` column, flagged or not, so a ``DATE`` column left unflagged + still compares a bare day -- and skipping the resolution check there + would hand the probe 10:00 for a bound the engine reads as midnight. + """ + if col is None or not col.type or col.type_generic != GenericDataType.TEMPORAL: + return LiteralResolution.FULL + + def render(moment: datetime) -> Optional[str]: + return self.db_engine_spec.convert_dttm( + col.type, moment, db_extra=self.db_extra + ) + + try: + early = render(dttm.replace(hour=1, minute=2, second=3, microsecond=444444)) + late = render( + dttm.replace(hour=22, minute=33, second=44, microsecond=555555) + ) + if early is not None and early == late: + return LiteralResolution.DAY + + # Flooring to the second is a no-op on a bound that has no + # sub-second part, so there is nothing to detect. + if not dttm.microsecond: + return LiteralResolution.FULL + + first = render(dttm.replace(microsecond=111111)) + second = render(dttm.replace(microsecond=222222)) + except Exception: # pylint: disable=broad-except # noqa: BLE001 + return LiteralResolution.FULL + + if first is not None and first == second: + return LiteralResolution.SECOND + return LiteralResolution.FULL + + def _round_bounds_outward( self, - time_col: "TableColumn", + start_dttm: Optional[datetime], + end_dttm: Optional[datetime], + col: Optional["TableColumn"], + ) -> tuple[Optional[datetime], Optional[datetime]]: + """ + The bounds widened to whatever the engine's literal actually resolves to. + + Rounding outward is never narrower than the predicate being mirrored, + which is the only property the mirror has to hold. + """ + resolutions = { + self._engine_literal_resolution(bound, col) + for bound in (start_dttm, end_dttm) + if bound is not None + } + + if LiteralResolution.DAY in resolutions: + # The engine compares a bare date, so the real predicate's lower + # bound is the start of that day and its upper bound admits the + # whole of the day before. Anything finer excludes rows it keeps. + if start_dttm is not None: + start_dttm = start_dttm.replace( + hour=0, minute=0, second=0, microsecond=0 + ) + if end_dttm is not None and end_dttm != end_dttm.replace( + hour=0, minute=0, second=0, microsecond=0 + ): + end_dttm = end_dttm.replace( + hour=0, minute=0, second=0, microsecond=0 + ) + timedelta(days=1) + elif LiteralResolution.SECOND in resolutions: + if start_dttm is not None: + start_dttm = start_dttm.replace(microsecond=0) + if end_dttm is not None and end_dttm.microsecond: + end_dttm = end_dttm.replace(microsecond=0) + timedelta(seconds=1) + + return start_dttm, end_dttm + + def _literal_is_coarser_than( + self, dttm: datetime, col: Optional["TableColumn"] + ) -> bool: + """ + Whether the engine's literal for ``dttm`` throws away part of it. + + Unlike a range, an equality cannot be widened outward, so the only safe + answer when the engine compares less than the full value is to decline + the mirror. + """ + resolution = self._engine_literal_resolution(dttm, col) + if resolution is LiteralResolution.DAY: + return dttm != dttm.replace(hour=0, minute=0, second=0, microsecond=0) + if resolution is LiteralResolution.SECOND: + return bool(dttm.microsecond) + return False + + def _column_literal_resolution( + self, col: Optional["TableColumn"] + ) -> LiteralResolution: + """ + How much of *any* value this engine compares on ``col``. + + `_engine_literal_resolution` answers about a particular instant, which + is what a mirror derived from one needs. Two callers have no instant to + ask about -- the summary the Explore glyph reads, and a filter value + `_instant_from_filter_value` cannot read as an instant at all -- and + both need the column's own answer. Through one helper so that the glyph + and the query path cannot disagree about which column is coarse, which + is the same reason `mirrorable_operators` is computed once. + + A non-temporal column has no answer, and asking anyway gets a wrong one: + SQLite's ``convert_dttm`` renders for ``types.String`` as well as for the + date types, so a reference instant makes a ``VARCHAR`` mapped column look + second-resolution -- and ``country = 'US'``, the text half of this + feature, reads as a value the engine compares coarsely. + + That guard is `_engine_literal_resolution`'s, not this function's. It + used to be here, on the belief that the per-instant callers never met + the case because they arrive only with a value that parsed as an + instant -- which is untrue of a text column holding ISO-shaped text, + where the value parses and the column is still not temporal. + """ + return self._engine_literal_resolution(_RESOLUTION_REFERENCE_INSTANT, col) + + def _round_bound_outward( + self, + instant: datetime, + col: Optional["TableColumn"], + *, + is_upper: bool, + ) -> datetime: + """ + One bound widened to whatever the engine resolves ``col`` to. + + `_round_bounds_outward` answers for a since/until pair, because a time + range is the shape it was written for. A single comparison is one end of + such a pair with the other end absent, and the widening is per-end + already -- a lower bound moves back, an upper bound forward. + + Narrowing the return type is the other half of the point: a bound handed + in non-`None` comes back non-`None`, and saying so here saves every + caller from re-establishing it. + """ + start_dttm, end_dttm = self._round_bounds_outward( + None if is_upper else instant, + instant if is_upper else None, + col, + ) + rounded = end_dttm if is_upper else start_dttm + return rounded if rounded is not None else instant + + def _in_column_number_type(self, value: Any, col: Optional["TableColumn"]) -> Any: + """ + A numeric ``value`` in the number type the mapped column stores. + + The engine compares a number against the column in the column's own + domain, so ``n = '1.0'`` on an integer ``n`` matches the row holding + ``1``. `filter_values_handler` hands over ``1.0``, though, and a + transform whose output depends on the representation -- + ``CAST(:value AS TEXT)`` answers ``'1.0'`` where the row's key is + ``'1'`` -- then mirrors to a partition value no matching row holds. + Probing at the value the column would hold keeps ``T(col) = T(v)``. + + Integer columns only. A non-integral value on an integer column is + returned unchanged -- it matches no row, so its mirror cannot drop one. + + Nothing is converted for an approximate or exact decimal column, even + though ``1`` and ``1.0`` are the same number to a ``REAL``. What the + transform sees is the *literal*, and how an engine renders one is not + something the engine spec exposes: on PostgreSQL ``CAST(1.0 AS TEXT)`` + is ``'1.0'`` while the ``REAL`` column holding that value renders as + ``'1'``, so coercing the value to ``float`` mirrored onto a partition + key no matching row holds -- the same failure this helper exists to + stop, in the other direction. A ``DECIMAL`` column says it louder, + since its text also depends on a scale the spec does not carry. For + both, the rendering stays the owner's assumption, like + ``p = T(mapped_col)`` itself. + """ + if ( + col is None + or not col.type + or isinstance(value, bool) + or not isinstance(value, (int, float, Decimal)) + ): + return value + try: + column_spec = self.db_engine_spec.get_column_spec( + col.type, db_extra=self.db_extra + ) + except Exception: # pylint: disable=broad-except # noqa: BLE001 + return value + if column_spec is None: + return value + sqla_type = column_spec.sqla_type + type_class = sqla_type if isinstance(sqla_type, type) else type(sqla_type) + if issubclass(type_class, sa.Integer): + if isinstance(value, int): + return value + try: + return int(value) if value == int(value) else value + except (OverflowError, ValueError): + # Infinity and NaN have no integer, and match no integer row. + return value + return value + + def _mirror_probe_input( + self, + operator: utils.FilterOperator, + value: Any, + col: Optional["TableColumn"], + ) -> Any: + """ + ``value`` in the representation the mirror may probe the transform at. + + A simple filter on a temporal column carries a *string*. + `filter_values_handler` rewrites only an all-digit value, which it reads + as epoch milliseconds, so ``'2026-07-06 10:08:11'`` reaches here exactly + as the chart author typed it and is bound as text. The engine compares + that text against the column under its own cast, and on a ``DATE`` + column the cast drops the time of day: the filter keeps the whole of + 2026-07-06 while a mirror probed at 10:08:11 keeps one instant of it, + which is narrower than the predicate it stands in for. + + So the text is read back into the instant it denotes -- the only way + `_engine_literal_resolution`, which answers about a `datetime`, reaches + a filter value at all -- and where the engine compares less of that + instant than the filter carries, what to do about it is a property of + the *operator*: + + * a bound rounds outward, through the helper the range collector uses. + Reading one extra bucket at the edge is the trade this feature makes + everywhere else too -- see `mirror_operator` -- and the original + predicate is still in the query to exclude the rows that bucket adds. + * an equality, or one member of an ``IN`` list, has nowhere to widen to. + These entries are ``AND``-ed onto the query, so "or the day either + side" is not something this sink can express, and the request is + declined with `_UNMIRRORABLE`. The query is then correct and unpruned, + which is the one failure mode this feature is allowed. + + An instant carrying a UTC offset is declined for every operator, a + bound included. Rounding one outward would first have to place it in the + frame the partition column was written in, and guessing that frame is + how a mirror ends up a whole offset narrower than the filter -- see the + load-bearing assumption in + `superset.connectors.sqla.partition_mapping`. + + A *string* `_instant_from_filter_value` cannot read at all is declined on + the same terms, and for the same reason one more step back: there is no + instant to round. ``'07/06/2026 10:08:11'`` is not ISO 8601, so the + parser leaves it alone -- and Postgres casts that same text to a DATE and + keeps the whole of July 6 while the probe, handed the string verbatim, + evaluates the transform at 10:08:11 and asks for a partition key no row + holds. That is exactly the narrowing the ISO-only parser was written to + prevent, reached through a value it declines to read, so the decline has + to travel with it. Which costs a non-ISO *date* its pruning too, because + telling July 6 from June 7 is the guess that started this. + + Only a string. `_as_probe_input` has already run, so the engine SQL + `filter_values_handler` returns for an epoch-millisecond bound arrives + here as a `RawProbeValue` rather than text -- and that literal was + rendered by the engine at the engine's own resolution, so it is the one + thing on this path with nothing left to resolve. The escaping gate has + run too, on the value as it arrived; a string this method *returns* is a + fresh one, which is why `evaluate_transform` asks again. + + Where the engine compares the whole instant, a string is returned + exactly as it arrived -- down to the type the probe binds and the cache + key it lands on, because every query that was already correct has to + stay byte-for-byte correct. A `datetime` is still converted, which it + has always needed: on a column stored as an epoch integer the transform + is a function of an integer, and a `datetime` is not the value the real + predicate compares. + """ + value = self._in_column_number_type(value, col) + parsed = _instant_from_filter_value(value) + if parsed is None: + if ( + isinstance(value, str) + and self._column_literal_resolution(col) is not LiteralResolution.FULL + ): + return _UNMIRRORABLE + return value + + # Ahead of the full-resolution return: a transform such as + # `CAST(:value AS TIMESTAMP)` drops the offset, so even an engine that + # compares the whole instant would probe it in the wrong frame. Its own + # reason, because mapping a column the engine compares in full -- the + # advice the resolution decline carries -- does not help here. + if parsed.tzinfo is not None: + return _UNMIRRORABLE_OFFSET + + if self._engine_literal_resolution(parsed, col) is LiteralResolution.FULL: + return ( + self._mirror_probe_value(parsed, col) + if isinstance(value, datetime) + else value + ) + + if operator in LOWER_BOUND_OPERATORS: + instant = self._round_bound_outward(parsed, col, is_upper=False) + elif operator in UPPER_BOUND_OPERATORS: + instant = self._round_bound_outward(parsed, col, is_upper=True) + elif self._literal_is_coarser_than(parsed, col): + return _UNMIRRORABLE + else: + instant = parsed + + if instant == parsed and not isinstance(value, datetime): + return value + return self._mirror_probe_value(instant, col) + + def _collect_partition_mirror_range( + self, + mapping: Optional["PartitionMapping"], + sink: list[tuple[utils.FilterOperator, Any]], + column_name: str, start_dttm: Optional[sa.DateTime], end_dttm: Optional[sa.DateTime], - time_grain: Optional[str] = None, - label: Optional[str] = "__time", - template_processor: Optional[BaseTemplateProcessor] = None, - ) -> Optional[ColumnElement]: - col = ( - time_col.get_timestamp_expression( - time_grain=time_grain, - label=label, - template_processor=template_processor, + widen_bounds_by: Optional[relativedelta] = None, + ) -> None: + """ + Record a time range for mirroring onto the partition column. + + The bounds are adjusted first, by the same helper `get_time_filter` uses: + the mirrored predicate has to describe the same instants as the + timestamp predicate it stands in for, or the pruning is wrong by exactly + the dataset's timezone offset -- silently. + + Either bound may be `None` for an open-ended range, in which case only + the bound that exists is mirrored. + + Both bounds are recorded with the operators the real predicate uses, + `>=` and `<`. Relaxing the strict one is `mirror_operator`'s job, at the + single point where an operator becomes SQL -- which is also the point + the preview endpoint goes through. + + `widen_bounds_by` is one grain bucket width, passed when the filter this + stands in for compares a *truncated* column. The real predicate then + keeps rows the raw bounds exclude, and widening both ends by one bucket + is the smallest range guaranteed to contain all of them -- see the "Time + grains" section of `superset.connectors.sqla.partition_mapping`. + + A widened range can also be wide enough to fail `_bounds_are_ordered` + against a tighter filter on the same column. That fails open -- no + mirroring, no pruning, no dropped rows. + """ + if mapping is None or column_name != mapping.mapped_column: + return + if not mapping.mirrors(utils.FilterOperator.TEMPORAL_RANGE): + return + + # Resolve the column so the offset adjustment sees the same type + # `get_time_filter` does -- an offset the column's type cannot represent + # is dropped, and the mirrored bounds have to agree with the originals. + mapped_col = next( + (col for col in self.columns if col.column_name == column_name), None + ) + start_dttm, end_dttm = self.adjust_time_bounds(start_dttm, end_dttm, mapped_col) + + # Adjust first, then widen. `adjust_time_bounds` moves the naive UI + # bounds into the frame the column is *stored* in, and the grain + # truncation in the real predicate happens in that same frame, so the + # bucket width has to be added there too. The two commute for the + # hour-offset branch but not for the `ZoneInfo` one, where widening + # across a DST boundary first lands an hour out. + if widen_bounds_by is not None: + if start_dttm is not None: + start_dttm -= widen_bounds_by + if end_dttm is not None: + end_dttm += widen_bounds_by Review Comment: `end_dttm += widen_bounds_by` is outside any handler, and adding a bucket width to a datetime near the maximum raises `ValueError`. A mapped `TEMPORAL_RANGE` filter with grain `P1Y` and a range ending `9999-12-31` builds valid bounds for the original predicate, then fails here on year 10000. The comment above says a widened range fails open, but this raises instead, so turning mapping on breaks a chart that ran fine without it. Should the widening skip the mirror on overflow rather than propagate? ########## superset/connectors/sqla/partition_mapping.py: ########## @@ -0,0 +1,3583 @@ +# 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 (...)`` ``T`` is a function *and* the + engine compares ``col`` + byte-exactly +``col >=|>|<|<= v``, ``TEMPORAL_RANGE`` only if ``T`` is monotonic +``col != v``, ``NOT IN``, ``LIKE``, ... never +=========================================== ============================= + +Monotonic here means non-decreasing, not strictly increasing: a bucketing +transform such as a day key is monotonic and maps a whole day of timestamps +onto one value. ``col < v`` then only implies ``T(col) <= T(v)``, so a strict +bound is mirrored non-strictly -- see `mirror_operator`. + +Equality under the engine's own rules +------------------------------------- +"``T`` is a function" gives ``col = v`` implies ``T(col) = T(v)`` for *value* +equality. The engine compares with *SQL* equality, and the two part company +wherever that comparison is not byte-exact: under a case-insensitive collation +a stored ``country`` of ``'us'`` satisfies a filter for ``'US'``, while the +mirror ``region_key = hex('US')`` excludes the row and the chart loses it with +nothing to indicate why. Trailing-space-insensitive ``CHAR`` comparison says +the same thing about padding. + +So equality and ``IN`` are withdrawn for a *string* mapped column unless the +engine spec declares ``binary_string_comparison`` -- `equality_mirrors_safely`. +Numeric and temporal comparison is exact everywhere and is untouched, and range +mirroring is untouched too: it already rests on the owner declaring ``T`` +order-preserving *with respect to the column's own order*, which is an +assertion about the very semantics this is checking for. + +What the gate cannot see is a column declaring its own collation +(``country COLLATE NOCASE``) on an otherwise byte-exact engine. Nothing in +SQLAlchemy's reflection or the engine specs exposes that, so there it remains +the owner's assumption, like ``p = T(mapped_col)`` itself. + +Resolution of the value the engine compares +------------------------------------------- +Every row of the table above assumes the engine compares the value the mirror +was built from. It does not always. A ``DATE`` column compared against +``'2026-07-06 10:08:11'`` is compared on the date part alone -- whether the +engine writes the literal itself (``TO_DATE('2026-07-06', 'YYYY-MM-DD')`` on +Postgres, ``DATE '2026-07-06'`` on Presto, Trino, BigQuery and some two dozen +others) or applies an implicit cast to a bound string. The filter therefore +keeps the whole of that day while ``p = T(2026-07-06 10:08:11)`` keeps one +instant of it: narrower than the predicate it stands in for, which is the one +thing a mirror may never be. + +What an engine throws away is detected from the column's type rather than +enumerated per engine -- `ExploreMixin._engine_literal_resolution` -- and what +to do about it is a property of the *operator*, not of the value: + +* a bound rounds outward, backward for a lower one and forward for an upper one, + through `ExploreMixin._round_bounds_outward`. The mirror then reads one extra + bucket at the edge, and the original predicate is still in the query to + exclude the rows that bucket adds. +* an equality, or one member of an ``IN`` list, has nowhere to widen to: these + predicates are ``AND``-ed onto the query, so "or the day either side" is not + something the mirror can express. They decline, which costs the query its + pruning and nothing else. + +So a ``DATE``-typed mapped column mirrors a range at day granularity, and does +not mirror an equality whose value carries a time of day at all. Map a +``TIMESTAMP`` column instead and every operator is available again. + +Writing the value into the probe +-------------------------------- +The probe evaluates the transform at a *literal*, because it runs with no bound +parameters at all -- so every value is written into SQL text by the driver's +literal renderer and read back by the engine's lexer, and the two do not always +agree. SQLAlchemy's default renderer doubles apostrophes and leaves backslashes +alone; Hive honours backslash escapes and not doubling, so ``'O''Brien'`` is +read there as two adjacent literals and ``'x\\''`` ends four characters into a +value the filter carries whole. A disagreement is either a mirror built from a +value nobody filtered on -- dropping rows the filter keeps -- or, where the +value closes its own quote, an expression of the filterer's choosing running in +the probe. + +Which side is right is not knowable from the driver: it is a per-session server +setting (``standard_conforming_strings``, ``NO_BACKSLASH_ESCAPES``, +``spark.sql.parser.escapedStringLiterals``), and SQLAlchemy's own view of it is +only correct on a dialect that has connected. So escaping is not what makes +this safe. `literal_round_trips` renders the literal and reads it back in the +engine's own dialect instead, and a value that does not survive the trip is not +mirrored -- the usual trade, pruning for correctness. `probe_sql_is_evaluable` +re-reads the finished statement behind it. + +The probe's *answers* make the same trip, in the other direction. The chart +query is compiled with ``literal_binds``, so a probed partition key is rendered +into its SQL by the same renderer and read by the same lexer -- and +``concat(:value, chr(92), 'b')`` turns the unremarkable input ``a`` into the key +``a\\b``, which Hive reads back as ``a`` followed by a backspace. So +`_predicates_from_probe` asks `literal_round_trips` about each result too, and +declines the request rather than emitting a predicate no row can satisfy: the +mirror is ``AND``-ed onto the query, so a mirror that matches nothing costs the +chart its rows rather than its pruning. + +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 numbers +import re +from collections.abc import Sequence +from dataclasses import dataclass +from datetime import date, datetime +from functools import lru_cache +from typing import Any, cast, TYPE_CHECKING + +import numpy as np +import pandas as pd +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.constants import LRU_CACHE_MAX_SIZE, TimeGrain +from superset.exceptions import ( + QueryClauseValidationException, + SupersetParseError, + SupersetSecurityException, +) +from superset.extensions import cache_manager, feature_flag_manager +from superset.sql.parse import SQLScript, SQLStatement +from superset.superset_typing import FilterValues +from superset.utils import core as utils, json +from superset.utils.core import FilterOperator, get_username + +if TYPE_CHECKING: + from superset.connectors.sqla.models import SqlaTable, TableColumn + from superset.db_engine_specs.base import BaseEngineSpec + from superset.models.core import Database + +logger = logging.getLogger(__name__) + +FEATURE_FLAG = "PARTITION_FILTER_MAPPING" + +#: Longest transform accepted anywhere. A transform is one SQL expression an +#: owner types by hand, so this is generous; the point is that it is bounded. +#: The bound matters because `is_transform_active` parses the stored value on +#: each Explore load, and an unbounded string would make that parse the +#: expensive part of rendering a chart. +#: +#: The typed column field, the import schema and the preview request each +#: enforce it through a marshmallow `Length` validator, which is what produces +#: the per-field message those three callers want. `stored_expression_error` +#: enforces it too, for the paths that load no schema -- the deprecated +#: `POST /datasource/save/` is one, and it writes straight through +#: `update_from_object`. +MAX_TRANSFORM_LENGTH = 1024 + +#: 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") + +#: The opening and closing delimiters of each kind of Jinja block. The probe +#: would render these in a different context at a different time from the chart +#: query, so a transform holding one is rejected at save time. +JINJA_BLOCK_DELIMITERS = (("{{", "}}"), ("{%", "%}"), ("{#", "#}")) + +#: MySQL and MariaDB "executable comments": ``/*!50000 ... */`` and +#: ``/*M!50000 ... */`` are comments to every other reader and *statement text* +#: to those two engines. +#: +#: This is the one construct that breaks the premise the probe's shape gates +#: rest on -- that the parse this module inspects and the statement the engine +#: runs are the same statement. ``1 /*!50000 + (SELECT secret FROM vault) */`` +#: parses with no sub-query and no FROM, clears `stored_expression_error` and +#: `probe_sql_is_evaluable` alike, and then reads a table on MySQL. The result +#: comes back to a dataset owner through the preview's ``emitted_predicate``, +#: with no RLS applied and no SQL Lab access needed. +#: +#: Refused on every engine rather than only the two that honour it. A transform +#: is a scalar function of ``:value``; there is no legitimate reason for one to +#: carry a comment of any kind, let alone this one, and a per-engine gate would +#: store a transform on Postgres that detonates when the dataset is re-pointed +#: at MySQL. +EXECUTABLE_COMMENT_RE = re.compile(r"/\*M?!") + +#: A PostgreSQL Unicode-escaped identifier, ``U&"..."``. +#: +#: The second construct that breaks the premise above, and in the same way: +#: ``U&"\\0071uery_to_xml"(...)`` parses as an anonymous function whose name is +#: the *literal* ``\\0071uery_to_xml``, so the function denylist is matching a +#: name the engine never sees -- PostgreSQL decodes the escape and runs +#: ``query_to_xml``, handing a dataset owner rows from outside their RLS scope +#: through the preview's ``emitted_predicate``. +#: +#: Normalizing the escapes and re-checking would be the other repair, and is +#: worse: it means reimplementing one engine's identifier decoding well enough +#: to be a security boundary. Refusing the construct is exact, and costs +#: nothing real -- a transform is a scalar function of ``:value``, and there is +#: no reason to name a function or column through an escape sequence. +#: +#: Refused on every engine, like the executable comment, so a transform stored +#: on SQLite cannot detonate when the dataset is re-pointed at PostgreSQL. The +#: ``&`` may be separated from the ``U`` by nothing at all, but whitespace or a +#: comment between the prefix and the string is not accepted by PostgreSQL +#: either, so the strict form is the whole surface. +#: +#: Matched in raw text, so the prefix is refused inside a string literal too. +#: That is the same trade the executable comment makes and has the same +#: justification: a gate whose contract is to fire *before* the parse cannot +#: ask the parse whether the text was quoted. ``concat(:value, 'u&"x"')`` is +#: refused, which costs an owner nothing anyone writes. +UNICODE_ESCAPED_IDENTIFIER_RE = re.compile(r'\bU&"', re.IGNORECASE) + +#: 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" + +#: Substituted for ``:value`` to find out *where* the placeholder landed, which +#: `_PARSE_STANDIN` cannot answer: ``NULL`` is a keyword, so after substitution +#: the parse tree no longer records that a placeholder was ever there. An +#: identifier does leave a trace -- a column reference -- and one that is +#: missing is a placeholder the engine will never evaluate. +#: +#: Named so it cannot plausibly collide with a real column, but a transform that +#: writes it anyway is refused rather than counted. A hand-written occurrence +#: does not merely make the count disagree: it *supplies* the column reference a +#: non-executable placeholder failed to produce, and the two cancel exactly. See +#: `placeholder_is_executable`. +_PLACEHOLDER_STANDIN = "superset_pfm_value_standin" + +#: 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: a transform such as +#: ``datediff(day, :value, LOCALTIMESTAMP)`` for a relative partition key is +#: evaluated once and then keeps emitting yesterday's answer, so the chart +#: silently drops rows after midnight. +#: +#: Matched by SQL function name, so every engine's spelling of "now" has to be +#: named -- the ANSI four were not enough. Listed rather than delegated to the +#: engine specs because the risk is the same everywhere and a spec that had not +#: got round to declaring its own would fail open; a name no engine has is +#: inert, so over-listing costs nothing. +NON_DETERMINISTIC_FUNCTIONS = { + # ANSI, and the engines that spell it this way. + "CURRENT_DATE", + "CURRENT_TIME", + "CURRENT_TIMESTAMP", + "LOCALTIME", + "LOCALTIMESTAMP", + "NOW", + "RAND", + "RANDOM", + "UUID", + # PostgreSQL. `now()` is statement-stable; these two are not even that. + "CLOCK_TIMESTAMP", + "STATEMENT_TIMESTAMP", + "TIMEOFDAY", + # MySQL and MariaDB. + "CURDATE", + "CURTIME", + "SYSDATE", + "UTC_DATE", + "UTC_TIME", + "UTC_TIMESTAMP", + "UUID_SHORT", + # Oracle. + "SYSTIMESTAMP", + "SYS_GUID", + # SQL Server. + "GETDATE", + "GETUTCDATE", + "SYSDATETIME", + "SYSUTCDATETIME", + "SYSDATETIMEOFFSET", + "NEWID", + # BigQuery. + "CURRENT_DATETIME", + "GENERATE_UUID", + # Snowflake. + "RANDSTR", + "UUID_STRING", + # ClickHouse. + "NOW64", + "RANDCANONICAL", + "GENERATEUUIDV4", + # H2 and Derby, which the test fixtures use. + "RANDOM_UUID", +} + +#: The wall-clock names an engine evaluates with no parentheses at all. +#: +#: These parse as *column references*, not functions, so the name-based walk +#: below cannot see them: `datediff(UTC_DATE, :value)` on MySQL and +#: `:value - SYSDATE` on Oracle both cleared the gate and froze probe time into +#: a cached mirror. +#: +#: A subset of `NON_DETERMINISTIC_FUNCTIONS` rather than the whole of it, +#: because refusing a bare name costs an owner a column genuinely called that. +#: Only the names SQL lets you write without ``()`` are listed -- the datetime +#: keywords -- so `rand`, `uuid` and `newid`, which always need parentheses and +#: are plausible column names, stay out. +BARE_WALL_CLOCK_KEYWORDS = frozenset( + { + "current_date", + "current_time", + "current_timestamp", + "localtime", + "localtimestamp", + "utc_date", + "utc_time", + "utc_timestamp", + "sysdate", + "systimestamp", + } +) + +#: 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, +} + + +#: Every operator that can be mirrored under *some* transform. The preview +#: endpoint accepts these; whether a given one actually mirrors still depends on +#: the monotonicity declaration. +MIRRORABLE_OPERATORS = MIRRORABLE_ALWAYS | MIRRORABLE_IF_MONOTONIC + +#: Operators the preview endpoint can construct. `TEMPORAL_RANGE` is mirrored by +#: the query path, but as *two* bounds that `_collect_partition_mirror_range` +#: decomposes a time range into -- the preview request carries sample values, not +#: a since/until pair, so there is no range for it to build. The editor never +#: asks for one either: `previewOperatorFor` sends `>=`, `=` or `IN`. +PREVIEWABLE_OPERATORS = MIRRORABLE_OPERATORS - {FilterOperator.TEMPORAL_RANGE} + + +class Unmirrorable: + """ + A filter no mirror can stand in for, and why. + + Returned by `ExploreMixin.mirror_probe_request` for a value the engine + compares more coarsely than the filter carries, one it cannot render as a + literal, one whose rendered literal the engine would not read back as the + same value, one in a time frame the transform would discard, or an ``IN`` + list with a member of any of those kinds. An object rather than `None`, + because `None` is a filter value in its own right. + + The reason rides along because the preview has to explain the decline to + the owner, and the two declines want different advice: "the engine compares + less of this value than you typed, so map a column it compares in full" is + sound guidance for a `DATE` column and actively misleading for an + offset-bearing bound on a `TIMESTAMP` one, which already is such a column. + Carried rather than re-derived so the preview's explanation cannot come + apart from the chart path's decision. + + Lives here rather than beside that method so the preview can read it too: + `superset.models.helpers` imports this module, so these can only cross the + boundary in this direction. Both callers recognise the same instances -- a + preview that answered "valid" for a request the chart path declines is the + drift this is here to prevent. + """ + + __slots__ = ("reason",) + + def __init__(self, reason: str) -> None: + self.reason = reason + + def __repr__(self) -> str: + return f"<Unmirrorable {self.reason}>" + + +#: The ordinary decline: whatever the mirror cannot be built from, with the +#: engine's own comparison resolution as the reason an owner can act on. +UNMIRRORABLE = Unmirrorable("resolution") + +#: A value carrying a UTC offset, declined whatever the column's resolution. +#: A transform such as ``CAST(:value AS TIMESTAMP)`` drops the offset, so even +#: an engine that compares the whole instant would be probed in the wrong +#: frame -- which is not something mapping a different column fixes. +UNMIRRORABLE_OFFSET = Unmirrorable("offset") + +#: A value the driver and the engine would not spell the same way, declined +#: whatever the column's resolution and whatever the operator. Nothing about +#: the column or the transform fixes it -- see `literal_round_trips` -- so the +#: advice the resolution reason carries would send an owner after the wrong +#: thing entirely. +UNMIRRORABLE_ESCAPING = Unmirrorable("escaping") + + +def is_unmirrorable(value: Any) -> bool: + """Whether `mirror_probe_request` declined this request.""" + return isinstance(value, Unmirrorable) + + +#: A transform that preserves ordering need not be *strictly* increasing: a day +#: key such as ``to_char(ts, 'YYYYMMDD')`` maps a whole day of timestamps onto +#: one value. For such a ``T``, ``col < v`` only implies ``T(col) <= T(v)`` and +#: ``col > v`` only implies ``T(col) >= T(v)`` -- the rows in the bucket at the +#: boundary satisfy the non-strict form and not the strict one. A strict mirror +#: therefore drops rows the filter keeps, which is the one thing a mirror may +#: never do, so a strict bound is mirrored non-strictly. +_NON_STRICT_EQUIVALENT = { + FilterOperator.GREATER_THAN: FilterOperator.GREATER_THAN_OR_EQUALS, + FilterOperator.LESS_THAN: FilterOperator.LESS_THAN_OR_EQUALS, +} + + +def mirror_operator(operator: FilterOperator) -> FilterOperator: + """ + The operator a mirrored predicate uses to stand in for ``operator``. + + Strict bounds widen; everything else passes through. Idempotent, so it is + safe wherever a request may already have been widened. + + The widened bound reads one extra partition at the edge, and the original + predicate is still in the query to exclude the rows that partition adds -- + the same trade as the grain widening and the ``NULL``-partition escape. + Declaring the transform strictly increasing would buy that partition back, + which is not worth a second property only the owner can vouch for. + """ + return _NON_STRICT_EQUIVALENT.get(operator, operator) + + +def mirrorable_operators( + is_monotonic: bool, *, equality_is_safe: bool = True +) -> set[FilterOperator]: + """ + The operators whose predicates may be mirrored onto the partition column. + + :param is_monotonic: whether the owner declared the transform + order-preserving + :param equality_is_safe: whether the engine's ``=`` on the mapped column + compares the way ``T`` was written against -- see + `equality_mirrors_safely` + + Both call sites -- the query path through `PartitionMapping.mirrors` and the + Explore indicator through `partition_filter_mapping_summary` -- come through + here, which is what keeps the glyph from promising pruning the SQL will not + do. + """ + operators = MIRRORABLE_ALWAYS if equality_is_safe else set() + if is_monotonic: + operators = operators | MIRRORABLE_IF_MONOTONIC + return set(operators) + + +def _native_generic_type( + column: "TableColumn", +) -> utils.GenericDataType | None: + """ + What the engine spec makes of ``column``'s declared type. + + `TableColumn.type_generic` answers ``TEMPORAL`` for every column carrying + ``is_dttm``, which is the right answer for a chart's time axis and the + wrong one for a comparison: an epoch ``BIGINT`` is a number to the engine + however it is labelled here. Falls back to the flag where the spec has no + answer, so a type Superset cannot resolve is no worse off than before. + """ + try: + column_spec = column.db_engine_spec.get_column_spec( + column.type, db_extra=column.db_extra + ) + except Exception: # pylint: disable=broad-except # noqa: BLE001 + column_spec = None + if column_spec is not None and column_spec.generic_type is not None: + return column_spec.generic_type + try: + return column.type_generic + except Exception: # pylint: disable=broad-except # noqa: BLE001 + return None + + +#: A declared type naming an array: PostgreSQL's ``TEXT[]`` / ``INTEGER[][]`` +#: / ``int[3]``, and the ``ARRAY<...>`` / ``ARRAY(...)`` / ``Array(...)`` +#: spellings of Hive, Spark, Trino, BigQuery and ClickHouse. +#: The bracket body is written so no two runs can match the same blanks: +#: ``\s*\d*\s*`` backtracks polynomially on a long run of spaces. +_ARRAY_TYPE_RE = re.compile(r"(?:\[\s*(?:\d+\s*)?\])+\s*$|^\s*ARRAY\b", re.IGNORECASE) + + +def declares_array_type(column_type: str | None) -> bool: + """ + Whether a declared column type is an array, whatever the engine spec says. + + The engine spec only reports ``MULTI_VALUE`` where it has opted into + ``supports_multivalue_columns``; PostgreSQL has not, and resolves ``TEXT[]`` + to ``STRING`` and ``INTEGER[]`` to ``NUMERIC``. So an array partition column + there cleared the array refusal, previewed valid, and every equality chart + on the mapping failed with ``malformed array literal``. The declared type + is what the engine will compare, so it is read directly as well. + """ + return bool(column_type and _ARRAY_TYPE_RE.search(column_type)) + + +#: Declared types whose comparison ignores trailing blanks. +#: +#: The fixed-width character types, which the SQL standard has compare as +#: though both sides were padded to the same length -- so ``'US'`` and +#: ``'US '`` are equal in a ``CHARACTER(3)``, while ``lower()`` of the two is +#: not. Spelled out rather than matched by prefix because the variable-width +#: names start with the same letters: ``VARCHAR`` and ``CHARACTER VARYING`` +#: compare the blanks and must not land here. +PADDED_CHARACTER_TYPES = frozenset( + { + "CHAR", + "NCHAR", + # PostgreSQL's own name for ``CHARACTER``, which is what reflection + # reports for a ``CHAR(n)`` column there. + "BPCHAR", + "CHARACTER", + "NATIONAL CHAR", + "NATIONAL CHARACTER", + } +) + + +def _is_padded_character_type(column: TableColumn) -> bool: + """ + Whether ``column``'s declared type compares without its trailing blanks. + + Read off the declared type rather than the engine spec's resolved one: the + specs map ``CHAR`` and ``VARCHAR`` onto the same `sqlalchemy.String`, so + the distinction this needs is gone by then. The length suffix is dropped + and the name matched against `PADDED_CHARACTER_TYPES`, so an unfamiliar + spelling answers ``False`` and the engine-level flag decides as before -- + this gate adds a decline rather than lifting one. A column with no + declared type has nothing to read, and answers the same way. + """ + if not isinstance(column.type, str): + return False + native = re.sub(r"\s+", " ", column.type.strip().upper()) + return native.split("(")[0].strip() in PADDED_CHARACTER_TYPES + + +def equality_mirrors_safely( + column: TableColumn | None, + db_engine_spec: type[BaseEngineSpec], +) -> bool: + """ + Whether ``col = v`` on this column implies ``T(col) = T(v)`` in the engine. + + The operator matrix calls equality safe "for any function ``T``" because + ``T`` is a function, so ``col = v`` gives ``T(col) = T(v)``. That reasons + about *value* equality while the engine reasons about *SQL* equality, and + the two part company under any comparison that is not byte-exact: with a + case-insensitive collation a stored ``country`` of ``'us'`` satisfies a + filter for ``'US'``, but the mirror ``region_key = hex('US')`` excludes the + row and the chart loses it with no indication why. Trailing-space-insensitive + ``CHAR`` comparison says the same thing about padding. + + Only string columns are at risk; numeric and temporal comparison is exact + everywhere. And only equality: range mirroring already requires the owner + to declare ``T`` order-preserving *with respect to the column's own order*, + which is an assertion about the same comparison semantics this is checking + for -- so the declaration covers it where equality has nothing to cover it. + + Padding is asked about per column, because it is a property of the *type* + rather than of the engine. ``binary_string_comparison`` speaks for an + engine's default text comparison, and on PostgreSQL -- which declares it + -- a ``CHAR(2)`` still matches a filter for ``'US '`` against a stored + ``'US'``, because the standard says a padded character type compares + without its trailing blanks. The mirror ``region_key = lower('US ')`` then + excludes the row. So a fixed-width character type declines whatever the + engine says, and an engine that ignores trailing blanks on its *variable* + types says so by not declaring the flag at all (see `RedshiftEngineSpec`). + + The type asked for is the engine spec's reading of the column's declared + type, not `TableColumn.type_generic`, which answers ``TEMPORAL`` for + anything carrying ``is_dttm``. A ``VARCHAR`` flagged temporal is still text + to the engine: on MySQL a filter for ``'2026-01-01T00:00:00'`` matches a + stored ``'...t00:00:00'``, while the mirror ``p = hex('...T...')`` excludes + it -- and asking the semantic flag skipped the check entirely. + + A type the spec cannot classify declines, which is what the paragraph above + promises and what only the exception branch used to do. PostgreSQL's + ``citext`` matches no type pattern, so it answered ``None`` -- neither + string nor not-string -- and was waved through, which is precisely wrong + for a type whose whole purpose is case-insensitive comparison. Declining + costs the pruning; admitting it costs rows. + + What this cannot see is a column that declares its own collation + (``country COLLATE NOCASE``) on an otherwise byte-exact engine. Neither + SQLAlchemy's reflection nor the engine specs expose it, so on such a column + the assumption stays where ``p = T(mapped_col)`` already is: with the owner. + """ + if column is None: + return True + # Ahead of the generic-type question, which a padded type can lose: the + # specs map ``CHARACTER`` onto no generic type at all on some engines, and + # a fixed-width character type is a string whether or not Superset can say + # so from the name. + if _is_padded_character_type(column): + return False + generic_type = _native_generic_type(column) + if generic_type is None: + return False + if generic_type != utils.GenericDataType.STRING: + return True + return bool(db_engine_spec.binary_string_comparison) + + +#: 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 + #: Whether the engine compares the mapped column the way ``T`` was written + #: against. Resolved once where the column and the engine are both in hand, + #: rather than re-derived at each `mirrors` call. + equality_is_safe: bool = True + + def mirrors(self, operator: FilterOperator) -> bool: + return operator in mirrorable_operators( + self.is_monotonic, equality_is_safe=self.equality_is_safe + ) + + +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_executable_comment(transform: str | None) -> bool: + """Whether the transform carries a MySQL/MariaDB executable comment. + + See `EXECUTABLE_COMMENT_RE`. Answered on raw text, like + `contains_jinja` and for the same reason: the whole point is that no + parser this module can reach reports the construct at all. + """ + return bool(transform) and EXECUTABLE_COMMENT_RE.search(transform or "") is not None + + +def contains_unicode_escaped_identifier(transform: str | None) -> bool: + """Whether the transform names something through a ``U&"..."`` escape. + + See `UNICODE_ESCAPED_IDENTIFIER_RE`. Answered on raw text for the same + reason as the comment above: the parse reports a name the engine does not + use, which is exactly what makes the denylist blind to it. + """ + return ( + bool(transform) + and UNICODE_ESCAPED_IDENTIFIER_RE.search(transform or "") is not None + ) + + +#: Sequence pseudo-columns, which advance a sequence when they are *read*. +#: +#: ``orders_seq.NEXTVAL`` parses as a qualified column reference, not a +#: function call, so neither `SQLStatement.is_mutating` -- which walks function +#: names -- nor the function denylist can see it. The probe would then run +#: ``SELECT orders_seq.NEXTVAL + 1 FROM DUAL`` and advance the sequence, again +#: on every cache expiry, for a dataset editor holding neither SQL Lab nor a +#: DML grant. Reading one is a write however the parser classifies it. +#: +#: The function spelling (``nextval('s')`` on PostgreSQL) is a different shape +#: and is caught as a function; this is only about the suffix form. +SEQUENCE_PSEUDO_COLUMNS = frozenset({"nextval", "currval"}) + +#: The same thing written as a call. +#: +#: Only the suffix form parses as a column; `nextval('s')` on PostgreSQL and +#: DuckDB, `NEXTVAL(s)`/`SETVAL(s, 1)` on MySQL and MariaDB, and SQL Server's +#: `NEXT VALUE FOR s` all parse as functions. `SQLStatement.is_mutating` walks +#: function names on PostgreSQL alone -- deliberately, since the same names are +#: read-only elsewhere -- so every other engine let these through. +#: +#: Matched on the name whatever the engine, because advancing a sequence is a +#: write wherever it happens and a transform has no reason to call one. +SEQUENCE_FUNCTIONS = frozenset( + {"nextval", "setval", "next_value_for", "nextvalfor", "prevval", "lastval"} +) + + +def references_sequence(statement: SQLStatement) -> bool: + """Whether the statement reads or advances a sequence. + + Both spellings: the pseudo-column suffix, which parses as a column (see + `SEQUENCE_PSEUDO_COLUMNS`), and the call, which parses as a function (see + `SEQUENCE_FUNCTIONS`). + + Answered on the parse rather than on raw text, because position is what + makes it the construct: a column or alias merely *called* ``nextval`` is + an ordinary column, and a string literal holding the word is text. + """ + return bool( + statement.get_column_names() & SEQUENCE_PSEUDO_COLUMNS + or statement.get_disallowed_functions(set(SEQUENCE_FUNCTIONS)) + ) + + +def contains_jinja(transform: str | None) -> bool: + """ + Whether the transform contains a balanced Jinja block. + + A block is an opening delimiter followed, anywhere later, by its closing + one -- the same thing ``{{.*?}}`` and its two siblings match under + ``re.DOTALL``. Scanned with `str.find` rather than that pattern, because a + lazy match retried from every opening delimiter is quadratic in a run of + them, and this runs on stored transforms with no length bound of its own: + `is_transform_active` reads whatever the column holds. The earliest opening + delimiter is the only one worth trying, since any closing delimiter after a + later one is also after it. + """ + if not transform: + return False + for opening, closing in JINJA_BLOCK_DELIMITERS: + start = transform.find(opening) + if start != -1 and transform.find(closing, start + len(opening)) != -1: + return True + return False + + +def parse_skeleton(transform: str, standin: str = _PARSE_STANDIN) -> 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. + + :param standin: what to substitute. `placeholder_is_executable` passes + `_PLACEHOLDER_STANDIN` to find out where the placeholder landed. + """ + # Substituted through a function so `standin` is taken literally. As a + # replacement *string* `re.sub` would read backslashes in it as escapes, + # and `build_probe_sql` passes rendered SQL here: a filter value containing + # `\n` would probe a newline instead of the two characters the predicate + # compares, and one containing `\1` would raise outright. + return VALUE_PLACEHOLDER_RE.sub(lambda _: standin, transform) + + +#: Prefix the transform is wrapped in before parsing. Its length is subtracted +#: from any reported column so positions refer to what the owner actually typed. +_SELECT_PREFIX = "SELECT " + + +def _parse_skeleton( + transform: str, engine: str, standin: str = _PARSE_STANDIN +) -> SQLStatement | None: + """ + Parse ``SELECT <transform>`` with the placeholder substituted out. + + Returns ``None`` when the transform does not parse. + """ + try: + return SQLStatement( + f"{_SELECT_PREFIX}{parse_skeleton(transform, standin)}", engine + ) + except SupersetParseError: + return None + + +def parse_error_detail(transform: str, engine: str) -> str | None: + """ + Where the parser gave up on the transform. + + Returns ``None`` when it parses, or when the parser offered no position. + Note that sqlglot parses unknown functions happily -- a misspelled function + name is not a parse error, it is an engine error, and surfaces only when the + transform is evaluated. + + The parser's own ``highlight`` is deliberately dropped: it would name the + ``NULL`` we substituted for ``:value``, which is not a token the owner typed. + """ + try: + SQLStatement(f"{_SELECT_PREFIX}{parse_skeleton(transform)}", engine) + except SupersetParseError as ex: + column = (ex.error.extra or {}).get("column") + if not isinstance(column, int): + return None + return str( + _( + "syntax error at position %(position)d.", + position=_position_in_transform(transform, column), + ) + ) + return None + + +def _position_in_transform(transform: str, parsed_column: int) -> int: + """ + Map a column in the parsed skeleton back to the transform as typed. + + Two substitutions stand between them: the ``SELECT`` prefix, and every + ``:value`` that became a shorter ``NULL``. Without unwinding both, a + reported position drifts left by two characters per placeholder ahead of it + -- which is worst exactly where transforms usually break, at the end. + """ + position = max(parsed_column - len(_SELECT_PREFIX), 0) + shift = len(":value") - len(_PARSE_STANDIN) + preceding = sum( + 1 + for index, match in enumerate(VALUE_PLACEHOLDER_RE.finditer(transform)) + if match.start() - index * shift < position + ) + return position + preceding * shift + + +def placeholder_is_executable(transform: str | None, engine: str) -> bool: + """ + Whether every ``:value`` sits where the engine will evaluate it. + + `contains_value_placeholder` is a regex over raw text, and `parse_skeleton` + substitutes before parsing, so neither notices a placeholder that is not in + an executable position. Two shapes reach the probe that way, and both are + worse than the inactive mapping a malformed transform earns: + + ``':value'`` -- the placeholder inside a string literal. `build_probe_sql` + binds it anyway, because SQLAlchemy's own ``text()`` scan is no more + literal-aware than the regex, and the dialect then renders the bind + *including its own quotes* in the middle of the owner's quotes. A value of + ``" || (SELECT secret FROM vault LIMIT 1) || "`` closes the literal and + compiles to a working sub-query -- the very read primitive + `stored_expression_error` refuses when it is written openly. + + ``1 -- :value`` -- the placeholder inside a comment. The probe's selections + are joined on one line, so the comment swallows its own ``AS v0`` alias and + everything after it; a single-value probe still returns one column, the + constant is accepted as the transform's result, and the mirror then emits + ``partition_col = <constant>`` for every filter value. That is wrong rows, + not lost pruning. + + Answered structurally: substitute an identifier rather than a keyword and + count the column references it produced. A literal holds no column + reference and a comment is not parsed at all, so either case comes back + short of the number of placeholders the owner wrote. + + Which is why a transform naming the stand-in itself is refused outright. The + count is the whole gate, and a hand-written occurrence pays the difference: + ``concat(superset_pfm_value_standin, ':value')`` has one placeholder, inside + a literal that contributes nothing, and one bare reference the owner wrote -- + so the counts match and the gate passes a placeholder the engine will never + evaluate. The probe then renders the bound value inside the owner's quotes, + where a value closing the literal can alias a column of any table the + connection can read into the stand-in's name and have the probe hand it back + through the preview. There is no legitimate transform naming this identifier, + so refusing it costs nothing and makes the count mean what it claims. + """ + if not transform: + return False + if _PLACEHOLDER_STANDIN in transform.lower(): + # Lowercased on both sides because `count_bare_column_references` is + # case-insensitive too: a dialect may normalize an unquoted identifier, + # so `SUPERSET_PFM_VALUE_STANDIN` would otherwise balance the count + # while slipping past a case-sensitive check here. + return False + expected = len(VALUE_PLACEHOLDER_RE.findall(transform)) + if not expected: + return False + statement = _parse_skeleton(transform, engine, _PLACEHOLDER_STANDIN) + if statement is None: + return False + return statement.count_bare_column_references(_PLACEHOLDER_STANDIN) == expected Review Comment: The gate counts a `:value` that sits inside a quoted identifier as a bare column reference, because the stand-in becomes `"superset_pfm_value_standin"` and parses as one `exp.Column`. At probe time the value is bound inside those identifier quotes, so a sample value containing `"` can close the identifier and add its own expression. On DuckDB, a transform like `list_extract(list_transform([0], lambda "'x", "y'": ":value"), 1)` with a sample value of `x" + nextval(chr(115)) + "y` compiles to a single-projection probe with no comment or subquery, so the final shape check accepts it and `nextval` advances a sequence even though writing `nextval` directly is refused. A dataset editor without SQL Lab access can reach this through the preview endpoint. Should a placeholder inside a delimited identifier be rejected here, the same way one inside a string literal or comment is? ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorPartitionMapping.test.tsx: ########## @@ -0,0 +1,649 @@ +/** + * 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. + */ +import fetchMock from 'fetch-mock'; +import { + screen, + waitFor, + userEvent, + selectOption, +} from 'spec/helpers/testing-library'; +import { isFeatureEnabled, FeatureFlag } from '@superset-ui/core'; +import type { DatasetObject } from 'src/features/datasets/types'; +import { + createProps, + DATASOURCE_ENDPOINT, + setupDatasourceEditorMocks, + cleanupAsyncOperations, + fastRender, + dismissDatasourceWarning, + type DatasourceEditorProps, +} from './DatasourceEditor.test.utils'; + +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + isFeatureEnabled: jest.fn(), +})); + +const PREVIEW_URL = 'glob:*/api/v1/dataset/*/partition_mapping/preview/'; + +type EditorColumn = DatasetObject['columns'][number] & { + partition_value_transform?: string | null; + partition_transform_is_monotonic?: boolean; +}; + +/** The saved columns from the editor's most recent `onChange`. */ +const lastSavedColumns = (props: DatasourceEditorProps): EditorColumn[] => { + const { calls } = props.onChange.mock; + return calls[calls.length - 1][0].columns as EditorColumn[]; +}; + +const columnNamed = (columns: EditorColumn[], name: string) => + columns.find(column => column.column_name === name); + +/** + * The validation errors from the editor's most recent `onChange`. + * + * What DatasourceModal disables Save on, so this is the save gate as the modal + * sees it. + */ +const lastValidationErrors = (props: DatasourceEditorProps): string[] => { + const { calls } = props.onChange.mock; + return (calls[calls.length - 1][1] ?? []) as string[]; +}; + +beforeEach(() => { + fetchMock.get(DATASOURCE_ENDPOINT, [], { name: DATASOURCE_ENDPOINT }); + setupDatasourceEditorMocks(); + // The transform field previews on a debounce as soon as it is mounted with a + // previewable transform; without this the request escapes the test. + fetchMock.post(PREVIEW_URL, { result: { valid: true } }); + // Only this flag: a blanket `true` also turns on advanced data types and + // dataset folders, which change the DOM these tests read. + jest + .mocked(isFeatureEnabled) + .mockImplementation(flag => flag === FeatureFlag.PartitionFilterMapping); +}); + +afterEach(async () => { + await cleanupAsyncOperations(); + fetchMock.clearHistory().removeRoutes(); + jest.mocked(isFeatureEnabled).mockReset(); +}); + +test('removing the mapping does not leave the default datetime column mirroring', async () => { + // `ds` is the default datetime column and holds a transform of its own while + // the mapping sits explicitly on `state`. A dataset reaches that state by + // re-pointing the default datetime column, or through the import/PUT API, + // and until now nothing cleaned it up: clearing the override dropped the + // mapping back onto `ds`, where the leftover transform switched mirroring + // straight back on under a different column. + const props = createProps(); + props.datasource.main_dttm_col = 'ds'; + props.datasource.partition_column = 'num'; + props.datasource.partition_mapped_column = 'state'; + const seeded = props.datasource.columns as EditorColumn[]; + columnNamed(seeded, 'ds')!.partition_value_transform = + 'unix_timestamp(:value)'; + columnNamed(seeded, 'ds')!.partition_transform_is_monotonic = true; + columnNamed(seeded, 'state')!.partition_value_transform = 'lower(:value)'; + + fastRender(props); + await dismissDatasourceWarning(); + await userEvent.click(await screen.findByTestId('collection-tab-Columns')); + + // The mapping is live on `state` before the click. + expect( + await screen.findByText(/will automatically apply an equivalent filter/), + ).toBeInTheDocument(); + + await userEvent.type( + await screen.findByPlaceholderText('Search columns by name'), + 'state', + ); + await userEvent.click((await screen.findAllByLabelText(/expand row/i))[0]); + await userEvent.click(await screen.findByText('Remove mapping')); + + // Nothing mirrors any more, and the panel says so rather than quietly + // re-pointing at `ds`. + expect( + await screen.findByText(/No value transform is set on ds/), + ).toBeInTheDocument(); + expect( + screen.queryByText(/will automatically apply an equivalent filter/), + ).not.toBeInTheDocument(); + + await waitFor(() => { + expect( + lastSavedColumns(props).every( + column => + !column.partition_value_transform && + !column.partition_transform_is_monotonic, + ), + ).toBe(true); + }); +}); + +test('moving the mapping onto a column does not arm a transform parked there', async () => { + // NEW-R11-01. `state` holds a transform while the mapping sits on `ds`, a + // state the editor cannot produce but the deprecated `POST /datasource/save/` + // and a PUT made with the feature flag off both can. Moving the mapping onto + // `state` used to pick that leftover up -- one click, no typing, and the + // mapping went live emitting `num = lower(...)` for every filter, with the + // pruning indicator still green and the row counts quietly short. + const props = createProps(); + props.datasource.main_dttm_col = 'ds'; + props.datasource.partition_column = 'num'; + props.datasource.partition_mapped_column = null; + const seeded = props.datasource.columns as EditorColumn[]; + columnNamed(seeded, 'ds')!.partition_value_transform = + 'unix_timestamp(:value)'; + columnNamed(seeded, 'ds')!.partition_transform_is_monotonic = true; + columnNamed(seeded, 'state')!.partition_value_transform = 'lower(:value)'; + columnNamed(seeded, 'state')!.partition_transform_is_monotonic = true; + + fastRender(props); + await dismissDatasourceWarning(); + await userEvent.click(await screen.findByTestId('collection-tab-Columns')); + + await userEvent.type( + await screen.findByPlaceholderText('Search columns by name'), + 'state', + ); + await userEvent.click((await screen.findAllByLabelText(/expand row/i))[0]); + await userEvent.click(await screen.findByTestId('move-mapping-here')); + + // `state` is a VARCHAR, so there is no engine default to pre-fill either: the + // mapping arrives inert, and the field's own required marker says why rather + // than a mirror switching itself on. + await waitFor(() => { + expect(columnNamed(lastSavedColumns(props), 'state')).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); + }); + // And the column it came from does not keep its own either -- one mapping, + // one transform. + expect(columnNamed(lastSavedColumns(props), 'ds')).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); + expect(lastValidationErrors(props)).toEqual( + expect.arrayContaining([ + expect.stringContaining('A value transform is required on state'), + ]), + ); +}); + +test('re-pointing the default datetime column leaves the value transform behind', async () => { + // With no override the mapped column *is* `main_dttm_col`, so the mapping + // moves either way. The transform does not travel with it: it was written + // about the old column, and asserting it on a new one mirrors an expression + // nobody checked there. It must not stay behind on the old column either -- + // invisible but saved, and ready to go live again. + const props = createProps(); + props.datasource.main_dttm_col = 'ds'; + props.datasource.partition_column = 'num'; + props.datasource.partition_mapped_column = null; + const seeded = props.datasource.columns as EditorColumn[]; + columnNamed(seeded, 'ds')!.partition_value_transform = + 'unix_timestamp(:value)'; + columnNamed(seeded, 'ds')!.partition_transform_is_monotonic = true; + seeded.push({ + id: 99, + type: 'DATETIME', + filterable: false, + is_dttm: true, + is_active: true, + expression: '', + groupby: false, + column_name: 'ingest_time', + } as EditorColumn); + + fastRender(props); + await dismissDatasourceWarning(); + await userEvent.click(await screen.findByTestId('collection-tab-Columns')); + await screen.findByTestId('default-datetime-column-select'); + + await selectOption('ingest_time', 'Default datetime column'); + + await waitFor(() => { + const columns = lastSavedColumns(props); + // The new default never held a mapping, so it is handed back untouched -- + // which is the same thing as holding no transform. + const arrived = columnNamed(columns, 'ingest_time'); + expect(arrived?.partition_value_transform ?? null).toBeNull(); + expect(arrived?.partition_transform_is_monotonic ?? false).toBe(false); + expect(columnNamed(columns, 'ds')).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); + }); + + // The mapping is designated but inert, and the editor says so rather than + // promising a mirror it is not performing. + expect( + await screen.findByText(/No value transform is set on ingest_time/), + ).toBeInTheDocument(); +}); + +test('the mapping will not follow the default datetime column onto a calculated column', async () => { + // A calculated column can be the default datetime column, but its row never + // renders the transform editor -- carrying a transform there would make a + // live mapping with no UI to see or undo it. The mapping goes inert instead, + // and nothing is left behind on the old column either. + const props = createProps(); + props.datasource.main_dttm_col = 'ds'; + props.datasource.partition_column = 'num'; + props.datasource.partition_mapped_column = null; + const seeded = props.datasource.columns as EditorColumn[]; + columnNamed(seeded, 'ds')!.partition_value_transform = + 'unix_timestamp(:value)'; + seeded.push({ + id: 98, + type: 'DATETIME', + filterable: false, + is_dttm: true, + is_active: true, + expression: 'DATE_TRUNC("day", ds)', + groupby: false, + column_name: 'ds_day', + } as EditorColumn); + + fastRender(props); + await dismissDatasourceWarning(); + await userEvent.click(await screen.findByTestId('collection-tab-Columns')); + await screen.findByTestId('default-datetime-column-select'); + + await selectOption('ds_day', 'Default datetime column'); + + await waitFor(() => { + expect( + lastSavedColumns(props).every( + column => !column.partition_value_transform, + ), + ).toBe(true); + }); + + expect( + await screen.findByText(/No value transform is set on ds_day/), + ).toBeInTheDocument(); +}); + +test('a mapping onto a bare non-temporal column blocks the save', async () => { + // `state` is a VARCHAR, so it has no engine default to fall back on and the + // transform field marks itself required. Until now that marker was decorative: + // the save went through, the PUT filed it as a non-blocking issue, and the + // mapping sat inactive with nothing saying so at the field. + const props = createProps(); + props.datasource.main_dttm_col = 'ds'; + props.datasource.partition_column = null; + props.datasource.partition_mapped_column = 'state'; + + fastRender(props); + await dismissDatasourceWarning(); + await userEvent.click(await screen.findByTestId('collection-tab-Columns')); + await screen.findByTestId('partition-column-select'); + + await selectOption('num', 'Partition column'); + + await waitFor(() => { + expect(lastValidationErrors(props)).toEqual([ + expect.stringContaining('A value transform is required on state'), + ]); + }); +}); + +test('a mapping onto the default datetime column does not block the save', async () => { + // The other side of the check: a temporal column with no transform is the + // "saved but inactive" case the PRD asks for, so picking a partition column + // must not strand the owner with a disabled Save button. + const props = createProps(); + props.datasource.main_dttm_col = 'ds'; + props.datasource.partition_column = null; + props.datasource.partition_mapped_column = null; + + fastRender(props); + await dismissDatasourceWarning(); + await userEvent.click(await screen.findByTestId('collection-tab-Columns')); + await screen.findByTestId('partition-column-select'); + + await selectOption('num', 'Partition column'); + + await waitFor(() => { + expect(props.onChange).toHaveBeenCalled(); + }); + expect(lastValidationErrors(props)).toEqual([]); +}); + +test('with the feature off, re-pointing the default datetime column keeps the transform', async () => { + // The transform controls, the validation and the partition picker are all + // flag-gated, but the "Default datetime column" select is not -- it predates + // this feature. So with the flag off an owner could re-point it, never see a + // mapping control, and silently lose stored configuration on save. The + // backend gate does not cover it: the cleared value travels as an explicit + // null inside the `columns` payload, which `update_columns` writes whatever + // the flag says. + jest.mocked(isFeatureEnabled).mockImplementation(() => false); + + const props = createProps(); + props.datasource.main_dttm_col = 'ds'; + props.datasource.partition_column = 'num'; + props.datasource.partition_mapped_column = null; + const seeded = props.datasource.columns as EditorColumn[]; + columnNamed(seeded, 'ds')!.partition_value_transform = + 'unix_timestamp(:value)'; + columnNamed(seeded, 'ds')!.partition_transform_is_monotonic = true; + seeded.push({ + id: 99, + type: 'DATETIME', + filterable: false, + is_dttm: true, + is_active: true, + expression: '', + groupby: false, + column_name: 'ingest_time', + } as EditorColumn); + + fastRender(props); + await dismissDatasourceWarning(); + await userEvent.click(await screen.findByTestId('collection-tab-Columns')); + await screen.findByTestId('default-datetime-column-select'); + + await selectOption('ingest_time', 'Default datetime column'); + + await waitFor(() => { + expect(props.onChange).toHaveBeenCalled(); + }); + expect(columnNamed(lastSavedColumns(props), 'ds')).toMatchObject({ + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }); +}); + +test('DatasourceEditor source pins syncMetadata to the live column state', () => { + // Source-pin, for the same reason the sibling pin in `DatasourceEditor.test.tsx` + // exists: this file's own note records that the sync button cannot be + // triggered from jest -- `fetchSyncedColumns` goes through `SupersetClient`, + // and the request never settles under the test harness, so neither toast + // fires and there is no observable outcome to assert on. + // + // What the pin locks is the merge base. `datasource.columns` is the + // mount-time snapshot and never moves: no `setDatasource` call writes + // `columns`, and the props-sync effect only re-seeds the two column states. + // `updateColumns` passes an unchanged column through verbatim, so merging a + // sync against that snapshot restores whatever the dataset held when the + // modal opened -- the transform and monotonicity flag the owner just + // cleared, a `filterable`/`groupby` flag they just toggled, any description + // edited this session. The merge semantics themselves are covered in + // `utils/partitionMapping.test.ts`. + // eslint-disable-next-line global-require + const { readFileSync } = require('fs'); + // eslint-disable-next-line global-require + const { join } = require('path'); + const src = readFileSync( + join(__dirname, '..', 'DatasourceEditor.tsx'), + 'utf8', + ); + + // The merge reads the live state, not the mount-time snapshot -- and reads + // it through the ref, so an invocation already in flight when the owner + // changes something sees the change too. A closure capture could not: the + // callback's `currentColumns` is fixed when the sync starts. + expect(src).toMatch(/const columnChanges = updateColumns\(\s*liveColumns,/); + expect(src).not.toMatch( + /const columnChanges = updateColumns\(\s*datasource\.columns,/, + ); + expect(src).toMatch( + /const liveColumns = currentColumnsRef\.current;\s*const liveDatasource = datasourceRef\.current;/, + ); + + // And the live state is both column collections, memoized on both, with the + // refs tracking that memo and the datasource. + expect(src).toMatch( + /const currentColumns = useMemo\(\s*[\s\S]{0,900}?\(\) => \[\.\.\.databaseColumns, \.\.\.calculatedColumns\],\s*\[databaseColumns, calculatedColumns\],/, + ); + expect(src).toMatch( + /currentColumnsRef\.current = currentColumns;\s*datasourceRef\.current = datasource;/, + ); +}); + +test('DatasourceEditor source pins the sync to the mapping it leaves behind', () => { + // Source-pin, for the reason the pin above gives: the sync button cannot be + // driven from jest, because `fetchSyncedColumns` goes through + // `SupersetClient` and the request never settles under the harness. The + // behaviour of the helper it pins is covered directly in + // `components/PartitionFilterMapping/utils.test.ts`. + // + // What this locks is that the invariant is asked on *both sides* of the + // move. A sync that drops the mapped column makes + // `clearDanglingPartitionMapping` null the override, which re-points the + // mapping at the default datetime column -- so asked only against the + // repaired mapping, that column reads as the mapped one and a transform some + // earlier writer stranded on it is kept, and goes live from a sync nobody + // typed into (NEW-R11-01, then NEW-R12-01 when the first fix answered for the + // repaired side alone). + // eslint-disable-next-line global-require + const { readFileSync } = require('fs'); + // eslint-disable-next-line global-require + const { join } = require('path'); + const src = readFileSync( + join(__dirname, '..', 'DatasourceEditor.tsx'), + 'utf8', + ); + + // The repair is resolved into the mapping the move lands on -- off the live + // datasource, so a mapping removed while the sync was in flight is the one + // the move starts from. + expect(src).toMatch( + /const syncedDatasource = \{ \.\.\.liveDatasource, \.\.\.clearedMapping \};/, + ); + // Both sides are passed, in that order, and before the columns are set. + expect(src).toMatch( + /clearUnmappedTransformsAcrossMove\(\s*liveDatasource,\s*syncedDatasource,\s*columnChanges\.finalColumns,?\s*\)[\s\S]{0,400}?setColumns\(\{/, Review Comment: These sync tests read `DatasourceEditor.tsx` as text and match regexes, so they pin the source's shape rather than its behavior. If `syncMetadata` cleared unmapped transforms unconditionally, a sync with `PARTITION_FILTER_MAPPING` off would silently drop a stored transform and its monotonicity flag, and these assertions would stay green. The existing flag-off test covers changing the default datetime column, not syncing. Could a render test cover this: flag off, a stored non-mapped transform, a mocked `fetchSyncedColumns` resolved, and an assertion that the resulting `onChange` payload keeps both fields? ########## superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx: ########## @@ -144,6 +144,13 @@ export default function DrillDetailPane({ return resultsPages.get(lastPageIndex.current); }, [pageIndex, resultsPages]); + // A lower-bound total only covers the rows up to the current page, so count + // one row past it to keep the next page reachable. + const recordCount = Review Comment: `recordCount` is derived only from the page currently shown. With 120 matching rows and 50-row pages, visiting page 3 caches an exact total of 120, but going back to page 1 uses that page's cached lower bound (50), so `recordCount` becomes 51. Page 3 disappears from the pager and the label reverts to "50+ rows" even though the exact total is already known. Should the highest known bound, or the exact total once seen, be kept separately from the displayed page? ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionColumnFields.tsx: ########## @@ -0,0 +1,367 @@ +/** + * 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. + */ +import { useMemo } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, useTheme } from '@apache-superset/core/theme'; +import { Alert } from '@apache-superset/core/components'; +import { + Button, + Flex, + Icons, + InfoTooltip, + Label, + Select, + Tooltip, + Typography, +} from '@superset-ui/core/components'; +import { + mappedColumnIsImplicit, + mappingIsActive, + partitionMappingErrors, + resolveMappedColumn, + suggestedMappedColumn, +} from './utils'; +import type { + PartitionMappingColumn, + PartitionMappingDatasource, + PartitionMappingIssue, +} from './types'; + +interface PartitionColumnFieldsProps { + datasource: PartitionMappingDatasource; + /** + * Physical columns: what the partition-column dropdown may offer, and where + * a replacement mapped column may be suggested from. A calculated column + * belongs in neither -- the engine cannot partition on an expression, and + * the mapping picker only renders on a physical column's row. + */ + columns: PartitionMappingColumn[]; + /** + * Every column on the dataset, physical and calculated. What *exists* is a + * different question from what may be picked, and validation asks the first: + * the backend accepts a calculated column as the mapped-column override, so + * checking existence against the physical columns alone reported a column + * that is really there as missing -- and blocked Save with no way out, since + * the dropdown cannot offer it back. + */ + allColumns: PartitionMappingColumn[]; + onPartitionColumnChange: (columnName: string | null) => void; + /** Open the given column's row expand in the Columns table. */ + onNavigateToColumn: (columnName: string) => void; + /** + * Whether the mapped column's own preview says the transform mirrors, or + * `null`/undefined when no verdict is in yet. The checks this section can + * make are all static -- a transform the database rejects clears every one + * of them -- so without the preview's answer the banner claimed a speed-up + * the query never delivers. + */ + previewMirrors?: boolean | null; +} + +/** + * "Partition column" and the computed "Maps to partition" (wireframes 1a, 1g). + * + * "Maps to partition" is deliberately read-only. It reflects + * `partition_mapped_column ?? main_dttm_col`, and a peer dropdown would let it + * drift from the default datetime column silently. The only way to override it + * is from the target column's own row, where a transform has to be supplied + * alongside. + */ +export default function PartitionColumnFields({ + datasource, + columns, + allColumns, + onPartitionColumnChange, + onNavigateToColumn, + previewMirrors, +}: PartitionColumnFieldsProps) { + const theme = useTheme(); + + const options = useMemo( + () => + columns.map(column => ({ + value: column.column_name, + label: column.column_name, + customLabel: ( + <Flex align="center" gap={theme.sizeUnit}> + <span>{column.column_name}</span> + {column.type && <Label>{column.type}</Label>} + </Flex> + ), + })), + [columns, theme.sizeUnit], + ); + + const mappedColumn = resolveMappedColumn(datasource); + const isImplicit = mappedColumnIsImplicit(datasource); + const mappedColumnRow = allColumns.find( + column => column.column_name === mappedColumn, + ); + const liveTransform = mappedColumnRow?.partition_value_transform ?? null; + // Range operators -- the time range among them -- only mirror under a + // transform declared order-preserving, so without it the banner must not + // promise the time range a partition filter it will not get. + const isMonotonic = Boolean( + mappedColumnRow?.partition_transform_is_monotonic, + ); + // The stored refusal, but only while the box still holds the expression it + // was about. It is a verdict on one transform, and it outlived it: reopen a + // dataset whose stored `no_such_fn(:value)` failed its last probe, replace it + // with something that works, watch the row's own preview come back valid -- + // and the banner still said nothing would mirror, because this read a summary + // computed when the editor opened. + const storedVerdictApplies = + liveTransform === datasource.partition_filter_mapping?.evaluated_transform; + // The static checks are necessary, not sufficient: an unparseable expression + // or a function the database does not have needs the engine to spot, and the + // mapped column's row already asks it. Deferring to that answer where there + // is one is what keeps this banner and that panel from contradicting each + // other. + const isActive = + mappingIsActive(datasource, allColumns) && + previewMirrors !== false && + !( + storedVerdictApplies && + datasource.partition_filter_mapping?.evaluable === false + ); + const { partition_column: partitionColumn } = datasource; + + // `field` is what the issues carry it for: a message about the partition + // column belongs under the partition column, not only in the Save button's + // tooltip, where an owner has to guess which of the two selects is at fault. + const issues = useMemo( + () => partitionMappingErrors(datasource, allColumns), + [datasource, allColumns], + ); + const issueFor = (field: PartitionMappingIssue['field']) => + issues.find(issue => issue.field === field); + // Non-blocking issues are saveable, so they read as warnings. The implicit + // self-mapping is the one that reaches here: the owner can save through it, + // and danger styling on a message beside an enabled Save button says the + // opposite of what the button is doing. + const issueType = (issue: PartitionMappingIssue) => + issue.blocking === false ? 'warning' : 'danger'; + const partitionColumnIssue = issueFor('partition_column'); + const mappedColumnIssue = issueFor('partition_mapped_column'); + + // "Map a different column instead" normally opens the currently-mapped + // column's row, which is where the picker lives. When the mapped column *is* + // the partition column -- the self-mapping the backend rejects, and reachable + // in one click because the picker offers every column -- that row renders no + // mapping section at all, so the one guided way out of the broken state led + // nowhere. Offer a column that is not the partition column instead. + // + // Null only when there is no mapped column at all, which is also when the + // block below does not render -- but a dataset whose every column is the + // partition column would otherwise be offered a link to nothing, so the link + // is guarded rather than the type asserted away. + const differentColumnTarget = + mappedColumn && mappedColumn !== partitionColumn + ? mappedColumn + : (suggestedMappedColumn(columns, partitionColumn) ?? mappedColumn); + + return ( + <Flex vertical gap={theme.sizeUnit} data-test="partition-column-fields"> + <Flex align="center" gap={theme.sizeUnit}> + <Typography.Text>{t('Partition column')}</Typography.Text> + <InfoTooltip + tooltip={t( + 'The physical column the engine partitions on. Filters on the mapped column are mirrored onto it so the engine can prune partitions.', + )} + /> + </Flex> + <Select + ariaLabel={t('Partition column')} + options={options} + value={partitionColumn ?? undefined} + onChange={value => onPartitionColumnChange((value as string) ?? null)} + onClear={() => onPartitionColumnChange(null)} + placeholder={t('None')} + allowClear + data-test="partition-column-select" + /> + {partitionColumnIssue && ( + <Typography.Text + type={issueType(partitionColumnIssue)} + data-test="partition-column-error" + > + {partitionColumnIssue.message} + </Typography.Text> + )} + <Typography.Text type="secondary"> + {t( + 'Column used for partition pruning on this table. Its Is filterable and Is dimension settings decide whether Explore offers it.', + )} + </Typography.Text> + + {partitionColumn && ( + <Flex vertical gap={theme.sizeUnit} data-test="maps-to-partition"> + <Flex align="center" gap={theme.sizeUnit}> + <Typography.Text type="secondary"> + {t('Maps to partition')} + </Typography.Text> + <InfoTooltip + tooltip={t( + 'The column whose filters are mirrored. It follows the default datetime column unless a different column holds the mapping.', + )} + /> + </Flex> + + {mappedColumn ? ( + <> + <Flex align="center" gap={theme.sizeUnit}> + <Label>{mappedColumn}</Label> + {isImplicit && ( + <> + <Typography.Text type="secondary"> + {t('Default datetime column')} + </Typography.Text> + <Tooltip + title={t( + 'Set from the default datetime column above, so re-pointing that column moves the mapping with it.', + )} + > + <Icons.LockOutlined + iconSize="s" + iconColor={theme.colorTextTertiary} + /> + </Tooltip> + </> + )} + </Flex> + {mappedColumnIssue && ( + <Typography.Text + type={issueType(mappedColumnIssue)} + data-test="partition-mapped-column-error" + > + {mappedColumnIssue.message} + </Typography.Text> + )} + <Typography.Text type="secondary"> + {t( + 'Filters on this column are mirrored onto the partition column.', + )} + {differentColumnTarget && ( + <> + {' '} + <Button + buttonStyle="link" + onClick={() => onNavigateToColumn(differentColumnTarget)} + data-test="map-a-different-column" + > + {t('Map a different column instead →')} + </Button> + </> + )} + </Typography.Text> + {isActive ? ( + <Alert + type="info" + showIcon + icon={<Icons.FilterOutlined />} + message={ + <span> + {isMonotonic + ? t( + 'Filters on %(mapped)s will automatically apply an equivalent filter to %(partition)s.', + { + mapped: mappedColumn, + partition: partitionColumn, + }, + ) + : t( + 'Equality and list filters on %(mapped)s will automatically apply an equivalent filter to %(partition)s. Time ranges and other range filters are not mirrored until "Transform preserves ordering" is on.', Review Comment: This message says equality and list filters on the mapped column will be mirrored, but nothing here checks that those operators are allowed. Take a MySQL dataset mapping a `VARCHAR` through `lower(:value)` with ordering unchecked: `mappingIsActive()` accepts it, while the server's `equality_mirrors_safely()` rejects equality and `IN`, so no operator is mirrorable. The owner is told the mapping works, yet every filter stays unpruned. Could the help text use the server's operator eligibility instead of assuming equality is always available? -- 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]
