hughhhh commented on code in PR #43870: URL: https://github.com/apache/superset/pull/43870#discussion_r4162114560
########## superset-frontend/src/explore/components/PartitionPruningIndicator/index.tsx: ########## @@ -0,0 +1,80 @@ +/** + * 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 { Icons, Tooltip } from '@superset-ui/core/components'; +import type { PartitionFilterMapping } from '@superset-ui/chart-controls'; + +interface PartitionPruningIndicatorProps { + /** The dataset's mapping summary, straight off the datasource payload. */ + mapping?: PartitionFilterMapping | null; +} + +/** + * The glyph on a filter whose column is mirrored onto a partition column + * (wireframe 1d). + * + * Chart authors do not configure any of this and ideally never learn the word + * "partition"; the indicator exists only to explain why their query got faster, + * and to point at the generated SQL where the extra predicate is visible. + */ +export default function PartitionPruningIndicator({ + mapping, +}: PartitionPruningIndicatorProps) { + const theme = useTheme(); + + if (!mapping?.active) { + return null; + } + + return ( + <Tooltip + placement="top" + title={t( + 'This filter is also applied to a partition column for faster queries. See "View query" for the generated SQL.', + )} + > + <span data-test="partition-pruning-indicator"> + <Icons.FilterOutlined + iconSize="s" + iconColor={theme.colorSuccess} + css={css` + margin-left: ${theme.sizeUnit}px; + vertical-align: middle; + `} + /> + </span> + </Tooltip> + ); +} + +/** + * Whether a filter on `columnName` is the one being mirrored. + * + * The mapping names exactly one mapped column, so anything else on the chart -- + * including other filters on the same dataset -- gets no indicator. + */ +export function isMirroredColumn( + mapping: PartitionFilterMapping | null | undefined, + columnName: string | null | undefined, +): boolean { + return Boolean( + mapping?.active && columnName && mapping.mapped_column === columnName, + ); Review Comment: Fixed in `bdfaeb9299` (earlier on this branch). `isMirroredFilter` checks the operator against `mirrorable_operators`, which the server computes from the transform's declared monotonicity — so `country != 'US'` gets no glyph. Tested at `PartitionPruningIndicator.test.tsx`. ########## superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx: ########## @@ -208,9 +208,56 @@ const time_grain_sqla: SharedControlConfig<'SelectControl'> = { sortComparator: () => 0, // Disable frontend sorting to preserve backend order }; +/** + * The mapping to hand the time control's partition-pruning indicator, or `null` + * when this time range is not mirrored. + * + * The control renders whatever mapping it is given, so the applicability check + * belongs here, where the selected temporal column and the range are both in + * scope. Unlike an ad-hoc filter chip the control names no column of its own: + * the query path mirrors the time range only when the *chart's* temporal column + * is the mapped one (`superset/models/helpers.py`, the `granularity` and + * `always_filter_main_dttm` branches), and only when the range resolves to at + * least one bound -- `No filter` resolves to neither. + */ +function timeRangePartitionMapping({ + datasource, + form_data: formData, +}: ControlPanelState) { + const dataset = datasource as Dataset | null; + const mapping = dataset?.partition_filter_mapping; + if ( + !mapping?.active || + !mapping.mirrorable_operators?.includes('TEMPORAL_RANGE') + ) { + return null; + } + if (formData?.time_range === NO_TIME_RANGE) { + return null; + } + const granularity = formData?.granularity_sqla; + const selectedColumn = isDefined(granularity) + ? getColumnLabel(granularity) + : undefined; + const mirrorsSelectedColumn = selectedColumn === mapping.mapped_column; + // `always_filter_main_dttm` adds a second time filter on the main datetime + // column even when the chart groups by another one, and that filter mirrors. + const mirrorsMainDttm = Boolean( + dataset?.always_filter_main_dttm && + dataset.main_dttm_col === mapping.mapped_column, + ); + return mirrorsSelectedColumn || mirrorsMainDttm ? mapping : null; +} + const time_range: SharedControlConfig<'DateFilterControl'> = { type: 'DateFilterControl', freeForm: true, + // The indicator needs to know whether this filter is mirrored onto a + // partition column; the summary is self-contained on the datasource so this + // does not have to reach into `columns`. + mapStateToProps: state => ({ Review Comment: Fixed in `46e4cd7c54` + `e21eac2728` (earlier on this branch). The control opts into render-time recalculation (`shouldMapStateToProps: () => true`) and names `granularity_sqla` in `validationDependencies`. The field-update regression case is `ControlPanelsContainer.test.tsx` — "the partition glyph drops when the time range becomes 'No filter'", built deliberately against the pre-action control state. ########## superset/daos/dataset.py: ########## @@ -461,7 +464,12 @@ def update( if force_update: attributes["changed_on"] = datetime.now() - return super().update(item, attributes) + updated = super().update(item, attributes) + # After the dataset-level attributes land, not before: the mapped column + # is `partition_mapped_column or main_dttm_col`, and either can be part + # of this very request. + cls.clear_unmapped_partition_transforms(updated) Review Comment: Fixed in `1e132aefc9`. Right — it discards stored configuration, so it had no business running on a write that cannot change which column is mirrored. It is now gated on the request actually touching `columns`, `main_dttm_col`, `partition_column` or `partition_mapped_column` (`PARTITION_MAPPING_ATTRIBUTES`). Pinned by `test_an_unrelated_save_leaves_a_stray_transform_alone`, which drives a `description`-only `update()`. ########## superset/daos/dataset.py: ########## @@ -475,6 +483,48 @@ def _validate_column_date_formats( "python_date_format is an invalid date/timestamp format." ) + @staticmethod + def clear_unmapped_partition_transforms(model: SqlaTable) -> None: + """ + Drop the value transform from every column the mapping does not mirror. + + A mapping has exactly one mirrored column, so at most one column may + carry a transform. A transform parked on any other column is invisible + -- no row but the mapped one renders one -- yet it is still stored, and + it goes live the moment the mapped column resolves back to it. Clearing + an override is enough to do that: a null `partition_mapped_column` means + "follow `main_dttm_col`", so dropping a mapping would otherwise activate + whatever the default datetime column happened to be holding, turning a + request to remove a mapping into a request to add a different one. + + Enforced here rather than in the editor alone because the editor is only + one writer: a PUT, an `override_columns=true` metadata sync and an + import all reach the columns directly. The same argument + `clear_dangling_partition_mapping` makes about dangling columns. + + Gated on the feature flag, unlike its sibling, because this discards + stored configuration rather than repairing a broken reference. With the + flag off nothing mirrors, so there is no armed mapping to disarm and + clearing would be pure loss. + """ + if not feature_flag_manager.is_feature_enabled(PARTITION_FILTER_MAPPING_FLAG): + return + + mapped_column = ( + (model.partition_mapped_column or model.main_dttm_col) + if model.partition_column + else None + ) + for column in model.columns: Review Comment: Fixed in `1e132aefc9`. Confirmed, and it was the one case the method most needed to catch. `db.session.flush()` + `db.session.expire(model, ["columns"])` now run before the loop, so the collection reflects the FK inserts. Pinned by `test_a_transform_on_a_column_added_in_the_same_request_is_cleared`, which adds a column *with* a transform and points the mapping elsewhere in one request — it fails without the flush. ########## superset-frontend/src/explore/components/PartitionPruningIndicator/index.tsx: ########## @@ -0,0 +1,169 @@ +/** + * 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 { NO_TIME_RANGE } from '@superset-ui/core'; +import { Icons, Tooltip } from '@superset-ui/core/components'; +import type { PartitionFilterMapping } from '@superset-ui/chart-controls'; +import { ExpressionTypes } from 'src/explore/components/controls/FilterControl/types'; + +interface PartitionPruningIndicatorProps { + /** The dataset's mapping summary, straight off the datasource payload. */ + mapping?: PartitionFilterMapping | null; +} + +/** + * The parts of an ad-hoc filter that decide whether it is mirrored. Structural + * rather than the `AdhocFilter` class so native filters and cross-filters -- + * which reach the query path in the same shape but are not that class -- can be + * checked with the same function. + */ +export interface MirrorCandidateFilter { + expressionType?: string; + subject?: string | { column_name?: string } | null; + operator?: string | null; + comparator?: unknown; +} + +/** + * The glyph on a filter whose column is mirrored onto a partition column + * (wireframe 1d). + * + * Chart authors do not configure any of this and ideally never learn the word + * "partition"; the indicator exists only to explain why their query got faster, + * and to point at the generated SQL where the extra predicate is visible. + */ +export default function PartitionPruningIndicator({ + mapping, +}: PartitionPruningIndicatorProps) { + const theme = useTheme(); + + if (!mapping?.active) { + return null; + } + + return ( + <Tooltip + placement="top" + title={t( + 'This filter is also applied to a partition column for faster queries. See "View query" for the generated SQL.', + )} + > + <span data-test="partition-pruning-indicator"> + <Icons.FilterOutlined + iconSize="s" + iconColor={theme.colorSuccess} + css={css` + margin-left: ${theme.sizeUnit}px; + vertical-align: middle; + `} + /> + </span> + </Tooltip> + ); +} + +/** + * The filter's column name. `subject` is a bare string for simple filters and a + * column object when the filter was built from a dropped column. + */ +function subjectName( + subject: MirrorCandidateFilter['subject'], +): string | undefined { + return typeof subject === 'string' + ? subject + : (subject?.column_name ?? undefined); +} + +/** + * Whether `columnName` is the one column the mapping mirrors. + * + * Necessary but not sufficient for the glyph -- see `isMirroredFilter`. + */ +export function isMirroredColumn( + mapping: PartitionFilterMapping | null | undefined, + columnName: string | null | undefined, +): boolean { + return Boolean( + mapping?.active && columnName && mapping.mapped_column === columnName, + ); +} + +/** + * Whether the comparator is one the mirrored predicate can be built from. + * + * A `NULL` inside an `IN` list widens the real predicate to + * `col IS NULL OR col IN (...)`, which the mirror cannot express, so the + * backend skips those lists outright. + */ +function hasMirrorableValue(operator: string, comparator: unknown): boolean { + if (operator === 'TEMPORAL_RANGE') { + // `No filter` resolves to neither bound, so no range is mirrored. + return typeof comparator === 'string' && comparator !== NO_TIME_RANGE; + } + if (operator === 'IN') { + return ( + Array.isArray(comparator) && + comparator.length > 0 && + !comparator.some(value => value == null) + ); + } + return comparator !== null && comparator !== undefined && comparator !== ''; +} + +/** + * Whether this filter actually produces a predicate on the partition column. + * + * Naming the mapped column is only the first of the query path's gates + * (`_collect_partition_mirror_filter` in `superset/models/helpers.py`): the + * operator has to be one the mapping can mirror, and the value has to be one + * the mirrored predicate can carry. A `country != 'US'` chip names the mapped + * column and mirrors nothing -- negations are never safe, because the transform + * need not be injective -- so labelling it would promise a speed-up the query + * does not deliver. + * + * The operator list is not restated here; it is computed server-side from the + * transform's declared monotonicity and shipped on the mapping. + */ +export function isMirroredFilter( + mapping: PartitionFilterMapping | null | undefined, + filter: MirrorCandidateFilter | null | undefined, +): boolean { + if (!mapping?.active || !filter) { + return false; + } + // Free-form SQL filters are appended verbatim as `extras.where`; the backend + // never sees an (operator, value) pair to mirror. + if ( + filter.expressionType && + filter.expressionType !== ExpressionTypes.Simple + ) { + return false; + } + if (!isMirroredColumn(mapping, subjectName(filter.subject))) { + return false; + } + if ( + !filter.operator || + !mapping.mirrorable_operators?.includes(filter.operator) + ) { + return false; + } + return hasMirrorableValue(filter.operator, filter.comparator); Review Comment: Fixed in `1e132aefc9`. Both accounted for. `MirrorCandidateFilter` carries `grain`, and a grain on anything but `TEMPORAL_RANGE` now means no glyph — matching the `and not filter_grain` skip in `models/helpers.py`. A grained range still shows, since that one mirrors by widening both bounds. `hasMirrorableValue` also recognises `NULL_STRING`, for the bare `==` case as well as inside an `IN` list. ########## superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx: ########## @@ -208,9 +208,68 @@ const time_grain_sqla: SharedControlConfig<'SelectControl'> = { sortComparator: () => 0, // Disable frontend sorting to preserve backend order }; +/** + * The mapping to hand the time control's partition-pruning indicator, or `null` + * when this time range is not mirrored. + * + * The control renders whatever mapping it is given, so the applicability check + * belongs here, where the selected temporal column and the range are both in + * scope. Unlike an ad-hoc filter chip the control names no column of its own: + * the query path mirrors the time range only when the *chart's* temporal column + * is the mapped one (`superset/models/helpers.py`, the `granularity` and + * `always_filter_main_dttm` branches), and only when the range resolves to at + * least one bound -- `No filter` resolves to neither. + */ +function timeRangePartitionMapping({ + datasource, + form_data: formData, +}: ControlPanelState) { + const dataset = datasource as Dataset | null; + const mapping = dataset?.partition_filter_mapping; + if ( + !mapping?.active || + !mapping.mirrorable_operators?.includes('TEMPORAL_RANGE') + ) { + return null; + } + if (formData?.time_range === NO_TIME_RANGE) { + return null; + } + const granularity = formData?.granularity_sqla; + const selectedColumn = isDefined(granularity) + ? getColumnLabel(granularity) + : undefined; + const mirrorsSelectedColumn = selectedColumn === mapping.mapped_column; + // `always_filter_main_dttm` adds a second time filter on the main datetime + // column even when the chart groups by another one, and that filter mirrors. + const mirrorsMainDttm = Boolean( Review Comment: Fixed in `1e132aefc9`. `mirrorsMainDttm` now requires a selected granularity, so it cannot fire where the backend's `if granularity:` branch never runs. The existing test always passed a truthy `granularity_sqla`, so the negative case is now covered too. -- 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]
