sadpandajoe commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4318321746
##########
superset/db_engine_specs/postgres.py:
##########
@@ -370,11 +373,33 @@ def blank_port_to_none(self, data: Any, **kwargs: Any) ->
Any:
return data
+#: The text forms PostgreSQL's date/time input reads, which
`RedshiftEngineSpec`
+#: shares. Both the ISO spellings and the unseparated ones --
+#: ``'20260115'::date`` is 2026-01-15 -- so a ``YYYYMMDD`` bucketing key
+#: mirrors on either.
+#:
+#: Declared on the concrete specs rather than on `PostgresBaseEngineSpec`,
+#: which Snowflake, Vertica, HANA, Netezza and Hologres also inherit.
+#: Snowflake's AUTO date input does not take ``YYYYMMDD`` and reads an
+#: all-digit string as epoch seconds, so inheriting this misread the mirror.
+POSTGRES_TEMPORAL_LITERAL_FORMATS = (
+ "%Y-%m-%d",
+ "%Y-%m-%d %H:%M:%S",
+ "%Y-%m-%dT%H:%M:%S",
+ "%Y-%m-%d %H:%M:%S.%f",
+ "%Y-%m-%dT%H:%M:%S.%f",
+ "%Y%m%d",
+ "%Y%m%d%H%M%S",
Review Comment:
PostgreSQL does not read an unseparated 14-digit string as a timestamp:
`select '20260706100811'::timestamp` fails with `timestamp out of range`, since
the digits are taken as a date. With a TIMESTAMP partition column and a
transform like `to_char(CAST(:value AS timestamp), 'YYYYMMDDHH24MISS')`, this
format lets the value pass the temporal literal gate, so the preview reports a
valid mapping and the chart query then carries `partition_ts =
'20260706100811'` and errors out instead of declining the mirror. Should
`%Y%m%d%H%M%S` be dropped from the shared list (or the result converted to a
supported literal) so the mirror is declined?
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/usePartitionMappingPreview.ts:
##########
@@ -0,0 +1,206 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect, useMemo, useRef, useState } from 'react';
+import { SupersetClient, getClientErrorObject } from '@superset-ui/core';
+import { t } from '@apache-superset/core/translation';
+import { useDebounceValue } from 'src/hooks/useDebounceValue';
+import type { PartitionMappingPreview } from './types';
+import { transformCanPreview } from './utils';
+
+interface PreviewRequest {
+ datasetId?: number;
+ mappedColumn: string;
+ partitionColumn: string;
+ valueTransform: string;
+ sampleValues: string[];
+ operator: string;
+ isMonotonic: boolean;
+ /**
+ * Whether both mapped and partition column exist in the *saved* dataset.
+ *
+ * The endpoint belongs to the saved dataset and resolves these names against
+ * its persisted columns, while the editor validates against its unsaved
+ * state. Mapping onto a column that has only been synced from source, not
+ * saved, therefore had the editor accept it while the preview reported that
+ * the column does not exist -- a valid candidate shown as broken.
+ */
+ columnsAreSaved: boolean;
+ /** Skip entirely when the section isn't showing a mapping. */
+ enabled: boolean;
+}
+
+/**
+ * Preview the predicate a transform would emit, debounced.
+ *
+ * The endpoint fires a real warehouse query, so the transform is debounced
+ * before it leaves the browser, and a transform that cannot work is not sent
at
+ * all. Neither is a guard -- the server keeps a per-user, per-dataset budget
of
+ * its own, and remains the authority on whether a transform is usable. The
+ * point is that a half-written transform should not spend that budget and
leave
+ * none for the finished one.
+ */
+export function usePartitionMappingPreview({
+ datasetId,
+ mappedColumn,
+ partitionColumn,
+ valueTransform,
+ sampleValues,
+ operator,
+ isMonotonic,
+ enabled,
+ columnsAreSaved,
+}: PreviewRequest) {
+ const [preview, setPreview] = useState<PartitionMappingPreview | null>(null);
+ const [loading, setLoading] = useState(false);
+ // Which request the displayed state belongs to. `AbortController` alone is
+ // not enough: aborting settles nothing, so a handler still runs and has to
+ // ask whether it is still the current one. See the effect below.
+ const latestRequest = useRef(0);
+
+ const debouncedTransform = useDebounceValue(valueTransform, 500);
+ // `sampleValues` is a fresh array on every render, so depending on it
+ // directly would refire the effect -- and spend the preview budget -- on
+ // every keystroke elsewhere in the row. Depend on its serialized form and
+ // hand the effect a value whose identity tracks that same key.
+ const sampleKey = JSON.stringify(sampleValues);
+ const samples = useMemo<string[]>(() => JSON.parse(sampleKey), [sampleKey]);
+
+ useEffect(() => {
+ const transform = debouncedTransform?.trim();
+ // Claimed before the early return as well, so a request already in flight
+ // cannot come back and overwrite what this run decided.
+ latestRequest.current += 1;
+ const requestId = latestRequest.current;
+
+ if (
+ enabled &&
+ datasetId &&
+ mappedColumn &&
+ !columnsAreSaved &&
+ transformCanPreview(transform, mappedColumn, partitionColumn)
+ ) {
+ // Answered without asking: the endpoint would resolve these names
+ // against the saved dataset and report a column that does not exist
+ // there yet. Said plainly rather than guessed at -- sending the
+ // candidate columns instead would have the probe build SQL from
+ // whatever the client claims their types are.
+ setPreview({
+ valid: false,
+ reason: 'unsaved',
+ error: t(
+ 'Save the dataset to preview this mapping. The preview runs against
the saved columns, and this mapping uses one that has not been saved yet.',
+ ),
+ });
+ setLoading(false);
+ return undefined;
+ }
+
+ if (
+ !enabled ||
+ !datasetId ||
+ !mappedColumn ||
+ !columnsAreSaved ||
+ !transformCanPreview(transform, mappedColumn, partitionColumn)
+ ) {
+ setPreview(null);
+ // The spinner belongs to a request this run has just superseded. Its own
+ // handlers see the abort and stop short of clearing it, so clearing it
+ // here is the only thing that does -- otherwise clearing the transform
+ // mid-flight left the panel spinning for good.
+ setLoading(false);
+ return undefined;
+ }
+
+ const controller = new AbortController();
+ setLoading(true);
+
+ SupersetClient.post({
+ endpoint: `/api/v1/dataset/${datasetId}/partition_mapping/preview/`,
+ jsonPayload: {
+ mapped_column: mappedColumn,
+ // The candidate, not the saved one: the owner is previewing a mapping
+ // they have not committed yet.
+ partition_column: partitionColumn,
+ value_transform: transform,
+ sample_values: samples,
+ operator,
+ is_monotonic: isMonotonic,
+ },
+ signal: controller.signal,
+ })
+ .then(({ json }) => {
+ // This hits a live warehouse query, so responses can arrive out of
+ // order. Without this an older one overwrites the current preview and
+ // reports "Valid" for a transform the input no longer holds.
+ if (requestId !== latestRequest.current) {
+ return;
+ }
+ setPreview(json.result as PartitionMappingPreview);
+ })
+ .catch(async error => {
+ if (requestId !== latestRequest.current) {
+ return;
+ }
+ const clientError = await getClientErrorObject(error);
+ // Asked again: normalizing the error reads the response body, so a
+ // newer request can resolve while this one is still in that await, and
+ // writing then put a stale error over a current valid preview -- the
+ // panel reporting a failure for the transform the box no longer holds.
+ if (requestId !== latestRequest.current) {
+ return;
+ }
+ setPreview({
+ valid: false,
Review Comment:
Every failed preview request, including a 429 from the documented per-minute
preview budget or a network error, is stored as `valid: false`, and
`PartitionMappingSection` publishes that as a negative verdict. After hitting
the limit on a mapping that works, the panel and the dataset banner say filters
will not be mirrored, although chart queries still mirror. Could a transport or
rate-limit failure stay an unknown verdict (like `unsaved`) rather than `valid:
false`, reserving `valid: false` for a response that actually evaluated the
mapping?
##########
tests/unit_tests/commands/dataset/update_test.py:
##########
@@ -1831,3 +1841,294 @@ def
test_update_dataset_rejects_malicious_fetch_values_predicate(
and "fetch_values_predicate" in (exc.field_name or "")
for exc in excinfo.value._exceptions
)
+
+
+def _mapping_command(
+ mocker: MockerFixture,
+ transform: str | None,
+ *,
+ partition_mapped_column: str | None = None,
+ main_dttm_col: str = "event_time",
+ database: Database | None = None,
+) -> UpdateDatasetCommand:
+ """
+ A command whose stored dataset maps `event_time` onto `dt_epoch`.
+
+ `database` takes a real one, for the gates that read more of it than
+ `backend` -- the function denylist is keyed on the engine spec's own name,
+ which a mock cannot supply.
+ """
+ mapped_column = mocker.MagicMock()
+ mapped_column.column_name = "event_time"
+ mapped_column.partition_value_transform = transform
+ partition_column = mocker.MagicMock()
+ partition_column.column_name = "dt_epoch"
+ # The partition column is the target of a mapping, never the source of one.
+ partition_column.partition_value_transform = None
+
+ mock_dataset = mocker.MagicMock(is_managed_externally=False)
+ if database is not None:
+ mock_dataset.database = database
+ else:
+ mock_dataset.database.backend = "sqlite"
+ mock_dataset.catalog = None
+ mock_dataset.schema = "main"
+ mock_dataset.columns = [mapped_column, partition_column]
+ mock_dataset.main_dttm_col = main_dttm_col
+ mock_dataset.partition_column = "dt_epoch"
+ mock_dataset.partition_mapped_column = partition_mapped_column
+
+ command = UpdateDatasetCommand(1, {})
+ command._model = mock_dataset
+ return command
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_an_unparseable_transform_does_not_block_the_save(
+ mocker: MockerFixture,
+) -> None:
+ """
+ An unparseable transform is a Tier-2 issue: the mapping saves and stays
+ inactive. `validate_stored_expression` rejects anything it cannot parse, so
+ running it here would turn that into a blocking error and cost the owner
the
+ rest of their edits.
+ """
+ gate =
mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(mocker, "unix_timestamp(:value")
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+ gate.assert_not_called()
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_parseable_transform_still_goes_through_the_stored_expression_gate(
+ mocker: MockerFixture,
+) -> None:
+ """
+ The gate that governs every other stored expression still runs -- now the
+ same `stored_expression_error` preview, import and the legacy save path
+ use, so the function denylist applies here too.
+ """
+ gate = mocker.patch(
+ "superset.commands.dataset.update.stored_expression_error",
return_value=None
+ )
+ command = _mapping_command(mocker, "unix_timestamp(:value)")
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+ gate.assert_called_once()
+ assert gate.call_args.args[-1] == "unix_timestamp(:value)"
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
[email protected](
+ "transform",
+ [
+ "unix_timestamp(:value) UNION ALL SELECT password FROM ab_user",
+ "unix_timestamp(:value); DROP TABLE ab_user",
+ "password || :value FROM ab_user",
+ ],
+ ids=["set-operation", "multi-statement", "bare-from"],
+)
+def test_a_transform_that_is_the_wrong_sql_still_reaches_the_gate(
+ mocker: MockerFixture, transform: str
+) -> None:
+ """
+ The companion to the test above, and the reason the skip condition is "not
+ SQL yet" rather than "does not parse". All three of these fail to parse as
+ a single statement, exactly as a half-typed transform does -- so a skip
+ keyed on parseability would have waved them through. None of them is on the
+ way to a valid transform.
+ """
+ command = _mapping_command(mocker, transform)
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ # Reported rather than silently accepted the way an unfinished transform
is.
+ assert exceptions
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_transform_calling_a_denied_function_is_refused_on_save(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A normal PUT was the one door into this field that skipped the function
+ denylist: preview, import and the legacy save path all ran
+ `stored_expression_error`, while this ran only the narrower parser gate. So
+ an owner could store what preview had just refused, and the probe would run
+ it on the next chart load.
+ """
+ command = _mapping_command(
+ mocker,
+ "schema_to_xml('public') || :value",
+ database=Database(database_name="pfm_pg",
sqlalchemy_uri="postgresql://u@h/d"),
+ )
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] ==
["partition_value_transform"]
+ assert "schema_to_xml" in str(exceptions[0].messages)
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_jinja_transform_is_still_rejected(mocker: MockerFixture) -> None:
+ """
+ Skipping the gate for unparseable transforms is not a hole for templating:
+ Jinja is a blocking issue of its own, reported before the gate is reached.
+ """
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(mocker, "unix_timestamp({{ current_user_id()
}})")
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] ==
["partition_value_transform"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_description_only_put_survives_an_implicit_self_mapping(
+ mocker: MockerFixture,
+) -> None:
+ """
+ `fetch_metadata` could choose the partition column as the default datetime
+ column without anyone asking, and a blocking self-mapping error then made
+ the dataset unsaveable forever -- including by the very write that would
+ have set the override that fixes it, and including a write that changes
+ nothing but the description.
+
+ The mapping is inert in that state either way, since
+ `resolve_partition_mapping` bails out on it, so the issue is reported
+ rather than blocking.
+ """
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(
+ mocker, "unix_timestamp(:value)", main_dttm_col="dt_epoch"
+ )
+ command._properties["description"] = "a new description"
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_self_mapping_is_rejected_while_the_feature_is_on(
Review Comment:
These mapping tests call `_validate_partition_mapping()` directly, so
removing its call from `UpdateDatasetCommand.validate()` leaves them green
while a PUT accepts an explicit self-mapping. Could one test drive
`UpdateDatasetCommand(...).run()` with the flag on and both references set to
`dt_epoch`, asserting a `DatasetInvalidError` on `partition_column` and that
the stored mapping is unchanged?
##########
superset/commands/dataset/importers/v0.py:
##########
@@ -291,7 +319,154 @@ def run(self) -> None:
.one()
)
dataset["database_id"] = database.id
- SqlaTable.import_from_dict(dataset, sync=self.sync)
+ self._prepare_partition_mapping(dataset, database)
+ table = SqlaTable.import_from_dict(dataset, sync=self.sync)
+ if table is not None:
+ imported.append(table)
+
+ self._repair_partition_mappings(imported)
+
+ @classmethod
+ def _default_omitted_monotonic_declarations(cls, config: Any) -> None:
+ """
+ Materialize `partition_transform_is_monotonic` wherever a transform is
+ set, anywhere in a v0 config.
+
+ The v1 importer does this on its own raw config, and for the same
+ reason: an overwrite keeps whatever the bundle omits, including a
+ ``True`` left behind by the transform this bundle is replacing. A
+ non-monotonic transform declared monotonic enables range mirrors that
+ exclude rows the filter keeps -- ``abs(:value)`` inheriting a stored
+ ``True`` turns ``n >= -1`` into ``p >= 1`` and misses ``n = 0``.
+
+ Walked rather than addressed directly because a v0 bundle comes in two
+ shapes: a CLI export nests datasets under ``databases[].tables[]``,
+ while a UI export is a bare list of datasets. Only columns that supply
+ a transform are touched, so a re-import that says nothing about a
+ column stays idempotent.
+ """
+ if isinstance(config, list):
+ for entry in config:
+ cls._default_omitted_monotonic_declarations(entry)
+ return
+ if not isinstance(config, dict):
+ return
+ _default_omitted_monotonic_declarations(config.get("columns") or [])
+ for key in (DATABASES_KEY, "tables"):
+ for child in config.get(key) or []:
+ cls._default_omitted_monotonic_declarations(child)
+
+ @staticmethod
+ def _prepare_partition_mapping(
+ dataset: dict[str, Any], database: Optional[Database]
+ ) -> None:
+ """
+ Run the two repairs that have to happen *before* the bundle is applied.
+
+ `_repair_partition_mappings` below runs after `import_from_dict`, which
+ is too late for either of these.
+
+ Sanitizing the bundle. The v1 importer drops a transform the save path
+ would refuse (`drop_unusable_partition_transforms`) on its way in, and
+ v0 did not -- so a legacy bundle naming ``partition_mapped_column: n``
+ and a transform of ``random() + :value`` on ``n`` imported happily and
+ then failed *every* later save: a description-only
+ ``PUT /api/v1/dataset/<id>`` returns 422 on the stored expression, and
+ the mapping never mirrors anything. Applied to the config rather than
+ the model because that is where the transform still is.
+
+ Disarming the old mapping.
`DatasetDAO.clear_unmapped_partition_transforms`
+ reads the mapping as it stands, so the post-import call can only
enforce
+ the one-transform invariant against the *new* references. An overwrite
+ that re-points the mapping makes a previously unmapped column the
+ effective one, and a transform parked there -- which nothing in the
+ bundle authored -- is found already mapped by the later call and kept.
+ A dataset mapping ``a -> p`` with a parked ``:value + 100`` on ``b``,
+ overwritten by a bundle setting ``partition_mapped_column: b`` and
+ omitting the columns, would mirror ``b = 5`` as ``p = 105`` and drop
the
+ row ``(a=1, b=5, p=1)``. The same pre-change call `DatasetDAO.update`
+ and the v1 importer both make, and for the same reason.
+
+ ``database`` is ``None`` only for a CLI bundle importing a connection
+ that does not exist yet, which by definition has no stored dataset to
+ disarm and no dialect to validate a transform against -- the same case
+ `drop_unusable_partition_transforms` already declines to act on.
+ """
+ if database is None:
+ return
+
+ # Deferred import: `daos.dataset` pulls in `views.base`, which
+ # circularly imports back into the commands package at app init.
+ from superset.daos.dataset import DatasetDAO # noqa: PLC0415
+
+ drop_unusable_partition_transforms(dataset, database)
+
+ if not (table_name := dataset.get("table_name")):
+ return
+ existing = (
+ db.session.query(SqlaTable)
+ .filter(
+ SqlaTable.table_name == table_name,
+ SqlaTable.schema == dataset.get("schema"),
+ SqlaTable.catalog == dataset["catalog"]
+ if dataset.get("catalog") is not None
+ else True,
+ SqlaTable.database_id == database.id,
+ )
+ .one_or_none()
+ )
+ if existing is not None:
+ DatasetDAO.clear_unmapped_partition_transforms(existing)
+
+ @staticmethod
+ def _repair_partition_mappings(datasets: list[SqlaTable]) -> None:
+ """
+ Keep the partition filter mapping honest across a v0 import.
+
+ The mapping's columns ride in `SqlaTable.export_fields`, so this path
+ carries them -- but it calls `SqlaTable.import_from_dict` directly and
+ goes through neither `DatasetDAO.update` nor the v1 importer, so
+ neither of the mapping's repairs ran. An older bundle imported with
+ ``--sync columns`` that omits the partition column deletes it and keeps
+ `partition_column` naming it: pruning silently stops, and every later
+ edit of that dataset fails the partition-column-not-found validation,
+ including a description-only PUT, which carries no columns payload and
+ so cannot reach the branch that forgives a stored-only dangle.
+
+ The same pair, in the same order, as `RefreshDatasetCommand` and the
+ v1 importer. Run after the import rather than around it because
+ ``import_from_dict`` is what discovers which datasets are involved.
+ """
+ if not datasets:
+ return
+ # Deferred import: `daos.dataset` pulls in `views.base`, which
+ # circularly imports back into the commands package at app init.
+ from superset.daos.dataset import DatasetDAO # noqa: PLC0415
+
+ db.session.flush()
+ for dataset in datasets:
+ DatasetDAO.clear_dangling_partition_mapping(
Review Comment:
`dataset.columns` was already loaded by
`clear_unmapped_partition_transforms(existing)` in
`_prepare_partition_mapping`, and a `--sync columns` import removes omitted
columns with `session.delete`, which leaves a deleted member in a loaded
collection until it is expired. So a bundle that omits the partition column `p`
still sees `p` as surviving here, `clear_dangling_partition_mapping` keeps
`partition_column = 'p'`, and every later save, including a description-only
PUT, fails the partition-column-not-found validation. Could the collection be
expired (as the v1 importer does) before computing the surviving column names?
--
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]
