hughhhh commented on code in PR #45557:
URL: https://github.com/apache/superset/pull/45557#discussion_r4457952312
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -3385,6 +3397,65 @@ def _reads_as_temporal(value: str, db_engine_spec:
type[BaseEngineSpec]) -> bool
return False
+def _temporal_literal_sql(partition_column: TableColumn, value: date) -> str |
None:
+ """
+ A probed date or timestamp as the engine's own literal for the column.
+
+ The probe answers a temporal transform with a `date` or a `datetime`, and
+ SQLAlchemy's literal processor writes either one as plain quoted text --
+ ``part_date >= '2026-01-01'`` -- whatever the dialect. Trino and Presto
+ have no implicit coercion from ``varchar`` to ``date`` or ``timestamp``, so
+ they refuse that comparison outright, and because the mirror is ``AND``-ed
+ onto the chart query the whole chart fails rather than its pruning.
+
+ `BaseEngineSpec.convert_dttm` is how every other temporal bound in a chart
+ query is written (see `ExploreMixin.dttm_sql_literal`), so the mirror
+ writes its values the same way: ``DATE '2026-01-01'`` on Trino,
+ ``CAST('2026-01-01' AS DATE)`` on Hive.
+
+ `None` where that cannot be done faithfully, and the caller declines:
+
+ * The engine has no literal for the column's type. An engine Superset has
+ no rendering for is one it has not confirmed, the same answer
+ `_reads_as_temporal` gives for text.
+ * The literal throws part of the value away -- SQLite's, for one, stops at
+ the second. A bound floored past what the probe answered is a mirror
+ narrower than the filter, which drops rows. Detected the way
+ `ExploreMixin._engine_literal_resolution` detects it, by rendering a
+ second instant and comparing the text: a value one microsecond earlier
+ writes differently from this one exactly when nothing of it is lost.
+ A column holding only days is exempt, because flooring to the day is
+ the column's own resolution and every comparison against it reads the
+ same either way.
+ """
+ instant = (
+ value if isinstance(value, datetime) else datetime.combine(value,
time.min)
+ )
+ spec = partition_column.db_engine_spec
+ column_type = partition_column.type or ""
+ db_extra = partition_column.db_extra
+ rendered = spec.convert_dttm(column_type, instant, db_extra=db_extra)
+ if rendered is None:
+ return None
+ holds_days_only = isinstance(spec.get_sqla_column_type(column_type),
sa.Date)
+ if not holds_days_only and rendered == spec.convert_dttm(
+ column_type, instant - timedelta(microseconds=1), db_extra=db_extra
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
Yes, it's guarded now, in f67dfa473c. `_temporal_literal_sql` catches the
`OverflowError` from `instant - timedelta(microseconds=1)` and returns `None`,
so the mirror is declined and chart query construction continues.
`test_the_earliest_instant_declines_rather_than_failing_the_chart` covers
`datetime.min` and `date.min` on a TIMESTAMP key. Against master's source it
fails with `OverflowError` at the subtraction, and it passes with the guard.
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -3385,6 +3397,65 @@ def _reads_as_temporal(value: str, db_engine_spec:
type[BaseEngineSpec]) -> bool
return False
+def _temporal_literal_sql(partition_column: TableColumn, value: date) -> str |
None:
+ """
+ A probed date or timestamp as the engine's own literal for the column.
+
+ The probe answers a temporal transform with a `date` or a `datetime`, and
+ SQLAlchemy's literal processor writes either one as plain quoted text --
+ ``part_date >= '2026-01-01'`` -- whatever the dialect. Trino and Presto
+ have no implicit coercion from ``varchar`` to ``date`` or ``timestamp``, so
+ they refuse that comparison outright, and because the mirror is ``AND``-ed
+ onto the chart query the whole chart fails rather than its pruning.
+
+ `BaseEngineSpec.convert_dttm` is how every other temporal bound in a chart
+ query is written (see `ExploreMixin.dttm_sql_literal`), so the mirror
+ writes its values the same way: ``DATE '2026-01-01'`` on Trino,
+ ``CAST('2026-01-01' AS DATE)`` on Hive.
+
+ `None` where that cannot be done faithfully, and the caller declines:
+
+ * The engine has no literal for the column's type. An engine Superset has
+ no rendering for is one it has not confirmed, the same answer
+ `_reads_as_temporal` gives for text.
+ * The literal throws part of the value away -- SQLite's, for one, stops at
+ the second. A bound floored past what the probe answered is a mirror
+ narrower than the filter, which drops rows. Detected the way
+ `ExploreMixin._engine_literal_resolution` detects it, by rendering a
+ second instant and comparing the text: a value one microsecond earlier
+ writes differently from this one exactly when nothing of it is lost.
+ A column holding only days is exempt, because flooring to the day is
+ the column's own resolution and every comparison against it reads the
+ same either way.
+ """
+ instant = (
+ value if isinstance(value, datetime) else datetime.combine(value,
time.min)
+ )
+ spec = partition_column.db_engine_spec
+ column_type = partition_column.type or ""
+ db_extra = partition_column.db_extra
+ rendered = spec.convert_dttm(column_type, instant, db_extra=db_extra)
+ if rendered is None:
+ return None
+ holds_days_only = isinstance(spec.get_sqla_column_type(column_type),
sa.Date)
+ if not holds_days_only and rendered == spec.convert_dttm(
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
The defect is real, but I'm not fixing it in this PR. It predates this PR,
which now only carries the `datetime.min` guard, and a fix needs its own
decision about how to find the engine's actual resolution.
I reproduced it on Azure SQL Edge (engine 16.0):
- `CONVERT(DATETIME, '2026-01-01T12:30:15.001', 126)` reads back as
`12:30:15.000`.
- A `DATETIME2(3)` row at `12:30:15.001` matches neither `part <= <literal>`
nor `part = <literal>`, so the mirror drops it.
- The same row matches the quoted text `part <= '2026-01-01T12:30:15.001'`.
`_temporal_literal_sql` on current code returns that literal for a
`DATETIME2` key, because `.001` and `.000` differ as text.
`MssqlEngineSpec.convert_dttm` writes every non-`SMALLDATETIME` `DateTime` type
as `CONVERT(DATETIME, ...)`, so the check can't see the rounding. I only tested
`.001`, so I haven't mapped which other millisecond values are affected.
I'm leaving this thread open so you and hughhhh can decide on a follow-up.
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -3385,6 +3397,65 @@ def _reads_as_temporal(value: str, db_engine_spec:
type[BaseEngineSpec]) -> bool
return False
+def _temporal_literal_sql(partition_column: TableColumn, value: date) -> str |
None:
+ """
+ A probed date or timestamp as the engine's own literal for the column.
+
+ The probe answers a temporal transform with a `date` or a `datetime`, and
+ SQLAlchemy's literal processor writes either one as plain quoted text --
+ ``part_date >= '2026-01-01'`` -- whatever the dialect. Trino and Presto
+ have no implicit coercion from ``varchar`` to ``date`` or ``timestamp``, so
+ they refuse that comparison outright, and because the mirror is ``AND``-ed
+ onto the chart query the whole chart fails rather than its pruning.
+
+ `BaseEngineSpec.convert_dttm` is how every other temporal bound in a chart
+ query is written (see `ExploreMixin.dttm_sql_literal`), so the mirror
+ writes its values the same way: ``DATE '2026-01-01'`` on Trino,
+ ``CAST('2026-01-01' AS DATE)`` on Hive.
+
+ `None` where that cannot be done faithfully, and the caller declines:
+
+ * The engine has no literal for the column's type. An engine Superset has
+ no rendering for is one it has not confirmed, the same answer
+ `_reads_as_temporal` gives for text.
+ * The literal throws part of the value away -- SQLite's, for one, stops at
+ the second. A bound floored past what the probe answered is a mirror
+ narrower than the filter, which drops rows. Detected the way
+ `ExploreMixin._engine_literal_resolution` detects it, by rendering a
+ second instant and comparing the text: a value one microsecond earlier
+ writes differently from this one exactly when nothing of it is lost.
+ A column holding only days is exempt, because flooring to the day is
+ the column's own resolution and every comparison against it reads the
+ same either way.
+ """
+ instant = (
+ value if isinstance(value, datetime) else datetime.combine(value,
time.min)
+ )
+ spec = partition_column.db_engine_spec
+ column_type = partition_column.type or ""
+ db_extra = partition_column.db_extra
+ rendered = spec.convert_dttm(column_type, instant, db_extra=db_extra)
+ if rendered is None:
+ return None
+ holds_days_only = isinstance(spec.get_sqla_column_type(column_type),
sa.Date)
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
No change is needed here: master already handles this case, and the code you
commented on has been replaced. `holds_days_only` now lets the database's own
dialect override the spec's mapping. Oracle's dialect makes `DATE` a
`DateTime`, so the exemption doesn't apply and the precision check runs.
I checked this through `_temporal_literal_sql` on current code, for an
Oracle `DATE` key:
- `datetime(2026, 1, 1, 12, 30)` returns `None`, so the mirror is declined.
- `datetime(2026, 1, 1)` returns `TO_DATE('2026-01-01', 'YYYY-MM-DD')`,
which loses nothing.
`test_an_oracle_date_key_is_not_floored_to_its_day` and
`test_an_oracle_date_key_still_takes_an_exact_midnight` pin both results. They
came in with #43870, so this PR doesn't change them. Keying on the dialect's
type plus the literal text covers your 12:30 example without a separate
resolution lookup.
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -3385,6 +3397,65 @@ def _reads_as_temporal(value: str, db_engine_spec:
type[BaseEngineSpec]) -> bool
return False
+def _temporal_literal_sql(partition_column: TableColumn, value: date) -> str |
None:
+ """
+ A probed date or timestamp as the engine's own literal for the column.
+
+ The probe answers a temporal transform with a `date` or a `datetime`, and
+ SQLAlchemy's literal processor writes either one as plain quoted text --
+ ``part_date >= '2026-01-01'`` -- whatever the dialect. Trino and Presto
+ have no implicit coercion from ``varchar`` to ``date`` or ``timestamp``, so
+ they refuse that comparison outright, and because the mirror is ``AND``-ed
+ onto the chart query the whole chart fails rather than its pruning.
+
+ `BaseEngineSpec.convert_dttm` is how every other temporal bound in a chart
+ query is written (see `ExploreMixin.dttm_sql_literal`), so the mirror
+ writes its values the same way: ``DATE '2026-01-01'`` on Trino,
+ ``CAST('2026-01-01' AS DATE)`` on Hive.
+
+ `None` where that cannot be done faithfully, and the caller declines:
+
+ * The engine has no literal for the column's type. An engine Superset has
+ no rendering for is one it has not confirmed, the same answer
+ `_reads_as_temporal` gives for text.
+ * The literal throws part of the value away -- SQLite's, for one, stops at
+ the second. A bound floored past what the probe answered is a mirror
+ narrower than the filter, which drops rows. Detected the way
+ `ExploreMixin._engine_literal_resolution` detects it, by rendering a
+ second instant and comparing the text: a value one microsecond earlier
+ writes differently from this one exactly when nothing of it is lost.
+ A column holding only days is exempt, because flooring to the day is
+ the column's own resolution and every comparison against it reads the
+ same either way.
+ """
+ instant = (
+ value if isinstance(value, datetime) else datetime.combine(value,
time.min)
+ )
+ spec = partition_column.db_engine_spec
+ column_type = partition_column.type or ""
+ db_extra = partition_column.db_extra
+ rendered = spec.convert_dttm(column_type, instant, db_extra=db_extra)
+ if rendered is None:
+ return None
+ holds_days_only = isinstance(spec.get_sqla_column_type(column_type),
sa.Date)
+ if not holds_days_only and rendered == spec.convert_dttm(
+ column_type, instant - timedelta(microseconds=1), db_extra=db_extra
+ ):
+ return None
+ return rendered
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
Applied in f67dfa473c, using your suggested replacement, with regression
tests for `datetime.min` and `date.min` on a TIMESTAMP key and for the preview.
You were right that the literal fix was already in #43870. That has merged,
so this branch now carries only this guard. hughhhh chose to keep this PR open
for it instead of closing it, and I've updated the title and description to
match.
Your reachability claim holds: psycopg2 2.9.13 against Postgres 17 reads
`'-infinity'` as `datetime(1, 1, 1)` and `date(1, 1, 1)`, and on master the
chart path raises `OverflowError`.
One correction: on master the preview doesn't return 500. It returns 200
with `reason: "engine"`, which
`test_preview_declines_the_earliest_instant_rather_than_failing` shows when run
against master's source. With the guard it returns `reason: "type"`.
--
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]
