sadpandajoe commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4129803967
##########
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:
This runs on every `DatasetDAO.update()` call, including one that only
changes an unrelated field like `description` -- there's no check that
`attributes` actually touches partition/mapping state. Any dataset that already
has a `partition_value_transform` stored on a column other than the current
mapped one (a state the client editor alone couldn't fully prevent before this
PR) will have that value silently and permanently cleared the next time
anything about the dataset is saved, including a plain metadata edit or a
client that GETs the dataset and PUTs it back unchanged. Could this run only
when the request actually touches columns or the mapping fields, rather than on
every save?
##########
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:
`model.columns` here is whatever was already materialized earlier in this
same `update()` call by `_upsert_columns`/`_override_columns`, which insert new
rows via `db.session.add(TableColumn(..., table_id=model.id))` rather than
appending to that relationship collection -- so a column added and given
`partition_value_transform` in the same request as this call is invisible to
this loop, and its transform is never cleared. Since that's exactly the "stray
transform reactivates later" scenario this method exists to prevent, could this
include newly-inserted columns before clearing?
##########
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:
`field` is asserted in the tests but neither production consumer reads it --
`DatasourceEditor`'s validation maps straight to `.message`, and
`PartitionMappingSection` renders `.message` without checking `.field`. Could
this wire `field` into error placement at the named input, or drop it so it
isn't carrying metadata nothing uses?
##########
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:
`mirrorsMainDttm` doesn't check that a temporal column is even selected. The
backend only collects this `always_filter_main_dttm` mirror inside the `if
granularity:` branch (`models/helpers.py`), so with no granularity selected the
query never adds it -- but here the glyph shows purely from
`always_filter_main_dttm && main_dttm_col === mapping.mapped_column`,
regardless of `formData.granularity_sqla`. Could this also require a selected
granularity before mirroring the time range through this branch?
##########
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:
This can show the pruning glyph on a filter the query path does not actually
mirror. Two concrete cases: (1) a filter with an applied time grain
(drill-to-detail) is explicitly excluded from mirroring in `models/helpers.py`
("Mirroring it raw would keep only the bucket's first instant"), but
`MirrorCandidateFilter` carries no grain information, so a grained equality
filter is always treated as mirrored here; (2) an `IN` filter containing the
`<NULL>` sentinel (`superset/constants.py`'s `NULL_STRING`) is converted to a
real `None` server-side and excluded from mirroring there, but
`hasMirrorableValue`'s `value == null` check doesn't recognize the string
sentinel, so the glyph appears while no partition predicate is emitted for that
filter. Could the applicability check account for both?
##########
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:
This can't tell an echo of its own last commit from a genuinely new external
value, so it can still drop a keystroke: if `commit` fires and the resulting
prop update (say "AB") arrives after the owner has already typed further
("ABC"), this resets `localValue` back to "AB", discarding the extra keystroke
-- the next character then commits on top of the reverted text. The new
round-trip test only re-syncs after typing has already finished, so this
specific race (a commit firing while typing is still in progress) isn't
covered. Could this skip the resync when the incoming `value` matches what was
just committed?
--
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]
