sadpandajoe commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4275997668
##########
superset/commands/dataset/importers/v0.py:
##########
@@ -291,7 +306,91 @@ def run(self) -> None:
.one()
)
dataset["database_id"] = database.id
- SqlaTable.import_from_dict(dataset, sync=self.sync)
+ table = SqlaTable.import_from_dict(dataset, sync=self.sync)
+ if table is not None:
+ imported.append(table)
+
+ self._repair_partition_mappings(imported)
Review Comment:
A legacy overwrite can switch on a stored transform that nothing in the
bundle wrote: `clear_unmapped_partition_transforms` only runs here, after
`import_from_dict` has already applied the new references. The v1 importer runs
it on the stored dataset before the import as well, because an omitted field
keeps its stored value.
Concretely, with the flag on, a dataset maps `a -> p` and has a parked
`:value + 100` on numeric column `b`. A v0 overwrite that sets
`partition_mapped_column: b` and omits the columns then makes `b` the effective
column, so this cleanup keeps its transform. A chart filter `b = 5` gets `p =
105` and drops the row `(a=1, b=5, p=1)`.
Could v0 disarm the old mapping's unmapped transforms before the import, the
way v1 does?
##########
superset/commands/dataset/importers/v0.py:
##########
@@ -291,7 +306,91 @@ def run(self) -> None:
.one()
)
dataset["database_id"] = database.id
- SqlaTable.import_from_dict(dataset, sync=self.sync)
+ table = SqlaTable.import_from_dict(dataset, sync=self.sync)
+ if table is not None:
+ imported.append(table)
+
+ self._repair_partition_mappings(imported)
+
+ @classmethod
+ def _default_omitted_monotonic_declarations(cls, config: Any) -> None:
+ """
+ Materialize `partition_transform_is_monotonic` wherever a transform is
+ set, anywhere in a v0 config.
+
+ The v1 importer does this on its own raw config, and for the same
+ reason: an overwrite keeps whatever the bundle omits, including a
+ ``True`` left behind by the transform this bundle is replacing. A
+ non-monotonic transform declared monotonic enables range mirrors that
+ exclude rows the filter keeps -- ``abs(:value)`` inheriting a stored
+ ``True`` turns ``n >= -1`` into ``p >= 1`` and misses ``n = 0``.
+
+ Walked rather than addressed directly because a v0 bundle comes in two
+ shapes: a CLI export nests datasets under ``databases[].tables[]``,
+ while a UI export is a bare list of datasets. Only columns that supply
+ a transform are touched, so a re-import that says nothing about a
+ column stays idempotent.
+ """
+ if isinstance(config, list):
+ for entry in config:
+ cls._default_omitted_monotonic_declarations(entry)
+ return
+ if not isinstance(config, dict):
+ return
+ _default_omitted_monotonic_declarations(config.get("columns") or [])
+ for key in (DATABASES_KEY, "tables"):
+ for child in config.get(key) or []:
+ cls._default_omitted_monotonic_declarations(child)
+
+ @staticmethod
+ def _repair_partition_mappings(datasets: list[SqlaTable]) -> None:
+ """
+ Keep the partition filter mapping honest across a v0 import.
+
+ The mapping's columns ride in `SqlaTable.export_fields`, so this path
+ carries them -- but it calls `SqlaTable.import_from_dict` directly and
+ goes through neither `DatasetDAO.update` nor the v1 importer, so
+ neither of the mapping's repairs ran. An older bundle imported with
+ ``--sync columns`` that omits the partition column deletes it and keeps
+ `partition_column` naming it: pruning silently stops, and every later
+ edit of that dataset fails the partition-column-not-found validation,
+ including a description-only PUT, which carries no columns payload and
+ so cannot reach the branch that forgives a stored-only dangle.
+
+ The same pair, in the same order, as `RefreshDatasetCommand` and the
+ v1 importer. Run after the import rather than around it because
+ ``import_from_dict`` is what discovers which datasets are involved.
+ """
+ if not datasets:
+ return
+ # Deferred import: `daos.dataset` pulls in `views.base`, which
+ # circularly imports back into the commands package at app init.
+ from superset.daos.dataset import DatasetDAO # noqa: PLC0415
+
+ db.session.flush()
+ for dataset in datasets:
+ DatasetDAO.clear_dangling_partition_mapping(
+ dataset, {column.column_name for column in dataset.columns}
+ )
+ # The other reference a bundle can leave unsaveable, cleared the
way
+ # the v1 importer clears it: an *explicit* self-mapping is blocking
+ # in `UpdateDatasetCommand`, so a bundle setting both references to
+ # the same existing column imported happily and then failed every
+ # later edit, including a description-only PUT. Dropping only the
+ # override leaves the mapping following `main_dttm_col`, which is
+ # the state a bundle omitting the field would have produced.
+ if (
+ dataset.partition_mapped_column
+ and dataset.partition_mapped_column == dataset.partition_column
+ ):
+ logger.warning(
+ "Clearing the partition mapped column on dataset %s during
"
+ "import: it names the partition column itself, which no "
+ "later save would accept",
+ dataset.table_name,
+ )
+ dataset.partition_mapped_column = None
+ DatasetDAO.clear_unmapped_partition_transforms(dataset)
Review Comment:
This cleanup keeps any transform on the effective mapped column, but v0
never drops one that the save path would reject, which v1 does through
`drop_unusable_partition_transforms`. Importing a legacy bundle with
`partition_mapped_column: n` and `n.partition_value_transform: "random() +
:value"` succeeds and stores the expression. Every later save then fails
validation: a description-only `PUT /api/v1/dataset/<id>` returns 422 on the
stored transform, and the mapping never mirrors anything.
Could v0 run the same transform sanitization as v1 on both bundle shapes
(CLI `databases[].tables[]` and the UI list)?
##########
superset/models/helpers.py:
##########
@@ -4340,36 +4444,730 @@ 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.
+
+ 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
+
+ # Then round outward, to whatever the engine's literal resolves to.
+ start_dttm, end_dttm = self._round_bounds_outward(
+ start_dttm, end_dttm, mapped_col
+ )
+
+ # Converted last. The widening and rounding above are `datetime`
+ # arithmetic, and the stored representation may be an integer.
+ if start_dttm is not None:
+ sink.append(
+ (
+ utils.FilterOperator.GREATER_THAN_OR_EQUALS,
+ self._mirror_probe_value(start_dttm, mapped_col),
+ )
)
- if time_grain
- else self.convert_tbl_column_to_sqla_col(
- time_col, label=label, template_processor=template_processor
+ if end_dttm is not None:
+ sink.append(
+ (
+ utils.FilterOperator.LESS_THAN,
+ self._mirror_probe_value(end_dttm, mapped_col),
+ )
)
+
+ def _array_mirror_probe_value(
+ self,
+ value: Any,
+ operator: utils.FilterOperator,
+ element_type: Optional[utils.GenericDataType],
+ db_engine_spec: builtins.type["BaseEngineSpec"],
+ ) -> Any:
+ """
+ The array literal the real predicate compares, ready to probe with.
+
+ On an array column the predicate is not built from `eq` at all: the
+ branch below reads the raw `val`, parses the pasted literal into
+ elements, coerces them to the array's element type and asks the engine
+ spec for its own array syntax. Mirroring `eq` would probe the transform
+ at a value the query never compares, so this reproduces that chain
+ instead -- the mirror then probes exactly what the predicate holds.
+
+ The result is SQL text rather than a value, because an array literal is
+ an expression; `RawProbeValue` carries it and `build_probe_sql`
+ substitutes rather than binds. Returns `None` when the engine spec
+ claims multi-value columns without implementing a literal for them, or
+ when an element has no literal the dialect can render -- a nested array
+ such as ``[[1, 2]]`` -- which costs the query its pruning and nothing
+ else.
+ """
+
+ def literal(entry: Any) -> Any:
+ return raw_probe_value(
+ self.database,
+ db_engine_spec.array_literal(
+ coerce_array_values(parse_array_literal(entry),
element_type)
+ ),
+ )
+
+ try:
+ if operator in {utils.FilterOperator.IN,
utils.FilterOperator.NOT_IN}:
+ candidates = (
+ list(value) if isinstance(value, (list, tuple)) else
[value]
+ )
+ return tuple(literal(candidate) for candidate in candidates)
+ return literal(value)
+ except (NotImplementedError, sa.exc.CompileError):
+ return None
+
+ def _as_probe_input(self, value: Any) -> Any:
+ """
+ ``value`` in a form `build_probe_sql` can put into the probe.
+
+ `filter_values_handler` returns engine SQL rather than a value for an
+ epoch-millisecond bound on a temporal column -- what drill-to-detail
and
+ cross-filters send -- and SQL cannot be the value of a bind parameter.
+ Carrying it as text lets the probe substitute it instead, exactly as
the
+ preview path's own coercion does and as an array literal needs.
+
+ Text rather than the expression itself also matters for caching: the
+ probe cache keys on `repr` of each value, and a `ColumnClause`'s
default
+ `repr` carries its memory address, so every request would miss.
+
+ Returns `_UNRENDERABLE` when the engine cannot render the literal,
which
+ costs the query its pruning and nothing else -- the same thing the
probe
+ itself does on failure.
+ """
+
+ def rendered(entry: Any) -> Any:
+ if isinstance(entry, ColumnElement):
+ return raw_probe_value(self.database, entry)
+ return entry
+
+ try:
+ if isinstance(value, (list, tuple)):
+ return type(value)(rendered(item) for item in value)
+ return rendered(value)
+ except Exception: # pylint: disable=broad-except # noqa: BLE001
+ logger.warning(
+ "Could not render a filter value for the partition mirror; "
+ "queries will not prune",
+ exc_info=True,
+ )
+ return _UNRENDERABLE
+
+ def _collect_partition_mirror_filter(
+ self,
+ mapping: Optional["PartitionMapping"],
+ sink: list[tuple[utils.FilterOperator, Any]],
+ column_name: str,
+ operator: utils.FilterOperator,
+ value: Any,
+ ) -> None:
+ """
+ Record a structured filter for mirroring onto the partition column.
+
+ Only operators the mapping declares safe are recorded; see the operator
+ matrix in `superset.connectors.sqla.partition_mapping`.
+
+ The value itself is resolved by `mirror_probe_request`, which the
preview
+ endpoint calls too so that the two cannot answer differently.
+ """
+ if mapping is None or column_name != mapping.mapped_column:
+ return
+ if not mapping.mirrors(operator):
+ return
+
+ mapped_col = next(
+ (col for col in self.columns if col.column_name == column_name),
None
)
+ probe_value = self.mirror_probe_request(operator, value, mapped_col)
+ if is_unmirrorable(probe_value):
+ return
+ sink.append((operator, probe_value))
+
+ def mirror_probe_request(
+ self,
+ operator: utils.FilterOperator,
+ value: Any,
+ col: Optional["TableColumn"],
+ ) -> Any:
+ """
+ ``value`` as the mirror may probe the transform at, or an
`Unmirrorable`.
+
+ The whole of what has to happen to a filter value before it can be
+ probed, in one place because two callers need it: the chart query path,
+ and the dataset editor's preview endpoint. The preview used to coerce
the
+ value and then probe it directly, so it reported a mirror for a request
+ the chart path declines -- an owner was shown a valid predicate on a
+ `DATE` mapped column whose equality filters never mirror at all.
+
+ Routing per *value* rather than per Python type is what makes a bound
+ widen and an equality decline whichever shape the value arrived in -- a
+ `datetime` from drill-to-detail, or the raw string a simple filter
binds.
+ """
+ # First, and ahead of anything that reads the value as an instant: a
+ # value `filter_values_handler` already turned into engine SQL is
+ # carried as text from here on, and the probe has to receive exactly
+ # that literal -- it is what the real predicate compares, so there is
+ # nothing left to resolve about it. Idempotent, so a caller that has
+ # already rendered its own values may still come through here.
+ value = self._as_probe_input(value)
+ if value is _UNRENDERABLE:
+ return UNMIRRORABLE
+
+ if operator == utils.FilterOperator.IN:
+ if not isinstance(value, (list, tuple)) or not value:
+ return UNMIRRORABLE
+ if any(item is None for item in value):
Review Comment:
This guard declines a whole `IN` list when one member is `None`, but nothing
in the mirroring tests drives it: the NULL tests cover a NULL partition value,
not a NULL member of the filter's own list. If it regressed to dropping the
`None` and mirroring the rest, `country IN ['US', None]` with
`coalesce(lower(:value), 'unknown')` would emit `region_key IN ('us') OR
region_key IS NULL` and exclude a row with `country IS NULL, region_key =
'unknown'` that the original keeps.
Could `get_sqla_query` get a test with that filter on the existing
mapped-table fixture, asserting the probe is not called and no `region_key`
predicate is emitted?
--
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]
