hughhhh commented on code in PR #43870: URL: https://github.com/apache/superset/pull/43870#discussion_r4319661281
########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionColumnFields.tsx: ########## @@ -0,0 +1,319 @@ +/** + * 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 { + Button, + Flex, + Icons, + InfoTooltip, + Label, + Select, + Tooltip, + Typography, +} from '@superset-ui/core/components'; +import { + mappedColumnIsImplicit, + mappingIsActive, + partitionMappingErrors, + resolveMappedColumn, + suggestedMappedColumn, +} from './utils'; +import type { + PartitionMappingColumn, + PartitionMappingDatasource, + PartitionMappingIssue, +} from './types'; + +interface PartitionColumnFieldsProps { + datasource: PartitionMappingDatasource; + /** + * Physical columns: what the partition-column dropdown may offer, and where + * a replacement mapped column may be suggested from. A calculated column + * belongs in neither -- the engine cannot partition on an expression, and + * the mapping picker only renders on a physical column's row. + */ + columns: PartitionMappingColumn[]; + /** + * Every column on the dataset, physical and calculated. What *exists* is a + * different question from what may be picked, and validation asks the first: + * the backend accepts a calculated column as the mapped-column override, so + * checking existence against the physical columns alone reported a column + * that is really there as missing -- and blocked Save with no way out, since + * the dropdown cannot offer it back. + */ + allColumns: PartitionMappingColumn[]; + onPartitionColumnChange: (columnName: string | null) => void; + /** Open the given column's row expand in the Columns table. */ + onNavigateToColumn: (columnName: string) => void; + /** + * Whether the mapped column's own preview says the transform mirrors, or + * `null`/undefined when no verdict is in yet. The checks this section can + * make are all static -- a transform the database rejects clears every one + * of them -- so without the preview's answer the banner claimed a speed-up + * the query never delivers. + */ + previewMirrors?: boolean | null; +} + +/** + * "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, + allColumns, + onPartitionColumnChange, + onNavigateToColumn, + previewMirrors, +}: 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); + // The static checks are necessary, not sufficient: an unparseable expression + // or a function the database does not have needs the engine to spot, and the + // mapped column's row already asks it. Deferring to that answer where there + // is one is what keeps this banner and that panel from contradicting each + // other. A stored transform whose last probe failed says the same thing + // through the datasource's own summary, which is what an owner sees on + // reopening the editor. + const isActive = + mappingIsActive(datasource, allColumns) && + previewMirrors !== false && Review Comment: Fixed. The stored verdict applies only while the box still holds the transform it was about — `storedVerdictApplies = liveTransform === datasource.partition_filter_mapping?.evaluated_transform` — so a successful edit is no longer vetoed by the summary computed when the editor opened. ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionMappingSection.tsx: ########## @@ -0,0 +1,344 @@ +/** + * 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 { useEffect } 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 { + Button, + Checkbox, + Flex, + Icons, + InfoTooltip, + Input, + Loading, + Typography, +} from '@superset-ui/core/components'; +import { useDebouncedCommit } from './useDebouncedCommit'; +import { usePartitionMappingPreview } from './usePartitionMappingPreview'; +import { + partitionRowState, + previewOperatorFor, + sampleValuesFor, + valueTransformIssues, +} 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; + /** + * Report whether the previewed transform mirrors, so the dataset-level + * banner can stop claiming a mirror this panel has already refused. A + * one-way report rather than a lift of the debounced transform this panel + * owns -- `null` while there is no verdict to report. + */ + onPreviewVerdict?: (mirrors: boolean | null) => 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, + onPreviewVerdict, +}: PartitionMappingSectionProps) { + const theme = useTheme(); + const columnName = item?.column_name ?? ''; + const state = partitionRowState(datasource, columnName); + const isMonotonic = Boolean(item?.partition_transform_is_monotonic); + const isTemporal = Boolean(item?.is_dttm); + + // The editor's commit path echoes the value back several renders later, so an + // input driven straight off `value` loses whatever is typed in the meantime. + const { + value: transform, + onChange: onTransformChange, + flush: flushTransform, + } = useDebouncedCommit(value, (next: string) => onChange?.(next || null)); + + const { preview, loading } = usePartitionMappingPreview({ + datasetId: datasource.id, + mappedColumn: columnName, + partitionColumn: datasource.partition_column ?? '', + // The local value, not the committed one: a preview is a statement about + // the expression in the box. Keying it off the commit would stack the + // commit debounce, the round trip and the preview's own debounce, and show + // a predicate for text that is no longer on screen. + valueTransform: transform, + sampleValues: sampleValuesFor(item), + operator: previewOperatorFor(item), + isMonotonic, + enabled: state === 'mapped', + }); + + // Reported rather than read upward, because the transform the preview judges + // is the local, debounced one this panel holds. `null` while a request is in + // flight: the previous verdict belongs to text that is no longer on screen. + useEffect(() => { + onPreviewVerdict?.(loading || !preview ? null : preview.valid); Review Comment: Fixed. Only the mapped column's row reports to `onPreviewVerdict`, so expanding another column no longer resets the dataset banner's verdict to `null`. ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/useDebouncedCommit.ts: ########## @@ -0,0 +1,100 @@ +/** + * 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); + // What was last handed to `commit`. A commit's own echo arrives as a prop + // change like any other, and the re-seed below cannot tell the two apart -- + // so a commit landing while typing continues (a blur or Enter flushes + // immediately) used to reset the input to the committed text and drop every + // keystroke since. State rather than a ref because the re-seed reads it + // during render, which is exactly what a ref is not for. + const [lastCommitted, setLastCommitted] = useState<string | undefined>( + undefined, + ); + + // 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 fire a stale + // callback that writes to a superseded owner. + const debouncedCommit = useRef( + debounce((next: string, commitFn: (value: string) => void) => { + setLastCommitted(next); + commitFn(next); + }, delay), + ); + + useEffect(() => () => debouncedCommit.current.cancel(), []); + + const onChange = useCallback( + (next: string) => { + setLocalValue(next); + debouncedCommit.current(next, commit); Review Comment: Fixed. The commit callback is handed to the debounce at call time rather than captured when it was built, so the timer can no longer fire a callback closed over a superseded collection snapshot. ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/usePartitionMappingPreview.ts: ########## @@ -0,0 +1,153 @@ +/** + * 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 { useEffect, useMemo, useRef, useState } from 'react'; +import { SupersetClient, getClientErrorObject } from '@superset-ui/core'; +import { t } from '@apache-superset/core/translation'; +import { useDebounceValue } from 'src/hooks/useDebounceValue'; +import type { PartitionMappingPreview } from './types'; +import { transformCanPreview } from './utils'; + +interface PreviewRequest { + datasetId?: number; + mappedColumn: string; + partitionColumn: string; + valueTransform: string; + sampleValues: string[]; + operator: string; + isMonotonic: boolean; + /** Skip entirely when the section isn't showing a mapping. */ + enabled: boolean; +} + +/** + * Preview the predicate a transform would emit, debounced. + * + * The endpoint fires a real warehouse query, so the transform is debounced + * before it leaves the browser, and a transform that cannot work is not sent at + * all. Neither is a guard -- the server keeps a per-user, per-dataset budget of + * its own, and remains the authority on whether a transform is usable. The + * point is that a half-written transform should not spend that budget and leave + * none for the finished one. + */ +export function usePartitionMappingPreview({ + datasetId, + mappedColumn, + partitionColumn, + valueTransform, + sampleValues, + operator, + isMonotonic, + enabled, +}: PreviewRequest) { + const [preview, setPreview] = useState<PartitionMappingPreview | null>(null); + const [loading, setLoading] = useState(false); + // Which request the displayed state belongs to. `AbortController` alone is + // not enough: aborting settles nothing, so a handler still runs and has to + // ask whether it is still the current one. See the effect below. + const latestRequest = useRef(0); + + const debouncedTransform = useDebounceValue(valueTransform, 500); + // `sampleValues` is a fresh array on every render, so depending on it + // directly would refire the effect -- and spend the preview budget -- on + // every keystroke elsewhere in the row. Depend on its serialized form and + // hand the effect a value whose identity tracks that same key. + const sampleKey = JSON.stringify(sampleValues); + const samples = useMemo<string[]>(() => JSON.parse(sampleKey), [sampleKey]); + + useEffect(() => { + const transform = debouncedTransform?.trim(); + // Claimed before the early return as well, so a request already in flight + // cannot come back and overwrite what this run decided. + latestRequest.current += 1; + const requestId = latestRequest.current; + + if ( + !enabled || + !datasetId || + !mappedColumn || + !transformCanPreview(transform, mappedColumn, partitionColumn) + ) { + setPreview(null); + // The spinner belongs to a request this run has just superseded. Its own + // handlers see the abort and stop short of clearing it, so clearing it + // here is the only thing that does -- otherwise clearing the transform + // mid-flight left the panel spinning for good. + setLoading(false); + return undefined; + } + + const controller = new AbortController(); + setLoading(true); + + SupersetClient.post({ + endpoint: `/api/v1/dataset/${datasetId}/partition_mapping/preview/`, + jsonPayload: { + mapped_column: mappedColumn, + // The candidate, not the saved one: the owner is previewing a mapping + // they have not committed yet. + partition_column: partitionColumn, + value_transform: transform, + sample_values: samples, + operator, + is_monotonic: isMonotonic, + }, + signal: controller.signal, + }) + .then(({ json }) => { + // This hits a live warehouse query, so responses can arrive out of + // order. Without this an older one overwrites the current preview and + // reports "Valid" for a transform the input no longer holds. + if (requestId !== latestRequest.current) { + return; + } + setPreview(json.result as PartitionMappingPreview); + }) + .catch(async error => { + if (requestId !== latestRequest.current) { + return; + } + const clientError = await getClientErrorObject(error); + setPreview({ + valid: false, + error: + clientError.message || Review Comment: Fixed. `message` is preferred only when `typeof clientError.message === 'string'`, otherwise it falls through to `clientError.error`, so a marshmallow 400 with an object-valued `message` no longer reaches `Alert description` as a React child. ########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/utils.ts: ########## @@ -0,0 +1,510 @@ +/** + * 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) { Review Comment: Fixed. Only the explicit self-mapping blocks: `blocking: Boolean(datasource.partition_mapped_column)`. The implicit case is shown as a warning the owner can save through, matching `validate_partition_mapping`'s tiers. -- 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]
