EnxDev commented on code in PR #45851:
URL: https://github.com/apache/superset/pull/45851#discussion_r4465700298
##########
superset/models/helpers.py:
##########
@@ -593,6 +593,21 @@ def shift(value: pd.Timestamp) -> pd.Timestamp:
_UNMIRRORABLE_ESCAPING = UNMIRRORABLE_ESCAPING
+def _is_temporal_column(col: Optional["TableColumn"]) -> bool:
+ """
+ Whether the engine compares ``col`` as a date or timestamp.
+
+ By generic type rather than the ``is_dttm`` flag alone, as
+ `ExploreMixin._engine_literal_resolution` reads it -- and not on a column
+ stored as text, which that flag can also mark. There the engine compares
+ the text with the text, so nothing is cast to an instant.
+ """
+ if col is None or not col.type or col.type_generic !=
GenericDataType.TEMPORAL:
+ return False
+ stored = col.db_engine_spec.get_column_spec(col.type,
db_extra=col.db_extra)
+ return stored is None or stored.generic_type != GenericDataType.STRING
Review Comment:
Might be misreading the canary test this leans on, but I don't think it
would notice if this returned True for a VARCHAR flagged temporal:
`iso_seconds` answers NULL for the datetime spelling and the mapping is
declined on the canaries either way.
Worth a direct test in `partition_mirroring_test` that puts an ISO string
through `==` on a VARCHAR column flagged `is_dttm` and asserts the probe got
the string verbatim.
##########
superset/models/helpers.py:
##########
@@ -5624,21 +5639,27 @@ def _mirror_probe_input(
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.
+ On a temporal column a string is never probed as the text it arrived
+ as, even where the engine compares the whole instant. The engine casts
+ the text to the column's type before comparing, so the filter matches
+ the *instant* -- but ``CAST(:value AS TEXT)`` handed the text answers
+ in the author's spelling, not the column's. On Postgres
+ ``t == '2026-07-06T10:08:11'`` matches the row whose text key is
+ ``'2026-07-06 10:08:11'``, and ``d == '2026-01-08T00:00:00'`` on a
+ ``DATE`` column -- what drill-by sends at the "Original value" grain --
+ matches the key ``'2026-01-08'``; probed verbatim, both mirrored onto a
+ key no row holds. So the instant is probed, through
+ `_mirror_probe_value` like any other: a `datetime`, or the `date`
itself
+ on a column holding only days. A string that is not ISO 8601 has no
+ instant to probe, and declines here too.
+
+ On any other column the string is the value the engine compares, and is
+ returned exactly as it arrived.
"""
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
- ):
+ if isinstance(value, str) and _is_temporal_column(col):
Review Comment:
This flips what the Explore glyph assumes. `hasMirrorableValue` in
`PartitionPruningIndicator/index.tsx` only withholds the indicator for an
unreadable string when `literal_resolution` is day or second, and its test pins
`'07/06/2026 10:08:11'` on a full-resolution column as mirrored. So on a
Postgres TIMESTAMP column the glyph now promises pruning the query declines.
The glyph can't tell a TIMESTAMP from a VARCHAR by `literal_resolution`
alone, both read as full. Could the summary carry whether the mapped column is
temporal, so the frontend gate can match this one?
##########
superset/models/helpers.py:
##########
@@ -5624,21 +5639,27 @@ def _mirror_probe_input(
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.
+ On a temporal column a string is never probed as the text it arrived
+ as, even where the engine compares the whole instant. The engine casts
+ the text to the column's type before comparing, so the filter matches
+ the *instant* -- but ``CAST(:value AS TEXT)`` handed the text answers
+ in the author's spelling, not the column's. On Postgres
+ ``t == '2026-07-06T10:08:11'`` matches the row whose text key is
+ ``'2026-07-06 10:08:11'``, and ``d == '2026-01-08T00:00:00'`` on a
+ ``DATE`` column -- what drill-by sends at the "Original value" grain --
+ matches the key ``'2026-01-08'``; probed verbatim, both mirrored onto a
+ key no row holds. So the instant is probed, through
+ `_mirror_probe_value` like any other: a `datetime`, or the `date`
itself
+ on a column holding only days. A string that is not ISO 8601 has no
+ instant to probe, and declines here too.
+
+ On any other column the string is the value the engine compares, and is
+ returned exactly as it arrived.
"""
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
- ):
+ if isinstance(value, str) and _is_temporal_column(col):
return _UNMIRRORABLE
Review Comment:
The docs still say the opposite of this. The Value resolution paragraph in
`docs/admin_docs/configuration/partition-filter-mapping.mdx` ends with "on a
TIMESTAMP column any format your database accepts mirrors, because there is
nothing left for Superset to resolve".
Mind rewording that to say a non-ISO value no longer mirrors on any temporal
column?
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -3760,6 +3821,37 @@ def _declined_for_reading_by_type(
text=text_answer,
)
)
+
+ reason: str | None = None
+ for index, text_answer in text_answers:
+ typed_answer = evaluated[index]
+ if isinstance(text_answer, TextRefused):
+ if not _is_bucket_of(values[index], typed_answer):
+ reason = str(
+ _(
+ "The transform could not read %(value)s written as "
+ "text, which a filter typed as text sends, so its "
+ "answer for the timestamp can only be checked by being
"
+ "a date or timestamp within a year of it. It answered "
+ "%(typed)s, so filters on this mapping will not be "
+ "mirrored. Write the transform so it reads :value both
"
+ "as a timestamp and as text.",
+ value=_as_filter_text(values[index]),
+ typed=typed_answer,
+ )
+ )
+ elif (
+ text_answer.hour is not None
+ and text_answer.text_hour is not None
+ and text_answer.text_hour != text_answer.typed_hour
+ ):
+ reason = read_differently(text_answer.typed_hour,
text_answer.text_hour)
Review Comment:
A transform that fails this hour check is declined on every chart, but the
typed probe that ran just before it recorded `mirrors=True` and nothing here
writes it back. So the summary's `evaluable` stays true, and the editor banner
and Explore glyph keep promising pruning that never happens.
The raw-instant decline re-records False for exactly that reason. The
first-round "reads differently" path has the same gap, so not new here, but
since this adds a third way in, should `_declined_for_reading_by_type` record
False too?
--
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]
