geido commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4366311176
##########
superset/datasets/api.py:
##########
@@ -2064,6 +2165,134 @@ def get(self, id_or_uuid: str, **kwargs: Any) ->
Response:
entity_concurrency_token(SqlaTable, table.id, table.uuid),
)
+ @expose("/<int:pk>/partition_mapping/preview/", methods=("POST",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: (
+ f"{self.__class__.__name__}.partition_mapping_preview"
+ ),
+ log_to_statsd=False,
+ )
+ def partition_mapping_preview(self, pk: int) -> Response:
Review Comment:
Thanks! The midnight sample is refused now, but the editor previews a
TIMESTAMP column at `10:47:23`, where the text probe is declined first, so it
still says Valid. Charts don't run the check: set up in the editor, 06:30-18:30
returns 0 rows vs 48 with the flag off, and with `NLS_DATE_FORMAT='YYYY-MM-DD'`
the preview looks right but totals are short. Could the chart path compare both
shapes too?
[mp4](https://gist.githubusercontent.com/geido/2e9a0de274464f8bac4b8ac43e8888e9/raw/final-oracle-todate-ui.mp4)
ยท [NLS_DATE_FORMAT='YYYY-MM-DD': preview looks right, 482 vs 554
rows](https://gist.githubusercontent.com/geido/2e9a0de274464f8bac4b8ac43e8888e9/raw/final-oracle-todate-iso-nls.gif)
([mp4](https://gist.githubusercontent.com/geido/2e9a0de274464f8bac4b8ac43e8888e9/raw/final-oracle-todate-iso-nls.mp4))
##########
superset/models/helpers.py:
##########
@@ -4552,36 +4661,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
+
+ # 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.
+
+ An element the driver and the engine would not spell the same way
+ declines with `_UNMIRRORABLE_ESCAPING` instead, which the preview can
+ explain. It has to be asked here: once the elements are inside the
+ engine's own array syntax there is no single value left for
+ `value_round_trips` to compare a parse against.
+ """
+
+ def literal(entry: Any) -> Any:
+ elements = coerce_array_values(parse_array_literal(entry),
element_type)
+ if not all(
+ value_round_trips(self.database, element) for element in
elements
+ ):
+ return _UNMIRRORABLE_ESCAPING
+ return raw_probe_value(
+ self.database,
+ db_engine_spec.array_literal(elements),
+ )
+
+ try:
+ if operator in {utils.FilterOperator.IN,
utils.FilterOperator.NOT_IN}:
+ candidates = (
+ list(value) if isinstance(value, (list, tuple)) else
[value]
+ )
+ members = tuple(literal(candidate) for candidate in candidates)
+ # One element's problem is the whole list's, for the reason
+ # `mirror_probe_request` gives: mirroring the rest would be
+ # narrower than the filter.
+ return next(
+ (member for member in members if is_unmirrorable(member)),
+ members,
+ )
+ 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.
+
+ Rendering is only half of what has to hold about a literal: whether the
+ engine reads the rendered text back as the value the filter compares is
+ asked separately, by `mirror_probe_request` through
+ `partition_mapping.value_round_trips`.
+ """
+
+ 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.
+ """
+ # A decline the caller already made, passed through: the array branch
+ # and the preview's own coercion both answer with an `Unmirrorable` of
+ # their own, and re-reading it as a filter value here would lose the
+ # reason the preview is about to explain.
+ if is_unmirrorable(value):
+ return value
+
+ # 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
+
+ # Then, before any of the resolution rules: whether this database's
+ # driver and engine would write the value down the same way. They do
+ # not always -- see `partition_mapping.literal_round_trips` -- and a
+ # value they disagree about is either probed as something nobody
+ # filtered on or, where it closes its own quote, read as SQL. Its own
+ # reason, because nothing about the column, the operator or the
+ # transform is what went wrong, so none of the advice the resolution
+ # branch carries applies.
+ #
+ # One member is the whole list's problem, as below: mirroring the rest
+ # would be narrower than the filter.
+ candidates = value if isinstance(value, (list, tuple)) else [value]
+ if not all(
+ value_round_trips(self.database, candidate) for candidate in
candidates
+ ):
+ return UNMIRRORABLE_ESCAPING
+
+ 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):
+ # A `None` in the list widens the real predicate to
+ # `col IS NULL OR col IN (...)`. Mirroring only the non-null
+ # members would be *narrower* than the original filter and
+ # would drop rows it keeps. Checked before the conversion
+ # below, which passes a `None` straight through and could not
+ # be told apart from a value afterwards.
+ return UNMIRRORABLE
+ members = tuple(
+ self._mirror_probe_input(operator, item, col) for item in value
+ )
+ if declined := next(
+ (member for member in members if is_unmirrorable(member)), None
+ ):
+ # One member the engine compares more coarsely than the mirror
+ # can be built from is the whole list's problem: dropping just
+ # that member is the same narrowing the `None` guard above
+ # refuses. So the list declines entire, carrying the first
+ # member's reason -- the one an owner reading the preview would
+ # have to fix first anyway.
+ return declined
+ return members
+
+ if value is None:
+ return UNMIRRORABLE
+ return self._mirror_probe_input(operator, value, col)
+
+ def _build_partition_mirror_predicates(
+ self,
+ mapping: "PartitionMapping",
+ requests: list[tuple[utils.FilterOperator, Any]],
+ ) -> list[ColumnElement]:
+ """
+ Resolve collected mirror requests into predicates on the partition
column.
+
+ Requests are deduplicated first: a chart can reach both collection
points
+ for the same column -- a `granularity` time filter *and* a
+ `TEMPORAL_RANGE` ad-hoc filter on the same column is a routine Explore
+ configuration -- and emitting the predicate twice is harmless SQL but
+ makes "View query" surprising.
+ """
+ if not requests:
+ return []
+
+ deduped: list[tuple[utils.FilterOperator, Any]] = []
+ seen: set[Any] = set()
+ for operator, value in requests:
+ # Keyed on the operator the mirror is *emitted* with, not the one
+ # collected: a strict bound and its non-strict twin on the same
+ # value build the same predicate, so they are the same request.
+ key = (
+ mirror_operator(operator),
+ value if isinstance(value, Hashable) else repr(value),
+ )
+ if key not in seen:
+ seen.add(key)
+ deduped.append((operator, value))
+
+ return build_mirrored_predicates(cast("SqlaTable", self), mapping,
deduped)
Review Comment:
I think this can now fail every chart filtering the mapped column on
ClickHouse. `_holds_days_only` (from 6f37942f48) calls `.get` on
`dialect.ischema_names`, a list in clickhouse-connect 1.10/1.11, so
`toDate(:value)` onto a `Date` key returns 500 with the flag on (200 off) and
the preview shows "Fatal error". Could the lookup accept only a `Mapping`, and
this call fail open, as the docs say?
[mp4](https://gist.githubusercontent.com/geido/2e9a0de274464f8bac4b8ac43e8888e9/raw/final-clickhouse-date-key-500.mp4)
--
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]
