sadpandajoe commented on code in PR #43870: URL: https://github.com/apache/superset/pull/43870#discussion_r4137602299
########## 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( Review Comment: A virtual dataset's SQL can consume a filter via a Jinja `get_filters('col', remove_filter=True)` (or `get_time_filter(..., remove_filter=True)`) template call; the query builder then skips both the filter's own predicate and any partition mirror for that column entirely (`should_skip_filter` in `superset/models/helpers.py`). This function only looks at the filter's own shape, so it still shows the glyph for a filter on the mapped column that produces no SQL predicate at all in that case. Could this account for `removed_filters`, or is that out of scope for the indicator? ########## 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"> Review Comment: The tooltip's explanation is reachable only by mouse hover: the icon sits in a non-focusable `<span>` with no accessible name, so a keyboard or screen-reader user has no way to discover why the glyph is there. Could this add a focusable target with an accessible label (e.g. an `aria-label` matching the tooltip text)? ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/useDebouncedCommit.ts: ########## @@ -0,0 +1,85 @@ +/** + * 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 { useCallback, useEffect, useRef, useState } from 'react'; +import { debounce } from 'lodash-es'; +import { Constants } from '@superset-ui/core/components'; + +/** + * Local text state that commits upward on a debounce. + * + * The dataset editor's commit path is asynchronous and snapshot-based: a value + * handed to `onChange` travels Field -> Fieldset -> CollectionTable -> + * DatasourceEditor -> DatasourceModal and arrives back on the `value` prop + * several renders later, and the callback that commits it was built during an + * earlier render. An input driven straight off that prop therefore loses any + * keystroke that lands while a commit is in flight -- which is why every other + * control in the editor types into local state and commits on a debounce, and + * why Fieldset's own comment says the editor assumes exactly that. This is + * TextControl's contract (src/explore/components/controls/TextControl), minus + * the ControlHeader and the number parsing this field does not want. + */ +export function useDebouncedCommit( + value: string | null | undefined, + commit: (next: string) => void, + delay: number = Constants.FAST_DEBOUNCE, +) { + const [localValue, setLocalValue] = useState(value ?? ''); + const [prevValue, setPrevValue] = useState(value); + + // The commit fires from a timer, so the callback is handed in at call time Review Comment: This comment says the commit callback reads the column's monotonic flag to decide what to write, but `commit` here is just the plain `(next: string) => void` passed in by the caller -- this hook doesn't reference monotonicity anywhere. Looks like a leftover from an earlier, more specific version of this code; could this be corrected or removed? ########## superset/daos/dataset.py: ########## @@ -476,6 +484,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: Concretely: `_upsert_columns` and `_override_columns` attach a brand-new column via `db.session.add(TableColumn(..., table_id=model.id))` rather than through the `columns` relationship, and both already read `model.columns` earlier in the same call to build their by-id/by-name maps. When that collection was loaded before the insert, the new row isn't reflected in it when this loop runs right after -- so a single request that both adds a column with a `partition_value_transform` and points the mapping elsewhere leaves that transform stored and uncleared, and it goes live the moment the mapping later resolves back to that column. `tests/unit_tests/dao/dataset_test.py` already notes this relationship-cache gap for a different reason (see its comment near line 394). Could this loop read from the same collection the insert path writes to, or expire `model.columns` first? ########## 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 !== ''; Review Comment: `hasMirrorableValue` treats an empty string as not mirrorable (`comparator !== ''`), but the backend's mirror collector (`_collect_partition_mirror_filter` in `superset/models/helpers.py`) only excludes `None`, not `''`. A valid `column == ''` filter is mirrored server-side while Explore hides the indicator for it. Could this align with the backend's null-only exclusion? ########## superset/connectors/sqla/partition_mapping.py: ########## @@ -416,7 +416,7 @@ def resolve_partition_mapping(datasource: SqlaTable) -> PartitionMapping | None: if not _transform_is_usable(transform, datasource.database.backend): Review Comment: This trusts `_transform_is_usable` as the gate for real query-time mirroring, but that function (below) doesn't call `find_non_deterministic_functions`, while `validate_transform` (used both for PUT-time validation and for `is_transform_active`, which drives the Explore indicator) treats a non-deterministic function as blocking. Since import and `CreateDatasetCommand` don't run `validate_partition_mapping`, a transform calling e.g. `NOW()` can reach the database, be treated as usable here, and mirror filters with a value frozen at whatever moment the probe/query happened to run -- while Explore's own indicator correctly identifies it as inactive and shows no glyph, so there's no visible signal that mirroring is happening with a non-deterministic value. Could `_transform_is_usable` share the same check? ########## 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) { Review Comment: This checks `formData?.time_range === NO_TIME_RANGE`, but when `time_range` is simply absent from `form_data` (e.g. a legacy saved chart predating this control, or before defaults populate the state), the value is `undefined` rather than the sentinel, so this falls through to the mirrors-check below instead of returning null. Could a missing `time_range` be treated the same as `NO_TIME_RANGE` here? -- 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]
