sadpandajoe commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4365591270
##########
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:
When the engine answers `convert_dttm` for a `DATE` column, the filter
compares against the date-only literal (`'2026-01-01'` on SQLite), but this
returns the widened `datetime`, which the probe binds as `'2026-01-01
00:00:00.000000'`. With a `DATE` column `event_date`, a text partition key `p =
CAST(event_date AS TEXT)` and a monotonic `CAST(:value AS TEXT)` transform, a
January range mirrors as `p >= '2026-01-01 00:00:00.000000'`, which sorts above
the key `'2026-01-01'` and drops every January 1 row the filter keeps. Could
the probe use the same rendering the engine gives the original bound (for
example the date itself when the literal carries no time of day)?
##########
UPDATING.md:
##########
@@ -287,6 +287,49 @@ reattached to a reachable container by layout repair.
instead; the response sets `managed_externally: true`. Read-only tools and
certification inspection are unaffected.
+- `DISALLOWED_SQL_FUNCTIONS` now names the whole PostgreSQL XML family, plus
the
+ two full-text search functions that share its blind spot. Seven shapes were
+ missing -- `schema_to_xml`, `schema_to_xmlschema`,
+ `schema_to_xml_and_xmlschema`, `database_to_xml_and_xmlschema`,
+ `table_to_xmlschema`, `ts_rewrite` and `ts_stat` -- which, being scalar calls
+ with no FROM clause and no sub-query, cleared every gate that reasons about
+ table references. `ts_rewrite` and `ts_stat` run their text argument as a
+ query, so the statement they execute is a string with no clause of its own
for
+ such a gate to see. The same reasoning adds a new `oracle` entry naming
+ `getxml`, `getxmltype`, `newcontext` and `newcontextfromhierarchy`, the
+ `DBMS_XMLGEN` / `DBMS_XMLQUERY` calls that run a query handed to them as
+ text, and the URI readers that name a table by path string -- `dburitype`,
+ `xdburitype`, `httpuritype` and `geturi`, the last reached through
+ `URIFACTORY.GETURI(...)`. They are listed by bare name because that is what
+ the parser sees for a package-qualified call.
+
+ The denylist is shared with SQL Lab and Charts, so a
+ deployment relying on any of these functions there will now see it refused --
+ including a deployment that never enables `PARTITION_FILTER_MAPPING`, since
+ the denylist is not gated on the feature flag. Override
+ `DISALLOWED_SQL_FUNCTIONS` in `superset_config.py` if that is intended.
+
+- A partition value transform is now refused if it changes data --
+ `nextval(:value)`, a stored-procedure call, a PostgreSQL large-object writer.
+ The transform is evaluated against the engine to check that it mirrors, and
+ that evaluation is cached and repeated, so it has to be a read. The check is
+ the same `is_mutating` gate SQL Lab and chart queries already apply, and it
+ covers transforms already in storage: one stored by an earlier version simply
+ stops mirroring rather than failing a query.
+
+- Only the mapped column may carry a partition value transform, and that is now
+ enforced on every write rather than only inside `UpdateDatasetCommand`. A
+ transform arriving on any other column is dropped -- by the deprecated
+ `POST /datasource/save/`, and by `PUT /api/v1/dataset/<pk>` and dataset
import
+ even when `PARTITION_FILTER_MAPPING` is off. An API client that was storing
one there
Review Comment:
This says a transform sent on a non-mapped column is dropped and the field
reads `null` afterwards, including with the flag off, and
`partition-filter-mapping.mdx` repeats it. But
`drop_unmapped_value_transforms()` deliberately keeps an unchanged echo of the
stored value (`stored_by_name`), which is exactly what a GET-then-PUT, a legacy
save, or an overwrite import sends. An operator who follows this note to clear
a parked transform by re-saving the dataset will still find it there. Could
both places state the unchanged-echo exception and say how to clear it (send
`null`)?
##########
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:
+ """Preview the predicate a partition value transform would emit.
+ ---
+ post:
+ summary: Preview a partition filter mapping
+ description: >-
+ Evaluate a partition value transform at sample values and return
+ the predicate that would be appended to queries. The predicate is
+ built by the same code the query path uses, so the preview cannot
+ drift from what a chart emits. Parse and denylist checks run
+ before anything reaches the engine, so an unparseable transform
+ costs no query.
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The dataset ID
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/PartitionMappingPreviewSchema'
+ responses:
+ 200:
+ description: Preview result
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: object
+ properties:
+ valid:
+ type: boolean
+ reason:
+ type: string
+ description: >-
+ Why the mapping is not valid. Absent when it is.
+ The editor branches on this to decide how to
+ present the failure.
+ enum:
+ - unconfigured
+ - validation
+ - parse
+ - operator
+ - resolution
+ - offset
+ - escaping
+ - transform_escaping
+ - engine
+ - type
Review Comment:
`_preview_probe()` also returns HTTP 200 with `reason: "shape"` when the
range and text probes emit different predicates (for example an Oracle
`TO_DATE(:value, 'YYYY-MM-DD HH24:MI:SS')` transform on a temporal sample), but
`shape` is not in this enum, the generated `openapi.json` entry, or the
TypeScript union. A client generated from the spec with a strict enum would
reject that response. Could `shape` be added to all three and the spec
regenerated?
--
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]
