sadpandajoe commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4254790118
##########
superset/mcp_service/common/schema_discovery.py:
##########
@@ -160,6 +160,11 @@ def _get_sqlalchemy_type_name(col_type: Any) -> str:
"filter_select_enabled": "Whether filter select is enabled",
"normalize_columns": "Whether to normalize column names",
"always_filter_main_dttm": "Whether to always filter on the main datetime
column",
+ "partition_column": "Physical column the engine partitions on",
Review Comment:
Schema discovery now lists `partition_column` and `partition_mapped_column`
as selectable dataset fields, but `DatasetInfo` and `serialize_dataset_object`
do not carry them. A client that follows discovery and calls `get_dataset_info`
with `select_columns: ["id", "partition_column", "partition_mapped_column"]`
gets neither field back and no error. Should these be added to the dataset
schema and serializer, or dropped from discovery?
##########
superset/commands/dataset/importers/v0.py:
##########
@@ -291,7 +302,43 @@ def run(self) -> None:
.one()
)
dataset["database_id"] = database.id
- SqlaTable.import_from_dict(dataset, sync=self.sync)
+ table = SqlaTable.import_from_dict(dataset, sync=self.sync)
+ if table is not None:
+ imported.append(table)
+
+ self._repair_partition_mappings(imported)
+
+ @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:
The v0 repair only handles a dangling reference, while the v1 importer also
clears an explicit self-mapping and defaults an omitted monotonicity flag. A
legacy bundle that sets `partition_column` and `partition_mapped_column` to the
same existing column (e.g. `event_time`) imports cleanly, and then every later
save, even a description-only PUT, fails the blocking self-mapping validation.
Likewise a legacy bundle that supplies a transform like `abs(:value)` without
the monotonic key keeps a stored `True` on overwrite, so range filters can
mirror and drop matching rows (e.g. `n >= -1` becomes `p >= 1` and misses `n=0,
p=0`). Could v0 run the same self-mapping and omitted-monotonic normalization
as v1?
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/utils.ts:
##########
@@ -0,0 +1,607 @@
+/**
+ * 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 { t } from '@apache-superset/core/translation';
+import type {
+ PartitionMappingColumn,
+ PartitionMappingDatasource,
+ PartitionMappingIssue,
+ PartitionRowState,
+} from './types';
+
+/**
+ * Jinja delimiters.
+ *
+ * A transform is evaluated in a different context and at a different time from
+ * the chart query, so a template would not render the same way -- the backend
+ * rejects one outright. Shared so the check that declines to preview a Jinja
+ * transform and the check that blocks saving it cannot drift apart.
+ */
+const JINJA_PATTERN = /\{\{|\{%|\{#/;
+
+/**
+ * The column whose filters are mirrored.
+ *
+ * `partition_mapped_column` is an explicit override; `null` means "follow the
+ * default datetime column", so re-pointing that column moves the mapping with
+ * it. Mirrors `SqlaTable.partition_filter_mapping_summary` on the backend.
+ */
+export function resolveMappedColumn(
+ datasource: PartitionMappingDatasource,
+): string | null {
+ if (!datasource.partition_column) {
+ return null;
+ }
+ return datasource.partition_mapped_column || datasource.main_dttm_col ||
null;
+}
+
+/** Whether the mapped column came from `main_dttm_col` rather than an
override. */
+export function mappedColumnIsImplicit(
+ datasource: PartitionMappingDatasource,
+): boolean {
+ return Boolean(
+ datasource.partition_column &&
+ !datasource.partition_mapped_column &&
+ datasource.main_dttm_col,
+ );
+}
+
+/**
+ * Whether a transform is worth spending a preview request on.
+ *
+ * A necessary condition, never a sufficient one: the server stays the
authority
+ * on whether a transform is usable, and anything this cannot see -- an
+ * unparseable expression, a non-deterministic function -- still comes back
from
+ * it. What this catches is the cases that are knowably hopeless while the
owner
+ * is still typing, which would otherwise spend the per-user preview budget and
+ * leave none for the transform they eventually finish writing.
+ */
+export function transformCanPreview(
+ transform: string | null | undefined,
+ mappedColumn?: string | null,
+ partitionColumn?: string | null,
+): boolean {
+ const trimmed = transform?.trim();
+ if (!trimmed) {
+ return false;
+ }
+ // Without the placeholder there is no value to substitute, so the transform
+ // is inert however well-formed it is.
+ if (!/:value\b/.test(trimmed)) {
+ return false;
+ }
+ // Jinja is rejected on save, so previewing it is pointless.
+ if (JINJA_PATTERN.test(trimmed)) {
+ return false;
+ }
+ // A column cannot stand in for itself; the backend rejects this outright.
+ if (mappedColumn && partitionColumn && mappedColumn === partitionColumn) {
+ return false;
+ }
+ return true;
+}
+
+/**
+ * Whether the mapping will actually mirror anything.
+ *
+ * Drives the choice between the green "filters will automatically apply"
+ * message and the warning that nothing is mirrored yet, so "non-blank text in
+ * the box" is not a good enough test: a transform with no `:value` to
+ * substitute, or one carrying Jinja, is as inert as an empty one and the
+ * backend's own summary reports it inactive. Claiming otherwise tells an owner
+ * their queries are pruning when they are not.
+ *
+ * Shares `transformCanPreview`'s conditions rather than restating them, and is
+ * necessary rather than sufficient for the same reason: an unparseable
+ * expression or a non-deterministic function needs a SQL parser to spot, so
the
+ * server stays the authority -- `valueTransformIssues` surfaces what it says
at
+ * the transform field itself.
+ */
+export function mappingIsActive(
+ datasource: PartitionMappingDatasource,
+ columns: PartitionMappingColumn[],
+): boolean {
+ const mappedColumnName = resolveMappedColumn(datasource);
+ if (!mappedColumnName || mappedColumnName === datasource.partition_column) {
+ return false;
+ }
+ const mappedColumn = columns.find(
+ column => column.column_name === mappedColumnName,
+ );
+ return transformCanPreview(
+ mappedColumn?.partition_value_transform,
+ mappedColumnName,
+ datasource.partition_column,
+ );
+}
+
+/**
+ * What is wrong with one column's value transform, if anything.
+ *
+ * Split out of `partitionMappingErrors` because the two callers disagree about
+ * which text to judge. The save gate reads the committed column record; the
+ * field's own message has to describe the text on screen, which lags the
commit
+ * by a debounce. Passing the transform separately lets both ask the same
+ * question about different values instead of reimplementing the checks.
+ */
+export function valueTransformIssues(
+ column: PartitionMappingColumn | undefined,
+ transform: string | null | undefined,
+): PartitionMappingIssue[] {
+ if (!column) {
+ return [];
+ }
+
+ if (JINJA_PATTERN.test(transform ?? '')) {
+ return [
+ {
+ field: 'partition_value_transform',
+ message: t(
+ 'Jinja templating is not supported in a partition value transform.
The transform is evaluated in a different context and at a different time from
the chart query, so a template would not render the same way.',
+ ),
+ },
+ ];
+ }
+
+ // A temporal column falls back on the engine's own default, so leaving the
box
+ // empty there only makes the mapping inactive -- which the dataset-level
+ // warning already says. A non-temporal column has nothing to fall back on.
+ if (!column.is_dttm && !transform?.trim()) {
+ return [
+ {
+ field: 'partition_value_transform',
+ message: t(
+ 'A value transform is required on %(name)s, which is not a temporal
column. Use :value for each value.',
+ { name: column.column_name },
+ ),
+ },
+ ];
+ }
+
+ return [];
+}
+
+/**
+ * Everything wrong with the mapping that should stop the save.
+ *
+ * The client half of `validate_partition_mapping` in
+ * `superset/connectors/sqla/partition_mapping.py`, restricted to its blocking
+ * (Tier 1) issues. Those are the ones the PUT would reject anyway, so catching
+ * them here turns a 400 toast arriving after the modal closed into a disabled
+ * Save button that says why. Tier 2 is deliberately absent: an unparseable
+ * transform, or one missing `:value`, is "saved but stays inactive until it
+ * parses" per the PRD, and must not cost the owner the rest of their edits.
+ *
+ * One check is stricter than the backend's: an empty transform on a
+ * non-temporal mapped column blocks. The backend files that as Tier 2 for
every
+ * column, but a non-temporal column has no default transform to fall back on
--
+ * the field carries a required marker saying exactly this -- so the marker
+ * should mean something.
+ *
+ * Callers gate on the feature flag themselves, the way the two mount sites in
+ * `DatasourceEditor` already do; a dataset carrying a stale `partition_column`
+ * on an engine that cannot use one must not block a save the backend accepts.
+ */
+export function partitionMappingErrors(
+ datasource: PartitionMappingDatasource,
+ columns: PartitionMappingColumn[],
+): PartitionMappingIssue[] {
+ const partitionColumn = datasource.partition_column;
+ if (!partitionColumn) {
+ return [];
+ }
+
+ const issues: PartitionMappingIssue[] = [];
+ const columnNames = new Set(columns.map(column => column.column_name));
+
+ if (!columnNames.has(partitionColumn)) {
+ issues.push({
+ field: 'partition_column',
+ message: t('Partition column %(name)s is not a column on this dataset.',
{
+ name: partitionColumn,
+ }),
+ });
+ }
+
+ const override = datasource.partition_mapped_column;
+ if (override && !columnNames.has(override)) {
+ issues.push({
+ field: 'partition_mapped_column',
+ message: t('Mapped column %(name)s is not a column on this dataset.', {
+ name: override,
+ }),
+ });
+ }
+
+ const mappedColumnName = resolveMappedColumn(datasource);
+ if (mappedColumnName === partitionColumn) {
+ // Only the *explicit* self-mapping blocks the save, matching
+ // `validate_partition_mapping`: an override naming the partition column is
+ // something the owner asked for and can take back, so it is Tier 1 there.
+ //
+ // The implicit one -- no override, and `main_dttm_col` happens to equal
the
+ // partition column -- is Tier 2 on purpose. Blocking it here disabled Save
+ // on a dataset already in that state (metadata sync used to be able to
+ // produce one) even for a description-only edit, which is exactly the
+ // unsaveable dataset the backend comment says it avoids. The owner gets
the
+ // same message, as a warning they can save through.
+ //
+ // No transform check can follow either way: the transform is a property of
+ // the mapped column, and there is no coherent mapped column to read it
from.
+ issues.push({
+ field: 'partition_column',
+ blocking: Boolean(datasource.partition_mapped_column),
+ message: t(
+ 'The partition column cannot be mapped onto itself. %(name)s is both
the partition column and the mapped column.',
+ { name: partitionColumn },
+ ),
+ });
+ return issues;
+ }
+
+ const mappedColumn = columns.find(
+ column => column.column_name === mappedColumnName,
+ );
+ issues.push(
+ ...valueTransformIssues(
+ mappedColumn,
+ mappedColumn?.partition_value_transform,
+ ),
+ );
+
+ return issues;
+}
+
+/** Which of the three row-expand treatments a column gets. */
+export function partitionRowState(
+ datasource: PartitionMappingDatasource,
+ columnName: string,
+): PartitionRowState {
+ if (!datasource.partition_column) {
+ return 'none';
+ }
+ if (columnName === datasource.partition_column) {
+ return 'partition';
+ }
+ return resolveMappedColumn(datasource) === columnName ? 'mapped' :
'unmapped';
+}
+
+/**
+ * The transform to pre-fill when a column becomes the mapped one.
+ *
+ * Only temporal columns get one, and only when the engine supplies it:
+ * `unix_timestamp(:value)` is Hive-family syntax and would not parse on
+ * Postgres or BigQuery, so a wrong default is worse than none.
+ */
+export function defaultTransformFor(
+ datasource: PartitionMappingDatasource,
+ column: PartitionMappingColumn | undefined,
+): string {
+ if (!column?.is_dttm) {
+ return '';
+ }
+ return datasource.partition_value_transform_default || '';
+}
+
+/** Declared types whose values are numbers rather than strings. */
+const NUMERIC_TYPE_RE =
+ /INT|NUMERIC|DECIMAL|DOUBLE|FLOAT|REAL|NUMBER|BIGNUMERIC/i;
+
+/** The sample instant, 2026-01-15 00:00:00 UTC, in each epoch representation.
*/
+const EPOCH_SAMPLES: Record<string, string> = {
+ epoch_s: '1768435200',
+ epoch_ms: '1768435200000',
+};
+
+/**
+ * Samples the preview evaluates the transform at.
+ *
+ * Temporal columns get one timestamp, because a time-range bound is what they
+ * are mapped for. Everything else gets two values, so the preview demonstrates
+ * the element-wise `IN` an ordinary categorical filter actually produces.
+ *
+ * The samples follow the column's declared type, not just whether it is
+ * temporal. A mapped integer column with `CAST(:value AS INTEGER)` is a real
+ * configuration, and evaluating it at `'US'` makes the engine reject a
transform
+ * that an actual `IN (2025, 2026)` filter would mirror perfectly well -- the
+ * preview was reporting a fault in its own sample data.
+ *
+ * That includes a temporal column stored as a number. A `BIGINT` epoch column
+ * marked temporal compares epoch integers, and a chart's time range reaches it
+ * as one; a timestamp string coerces to nothing on that column, so the preview
+ * would report a broken mapping that the chart path mirrors correctly. Such a
+ * column gets the sample instant in its epoch representation, or the plain
+ * numeric samples when its format is not an epoch.
+ */
+export function sampleValuesFor(
+ column: PartitionMappingColumn | undefined,
+): string[] {
+ const isNumeric = Boolean(column?.type && NUMERIC_TYPE_RE.test(column.type));
+ if (column?.is_dttm) {
+ if (!isNumeric) {
+ return ['2026-01-15 00:00:00'];
Review Comment:
Every non-numeric temporal column gets `'2026-01-15 00:00:00'` regardless of
`python_date_format`. For a `STRING` column stored as `%Y%m%d` mapped to a
`DATE` partition key with `PARSE_DATE('%Y%m%d', :value)`, chart bounds arrive
as `'20260115'`, so the preview parses a differently formatted sample and
reports a working mapping as broken. Could the sample be rendered through the
column's `python_date_format` when one is set?
##########
superset/daos/dataset.py:
##########
@@ -434,35 +452,147 @@ def update(
delete_metric_ids removes only explicitly selected metrics, without
replacing or replaying the remaining metrics.
"""
+ # Snapshot the keys: the column and metric branches below `pop` theirs
+ # out of `attributes`, so by the time the partition-mapping cleanup
runs
+ # the dict no longer says what the request contained.
+ attribute_names = set(attributes or {})
+ touches_mapping = not
PARTITION_MAPPING_ATTRIBUTES.isdisjoint(attribute_names)
+
+ # Once before the update and once after, because one pass can only ever
+ # see one of the two resolutions and the bug lives between them.
+ #
+ # The cleanup skips whichever column the mapping currently mirrors. Run
+ # only afterwards, it therefore skips the column this very request has
+ # just *made* the mapped one -- and a transform parked there, invisible
+ # because no row but the mapped one renders one, goes live. That turns
+ # removing a mapping into adding a different one, exactly what
+ # `clear_unmapped_partition_transforms` exists to prevent.
+ #
+ # This pass clears what is unmapped under the *old* resolution, which
+ # includes the column about to become mapped. The pass after clears
+ # what the *new* resolution strands. A transform the request itself
+ # supplies lands between the two, via `update_columns`, and survives --
+ # correctly, since the owner typed it.
+ if item is not None and touches_mapping:
+ cls.clear_unmapped_partition_transforms(item)
+ # The same invariant against what the request *carries*, which the
+ # pair of model-level passes cannot answer for: they are gated on
+ # the feature flag, correctly -- they discard stored configuration
+ # -- so with the flag off a payload could put a transform on a
+ # non-mapped column and have `update_columns` write it. Nothing
+ # reads it while the flag is off, and then the flag goes on and the
+ # mapping resolves onto it. See `drop_unmapped_value_transforms`.
+ #
+ # Harmless with the flag on, where the pass above has already
+ # cleared the same columns on the model and the pass below answers
+ # for the resolution this request leaves behind.
+ #
+ # Each name falls back to the model, because the mapping is
+ # `partition_mapped_column or main_dttm_col` and a request may
+ # change one of the three without mentioning the others.
+ requested = attributes or {}
+ if cleared := drop_unmapped_value_transforms(
Review Comment:
With the flag off, this runs against the incoming columns and nulls the
transform on any column that is not the currently mapped one. If the editor
changes the default datetime column from `event_time` to `event_time2` and
saves, its columns payload still echoes `event_time`'s stored transform and
monotonic flag, so they are written back as NULL/False. Switching the default
column back and enabling the flag cannot recover the configuration, which is
the same permanent loss the editor-side guard is meant to prevent. Should this
skip clearing when the incoming value equals what is already stored, and only
drop a transform the request is newly introducing?
--
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]
