sadpandajoe commented on code in PR #43891: URL: https://github.com/apache/superset/pull/43891#discussion_r4129728956
########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionMappingSection.tsx: ########## @@ -0,0 +1,292 @@ +/** + * 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 { css, useTheme } from '@apache-superset/core/theme'; +import { Alert } from '@apache-superset/core/components'; +import { + Button, + Checkbox, + Flex, + Icons, + InfoTooltip, + Input, + Loading, + Typography, +} from '@superset-ui/core/components'; +import { usePartitionMappingPreview } from './usePartitionMappingPreview'; +import { + partitionRowState, + previewOperatorFor, + sampleValuesFor, +} from './utils'; +import type { + PartitionMappingColumn, + PartitionMappingDatasource, +} from './types'; + +interface PartitionMappingSectionProps { + /** Injected by Field when `passItemToControl` is set. */ + item?: PartitionMappingColumn; + /** The column's `partition_value_transform`, injected by Field. */ + value?: string | null; + onChange?: (newValue: string | null) => void; + datasource: PartitionMappingDatasource; + /** Move the mapping onto this column, pre-filling a transform if we have one. */ + onMoveMappingHere: (columnName: string) => void; + onRemoveMapping: () => void; + onMonotonicChange: (columnName: string, isMonotonic: boolean) => void; +} + +/** + * The "Partition filter mapping" subsection of a column's row expand + * (wireframes 1c, 1h). + * + * Three states, because the model is one partition column and one mapped + * column: the mapped column carries the transform, every other column offers to + * take the mapping over, and the partition column itself shows nothing -- it is + * the target, not a source. + */ +export default function PartitionMappingSection({ + item, + value, + onChange, + datasource, + onMoveMappingHere, + onRemoveMapping, + onMonotonicChange, +}: PartitionMappingSectionProps) { + const theme = useTheme(); + const columnName = item?.column_name ?? ''; + const state = partitionRowState(datasource, columnName); + const isMonotonic = Boolean(item?.partition_transform_is_monotonic); + const transform = value ?? ''; + const isTemporal = Boolean(item?.is_dttm); + + const { preview, loading } = usePartitionMappingPreview({ Review Comment: `usePartitionMappingPreview.ts` uses a literal NUL byte as an array delimiter (`sampleValues.join('\0')` at line 62, `sampleKey.split('\0')` at line 87). That's why `git diff`/GitHub render that file as "Binary files differ" instead of text — every future change to it will show the same way, hiding it from normal diff and blame tooling. Could the delimiter be something that isn't a valid string character in practice (or the round-trip through a string key be dropped in favor of the array itself)? ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionColumnFields.tsx: ########## @@ -0,0 +1,234 @@ +/** + * 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 { useMemo } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, useTheme } from '@apache-superset/core/theme'; +import { Alert } from '@apache-superset/core/components'; +import { + Flex, + Icons, + InfoTooltip, + Label, + Select, + Tooltip, + Typography, +} from '@superset-ui/core/components'; +import { + mappedColumnIsImplicit, + mappingIsActive, + resolveMappedColumn, + suggestedMappedColumn, +} from './utils'; +import type { + PartitionMappingColumn, + PartitionMappingDatasource, +} from './types'; + +interface PartitionColumnFieldsProps { + datasource: PartitionMappingDatasource; + columns: PartitionMappingColumn[]; + onPartitionColumnChange: (columnName: string | null) => void; + /** Open the given column's row expand in the Columns table. */ + onNavigateToColumn: (columnName: string) => void; +} + +/** + * "Partition column" and the computed "Maps to partition" (wireframes 1a, 1g). + * + * "Maps to partition" is deliberately read-only. It reflects + * `partition_mapped_column ?? main_dttm_col`, and a peer dropdown would let it + * drift from the default datetime column silently. The only way to override it + * is from the target column's own row, where a transform has to be supplied + * alongside. + */ +export default function PartitionColumnFields({ + datasource, + columns, + onPartitionColumnChange, + onNavigateToColumn, +}: PartitionColumnFieldsProps) { + const theme = useTheme(); + + const options = useMemo( + () => + columns.map(column => ({ + value: column.column_name, + label: column.column_name, + customLabel: ( + <Flex align="center" gap={theme.sizeUnit}> + <span>{column.column_name}</span> + {column.type && <Label>{column.type}</Label>} + </Flex> + ), + })), + [columns, theme.sizeUnit], + ); + + const mappedColumn = resolveMappedColumn(datasource); + const isImplicit = mappedColumnIsImplicit(datasource); + const isActive = mappingIsActive(datasource, columns); + const { partition_column: partitionColumn } = datasource; + + return ( + <Flex vertical gap={theme.sizeUnit} data-test="partition-column-fields"> + <Flex align="center" gap={theme.sizeUnit}> + <Typography.Text>{t('Partition column')}</Typography.Text> + <InfoTooltip + tooltip={t( + 'The physical column the engine partitions on. Filters on the mapped column are mirrored onto it so the engine can prune partitions.', + )} + /> + </Flex> + <Select + ariaLabel={t('Partition column')} + options={options} + value={partitionColumn ?? undefined} + onChange={value => onPartitionColumnChange((value as string) ?? null)} + onClear={() => onPartitionColumnChange(null)} + placeholder={t('None')} + allowClear + data-test="partition-column-select" + /> + <Typography.Text type="secondary"> + {t( + "Column used for partition pruning on this table. Selecting one hides it from Explore's dimension and filter pickers by default.", + )} + </Typography.Text> + + {partitionColumn && ( + <Flex vertical gap={theme.sizeUnit} data-test="maps-to-partition"> + <Flex align="center" gap={theme.sizeUnit}> + <Typography.Text type="secondary"> + {t('Maps to partition')} + </Typography.Text> + <InfoTooltip + tooltip={t( + 'The column whose filters are mirrored. It follows the default datetime column unless a different column holds the mapping.', + )} + /> + </Flex> + + {mappedColumn ? ( + <> + <Flex align="center" gap={theme.sizeUnit}> + <Label>{mappedColumn}</Label> + {isImplicit && ( + <> + <Typography.Text type="secondary"> + {t('Default datetime column')} + </Typography.Text> + <Tooltip + title={t( + 'Set from the default datetime column above, so re-pointing that column moves the mapping with it.', + )} + > + <Icons.LockOutlined + iconSize="s" + iconColor={theme.colorTextTertiary} + /> + </Tooltip> + </> + )} + </Flex> + <Typography.Text type="secondary"> + {t( + 'Filters on this column are mirrored onto the partition column.', + )}{' '} + <Typography.Link Review Comment: None of the four new `Typography.Link` action triggers this section introduces have an `href` — here, and also at line 171 and line 200 in this file, plus `PartitionMappingSection.tsx:106`. Ant Design's `Typography.Link` without `href` renders an `<a>` that's outside the natural tab order and doesn't respond to Enter/Space, so a keyboard-only user can't reach "Map a different column instead", "Customize the value transform", "Map a column", or "Move mapping to this column". `Remove mapping` a few lines below already uses a link-styled `Button` instead — could these four follow the same pattern? ########## superset/datasets/api.py: ########## @@ -2175,6 +2191,8 @@ def partition_mapping_preview(self, pk: int) -> Response: properties: valid: type: boolean + sample_input: Review Comment: The response schema documents `sample_input`, `emitted_predicate`, `error`, and `valid`, but not `reason` — even though every invalid result this endpoint returns now includes one of `parse`, `validation`, `operator`, `engine`, or `unconfigured` (see `preview_partition_mapping` in `partition_mapping.py`), and the frontend's `PartitionMappingPreview` type reads and branches on it. Generated OpenAPI clients have no typed way to see this field. Could `reason` be added to the response schema here? ########## docs/admin_docs/configuration/partition-filter-mapping.mdx: ########## @@ -0,0 +1,189 @@ +--- +title: Partition Filter Mapping +hide_title: true +sidebar_position: 15 +version: 1 +--- + +<!-- +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. +--> + +# Partition Filter Mapping + +Tables on Hadoop-family engines are often partitioned on a *technical* column — an epoch +integer, a lowercased region key — that no analyst would ever filter on. Unless a query +carries a predicate on that column, the engine scans every partition. + +Partition filter mapping makes this a dataset setting instead of a per-chart chore. A +dataset owner names the partition column, the business column whose filters should be +mirrored onto it, and a value transform. Superset then appends an equivalent predicate on +the partition column to every query. Chart authors change nothing; queries prune. + +:::caution Experimental +This feature is behind the `PARTITION_FILTER_MAPPING` feature flag and is off by default. +::: + +## Enabling it + +```python +FEATURE_FLAGS = { + "PARTITION_FILTER_MAPPING": True, +} +``` + +Configure it as a **static boolean**. `FEATURE_FLAGS` also accepts per-request callables, +but a flag that resolves differently per user or tenant would let a user with the feature +off read a cached chart result that was produced from pruned SQL by a user with it on. + +## Configuring a mapping + +In the dataset editor's **Columns** tab, under *Default Column Settings*, pick a +**Partition column**. By default the mapping follows the dataset's default datetime column, +so re-pointing that column moves the mapping with it; set an explicit override if you want it +pinned to a different column. + +Expand the mapped column's row in *Column Settings* and set the **value transform**: a SQL +expression containing a `:value` placeholder, which stands for the filter value being +mirrored. The **Transform preserves ordering** checkbox sits directly beneath it. + +| Mapped column | Partition column | Transform | +|---|---|---| +| `event_time` (`TIMESTAMP`) | `dt_epoch` (`BIGINT`) | `unix_timestamp(:value)` | +| `country` (`VARCHAR`) | `region_key` (`VARCHAR`) | `lower(:value)` | + +A filter of `event_time >= '2026-01-01'` then adds `dt_epoch >= 1767225600` to the query. +The added predicate is an ordinary `WHERE` clause and shows up in **View query**. + +### Transform preserves ordering + +Range filters — including the Explore time range, the most important case — are only +mirrored when you check **Transform preserves ordering**. + +Monotonicity is a property of the *transform*, not of the column's data type. +`unix_timestamp(:value)` preserves ordering. `hour(:value)`, +`date_format(:value, 'dd')` and `dayofweek(:value)` are all perfectly reasonable +partition transforms on a `TIMESTAMP` column and none of them do: `hour('2026-01-01 23:00')` +is greater than `hour('2026-01-02 01:00')` even though the first instant is earlier. Mirroring +a range through one of those would silently return wrong numbers, so Superset asks you to +declare it rather than guessing. + +When the box is unchecked, `=` and `IN` filters still mirror; ranges do not. + +## What is and isn't mirrored + +| Filter | Mirrored | +|---|---| +| `=`, `IN` | Always | +| `>`, `>=`, `<`, `<=`, time ranges | Only when the transform preserves ordering | +| `!=`, `NOT IN`, `LIKE`, `ILIKE`, `IS NULL`, `IS TRUE` | Never | + +Negations are never safe. A transform need not be injective: `lower(:value)` with +`country != 'US'` would mirror to `region_key != 'us'`, which excludes rows whose `country` +is already lowercase `'us'` — rows the original filter *keeps*. + +**Time grains.** A filter that carries a time grain — drill-to-detail, mostly — compares the +*truncated* column, so the raw bounds it carries do not describe the rows it keeps: a row in +the final partial bucket satisfies `DATE_TRUNC(...) < until` while `col < until` excludes it. +Grained *ranges* still mirror, with both bounds widened by one bucket so the mirror stays no +narrower than the real filter. A `P1D` drill therefore reads three days of partitions rather +than one, instead of scanning the table. Grained `=` and `IN` filters do not mirror. + +Widening is only applied to grains whose bucket width Superset knows, which means the +built-in ones. A grain you added through `TIME_GRAIN_ADDONS`, or a built-in grain whose SQL +you replaced through `TIME_GRAIN_ADDON_EXPRESSIONS`, has no width Superset can rely on and +simply does not mirror. + +Known gaps, all of which are out of scope rather than bugs: + +- **Filter-value dropdowns do not prune.** Populating a filter's value list runs its own + `SELECT DISTINCT`, which never goes through the chart query path. There is no filter to + mirror from. +- **Row-level security predicates do not mirror.** They are stored as raw SQL and appended + downstream of the structured filters. +- **Custom SQL `WHERE` clauses do not mirror**, for the same reason. +- **Columns with an active advanced data type do not mirror.** Those build their own + predicate shape from translated values, so there is no operator/value pair to mirror. +- Dashboard native filters and cross-filters *do* mirror — they arrive as ordinary filters — + they just carry no visual indicator in the filter bar. + +## The assumption this rests on + +Superset emits a predicate on the partition column that stands in for one on the mapped +column. That substitution is only valid if, for every row in the table: + +``` +partition_column = <transform>(mapped_column) +``` + +**Superset cannot verify this.** It is a property of whatever ETL populates the partition +column. If that job lags, backfills with different logic, or writes the partition key in a +different timezone than the transform resolves, mirrored predicates silently drop real rows +and charts show quietly wrong numbers. Confirm the invariant with whoever owns the pipeline +before enabling a mapping on a production dataset. + +Rows in a `NULL` partition are the one case Superset *does* defend against. A predicate like +`dt_epoch >= X` is `NULL` — and so drops the row — wherever `dt_epoch` is `NULL`, even when +the original filter matches it. That is reachable: a dynamic-partition insert on Hive or +Impala parks rows whose partition key was `NULL` in the default partition +(`__HIVE_DEFAULT_PARTITION__`), and the column reads back as `NULL` when you query it. A +transform that returns `NULL` for an input it cannot convert produces the same thing on any +engine. + +The mirrored predicate is therefore emitted as: + +```sql +(dt_epoch >= X AND dt_epoch < Y) OR dt_epoch IS NULL +``` + +The mirror only has to be *no narrower* than the filter it stands in for, so admitting the +`NULL` partition costs one extra partition read and keeps those rows. Engines still prune +everything else. + +## How the transform is evaluated + +The transform is evaluated against the engine — pinned to the dataset's database, catalog +and schema — and the result is emitted as a literal constant. Results are cached +(`PARTITION_TRANSFORM_PROBE_CACHE_TIMEOUT`, 24 hours by default), which matters because this +adds a round trip to the chart query path. Day-aligned ranges like "Last month" hit the cache +constantly; second-granularity relative ranges like "Last 24 hours" essentially never do. + +If the evaluation fails for any reason, no predicate is added: the query still runs and is +still correct, it just scans more partitions. + +Because the evaluation happens in a **different session** from the chart query, transforms +that call non-deterministic functions are rejected when you save. That includes `now()`, +`current_date`, `current_timestamp`, `rand()` and the zero-argument `unix_timestamp()`, which +means "now" on Hive and Impala. The one-argument `unix_timestamp(:value)` is fine. + +Session-dependent behaviour that Superset cannot detect is still your responsibility: +`unix_timestamp()` is timezone-dependent on Hive and Impala, so if the evaluating session and Review Comment: This names the zero-argument `unix_timestamp()`, but the paragraph right above says that form is rejected at save time — the one that can actually be saved and used is `unix_timestamp(:value)`, which is the one whose result actually depends on session timezone. Could the warning name `unix_timestamp(:value)` instead, so it's clear which usable transform needs care? ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionMappingSection.tsx: ########## @@ -0,0 +1,292 @@ +/** + * 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 { css, useTheme } from '@apache-superset/core/theme'; +import { Alert } from '@apache-superset/core/components'; +import { + Button, + Checkbox, + Flex, + Icons, + InfoTooltip, + Input, + Loading, + Typography, +} from '@superset-ui/core/components'; +import { usePartitionMappingPreview } from './usePartitionMappingPreview'; +import { + partitionRowState, + previewOperatorFor, + sampleValuesFor, +} from './utils'; +import type { + PartitionMappingColumn, + PartitionMappingDatasource, +} from './types'; + +interface PartitionMappingSectionProps { + /** Injected by Field when `passItemToControl` is set. */ + item?: PartitionMappingColumn; + /** The column's `partition_value_transform`, injected by Field. */ + value?: string | null; + onChange?: (newValue: string | null) => void; + datasource: PartitionMappingDatasource; + /** Move the mapping onto this column, pre-filling a transform if we have one. */ + onMoveMappingHere: (columnName: string) => void; + onRemoveMapping: () => void; + onMonotonicChange: (columnName: string, isMonotonic: boolean) => void; +} + +/** + * The "Partition filter mapping" subsection of a column's row expand + * (wireframes 1c, 1h). + * + * Three states, because the model is one partition column and one mapped + * column: the mapped column carries the transform, every other column offers to + * take the mapping over, and the partition column itself shows nothing -- it is + * the target, not a source. + */ +export default function PartitionMappingSection({ + item, + value, + onChange, + datasource, + onMoveMappingHere, + onRemoveMapping, + onMonotonicChange, +}: PartitionMappingSectionProps) { + const theme = useTheme(); + const columnName = item?.column_name ?? ''; + const state = partitionRowState(datasource, columnName); + const isMonotonic = Boolean(item?.partition_transform_is_monotonic); + const transform = value ?? ''; + const isTemporal = Boolean(item?.is_dttm); + + const { preview, loading } = usePartitionMappingPreview({ Review Comment: In `usePartitionMappingPreview.ts`, neither the success handler (`.then()`, line 93) nor the early-return guard (lines 66–74) checks whether the in-flight request is still the current one, so the preview panel this returns can get stuck or show stale data. If `transformCanPreview` starts failing while a request is pending (for example the owner clears the transform), the pending request's `.catch`/`.finally` both skip `setLoading(false)` because `controller.signal.aborted` is already true, and the next effect run's early return never resets `loading` either — the spinner then stays on indefinitely. Separately, `.then()` doesn't check `controller.signal.aborted`, so if an older, now-superseded request resolves after a newer one — plausible since this hits a live warehouse query — it overwrites the current preview with stale data, including showing "Valid" for a transform the input no longer contains. Could both paths track whether their request is still the latest before to uching state? ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/utils.ts: ########## @@ -0,0 +1,261 @@ +/** + * 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 type { + PartitionMappingColumn, + PartitionMappingDatasource, + PartitionRowState, +} from './types'; + +/** + * 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 the mapping will actually mirror anything. + * + * Deliberately the same cheap signals the backend's summary uses: a mapping + * with no transform is configured but inert, and saying so is the point of the + * warning in wireframe 1g. + */ +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, + ); Review Comment: This treats any non-blank `partition_value_transform` as an active mapping, but the backend's own `partition_filter_mapping_summary` (models.py) requires `is_transform_active()` — parseable and passing the denylist — before calling a mapping active. `PartitionColumnFields.tsx` uses this frontend `isActive` to choose between the green "Filters on X will automatically apply..." message and the "No value transform is set" warning, so a transform that's non-blank but fails to parse gets the confident green message even though the backend won't actually mirror it. Should this call the same validation the backend uses (or read the mapping's actual computed `active` state) instead of just checking for non-blank text? -- 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]
