hughhhh commented on code in PR #43870: URL: https://github.com/apache/superset/pull/43870#discussion_r4162115360
########## 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 + // rather than captured when the debounce was built -- otherwise it would + // close over whichever render happened to create it, and the commit reads + // props (the column's monotonic flag) to decide what to write. + const debouncedCommit = useRef( + debounce( + (next: string, commitFn: (value: string) => void) => commitFn(next), + delay, + ), + ); + + useEffect(() => () => debouncedCommit.current.cancel(), []); + + const onChange = useCallback( + (next: string) => { + setLocalValue(next); + debouncedCommit.current(next, commit); + }, + [commit], + ); + + /** + * Commit a pending edit now. lodash's `flush` is a no-op when nothing is + * pending, so a blur with no edit behind it costs nothing. + */ + const flush = useCallback(() => { + debouncedCommit.current.flush(); + }, []); + + // Re-seed from the prop only when the prop itself changed. Adjusting state + // during render is React's documented pattern for this; deriving a display + // value without writing it back leaves `localValue` holding superseded text, + // so the next render that changes only an unrelated prop -- and so does not + // re-enter this branch -- puts the stale value back in the input. + if (prevValue !== value) { Review Comment: Fixed in `1e132aefc9`. The hook records what it last handed to `commit` and skips the re-seed for that one value, consuming it so a later genuine reset to the same text is still honoured. The new test flushes with Enter and keeps typing across the echo's arrival, which is the interleaving the debounce does not cover — it fails without the change. ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/types.ts: ########## @@ -55,3 +55,22 @@ export interface PartitionMappingPreview { * mapping rather than a source for one. */ export type PartitionRowState = 'mapped' | 'unmapped' | 'partition' | 'none'; + +/** + * One problem with the mapping that stops the save. + * + * The client half of `MappingValidationIssue` in + * `superset/connectors/sqla/partition_mapping.py`, without its `blocking` flag: + * the backend reports two tiers because a half-written transform is allowed to + * save and sit inactive, and only the blocking tier is worth stopping the owner + * in the editor for. Everything this carries blocks, so the flag has nothing to + * distinguish. `field` names the input at fault, so the message can be shown + * there rather than only in the Save button's tooltip. + */ +export interface PartitionMappingIssue { Review Comment: Fixed in `1e132aefc9`. Wired in rather than dropped. `PartitionColumnFields` now renders the `partition_column` and `partition_mapped_column` messages under the select each is about, instead of leaving an owner to work out which of three inputs the Save tooltip means. ########## 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: Fixed in `1e132aefc9`. Aligned with the backend — `_collect_partition_mirror_filter` excludes only `None`, so `col = ''` does mirror and the glyph belongs on it. The test that pinned the divergence the wrong way round is flipped, with `<NULL>` cases added alongside it. ########## 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: Out of scope for the indicator, and now written down where the check lives rather than left implicit. `removed_filters` is assembled while the query is generated and is not in `form_data`, so the only way for the browser to know would be to render the Jinja itself. The glyph is advisory; "View query" stays the authority. ########## 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: Fixed in `7951536198`. `_transform_is_usable` is deleted. The query-time gate is `is_transform_active` — the same function the indicator reads, which is `validate_transform` with the messages discarded — so the two cannot drift again. That also closes the `now()`-via-import path you describe: it is blocking there, so the mapping no longer resolves at all. ########## 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: Fixed in `1e132aefc9`. `(formData?.time_range ?? NO_TIME_RANGE) === NO_TIME_RANGE`, so a legacy saved chart with no key at all is treated the same as the sentinel. ########## 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: Fixed in `1e132aefc9`. It is a text-variant `Button` carrying the tooltip text as its `aria-label` now — the shape `InfoTooltip` already uses. A `span` with `tabIndex` was the first attempt and `jsx-a11y/no-noninteractive-tabindex` was right to reject it. The test tabs to it and asserts focus. -- 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]
