hughhhh commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4392784632
##########
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
Review Comment:
{ai generated comment}
Thanks, fixed in d91389152e. When the engine renders the bound and the
column holds only days, `_mirror_probe_value` now returns `dttm.date()` at
midnight, so the probe binds the same day the filter compares. Columns that
store a time of day keep the `datetime`: TIMESTAMP, and Oracle `DATE` (via
`holds_days_only`, which reads the dialect's own type). With your example run
against a real SQLite file, a January range now mirrors as `p >= '2026-01-01'
AND p <= '2026-02-01'` and keeps every January 1 row.
One note: at the head you reviewed, this case no longer dropped rows. The
new two-shapes check saw the timestamp answer (`…00:00:00.000000`) disagree
with the text one (`…00:00:00`) and declined the mirror, so results were
correct but the pruning was lost. With the date probe the two forms agree, so
the mirror is kept. Covered by
`test_a_midnight_bound_on_a_date_column_is_probed_as_the_day`.
--
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]
