sadpandajoe commented on code in PR #45557:
URL: https://github.com/apache/superset/pull/45557#discussion_r4340484015
##########
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:
`holds_days_only` exempts anything whose SQLAlchemy type is `sa.Date`, but
Oracle's `DATE` stores hours/minutes/seconds. For an Oracle `DATE` partition
column and a transform that answers `datetime(2026, 1, 1, 12, 30)`,
`convert_dttm` emits `TO_DATE('2026-01-01', 'YYYY-MM-DD')`, so the
day-resolution check passes and a `<=` or `==` mirror excludes rows at 12:30
that the filter keeps. Would it be safer to key the exemption on the engine's
actual literal resolution rather than the SQLAlchemy type?
##########
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:
Different literal text between `instant` and `instant - 1µs` doesn't prove
the engine keeps the value: the engine can still round the literal it parses.
On SQL Server, `convert_dttm` for a `DATETIME2` column emits `CONVERT(DATETIME,
'…T12:30:15.001', 126)`, and a probe of `12:30:15.001000` passes this guard,
but `DATETIME` rounds to .000/.003/.007 increments. An `==` or upper-bound
mirror then drops rows whose partition value is exactly the probed instant,
which the old precise quoted literal didn't. Should the guard also decline when
the spec's literal is coarser than the column type (or is this accepted as out
of scope for engines whose `convert_dttm` targets a narrower type)?
##########
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:
Agreed—for a `datetime.min` probe, `instant - timedelta(microseconds=1)`
raises `OverflowError` before the mirror can decline, so chart query
construction aborts instead of just losing pruning. Should that subtraction be
guarded so the helper returns `None` in that case?
--
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]
