eschutho commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4235123713
##########
superset/models/helpers.py:
##########
@@ -4334,36 +4432,715 @@ 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)``.
+
+ Only lossless conversions: an integral number for an integer column, a
+ number a float represents exactly for a float column. Anything else is
+ returned unchanged -- a non-integral value on an integer column matches
+ no row, so its mirror cannot drop one, and a ``DECIMAL`` column's text
+ depends on a scale the spec does not expose.
+ """
+ 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
+ if issubclass(type_class, sa.Float) and float(value) == value:
+ return float(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.
+ if parsed.tzinfo is not None:
+ return _UNMIRRORABLE
+
+ 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,
+ 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:
Review Comment:
Addressed on Hugh's behalf in 54fe432f54: `_array_mirror_probe_value` now
catches `sa.exc.CompileError` alongside `NotImplementedError` and returns
`None`, so the preview declines instead of returning a 500. The chart path
declines the mirror the same way. Added a parametrized preview test for `[[1,
2]]` on `Array(Array(Int32))` and `[1j]`. Both cases returned a 500 before the
fix.
--
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]
