sadpandajoe commented on code in PR #44431:
URL: https://github.com/apache/superset/pull/44431#discussion_r4130996005
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/utils.ts:
##########
@@ -420,7 +463,15 @@ export function applyImplicitMappingMove<T extends
PartitionMappingColumn>(
if (previousColumnName === nextColumnName) {
return columns;
}
- return clearMappingTransforms(columns);
+ const previous = columns.find(
+ column => column.column_name === previousColumnName,
+ );
+ return withMappingOn(
Review Comment:
`applyImplicitMappingMove` now copies the previous column's
`partition_value_transform` and `partition_transform_is_monotonic` onto the
newly-pointed default datetime column instead of clearing them. If an owner
re-points the default datetime column to a column with different semantics than
the one the transform was written for (e.g. `event_time` → `ingest_time` on a
table partitioned by `event_time`), the mapping now stays active and
immediately mirrors filters onto the partition column using the old column's
unverified transform, silently pruning the wrong rows — rather than going inert
until the owner reviews it, which is what clearing on re-point (the removed
behavior) guaranteed. Was carrying the transform over intentional, given that's
the exact risk the prior behavior was written to avoid?
##########
superset-frontend/src/explore/reducers/exploreReducer.ts:
##########
@@ -460,14 +460,7 @@ export default function exploreReducer(
>;
const dependantControls = Object.entries(controlsTyped)
.filter(
- ([key, item]) =>
- // A control is never its own dependant. Rebuilding one here uses
- // the value held *before* this action, so a control that named
- // itself would overwrite the value this action just set -- the
- // control and `form_data` would then disagree for the rest of the
- // session, and the query is built from the controls. Recomputing a
- // control's own derived props belongs in `shouldMapStateToProps`.
- key !== controlName &&
+ ([, item]) =>
Review Comment:
Removing the `key !== controlName` guard here reopens the exact regression
it existed to prevent. `time_range`'s `validationDependencies` now includes
itself (to recompute the partition-mapping indicator), so every `time_range`
change now flows through this dependent-control rebuild too. That rebuild calls
`getControlStateFromControlConfig` with `controlState?.value` — the value from
*before* this action — and the result then overwrites the control in
`updatedControlStates`, which is spread last into the returned `controls`.
`form_data.time_range` gets the new value, but `controls.time_range.value`
reverts to the previous range, and the query is built from controls, not
`form_data`. This reaches every chart still using the standalone Time Range
control (Calendar, Horizon, Rose, TimePivot, PairedTTest, Partition,
TimeTable): picking a new range or "No filter" updates the indicator but the
chart queries with the stale range.
This diff also removes the three tests that covered exactly this
(`SET_FIELD_VALUE applies the new time range to the control, not just the form
data`, `SET_FIELD_VALUE ignores a control that names itself as a dependency`,
and the `CHARTS_WITH_A_TIME_RANGE_CONTROL` suite). Was dropping the guard
intentional, or does `time_range`'s own derived `partitionMapping` need to be
recomputed another way (e.g. back at render time, as the removed test's comment
describes) rather than through self-referencing `validationDependencies`?
##########
superset/connectors/sqla/models.py:
##########
@@ -1969,6 +1969,19 @@ def partition_value_transform_default(self) -> str |
None:
"""
return self.db_engine_spec.partition_value_transform_default
+ @property
+ def supports_partition_filter_mapping(self) -> bool:
Review Comment:
This gates the editor UI, but `resolve_partition_mapping()` in
`superset/connectors/sqla/partition_mapping.py` — the code that actually
mirrors a filter onto the partition column at query time — never checks it; it
only reads `partition_column`. Any dataset that already has a partition mapping
configured on an engine that doesn't get `True` here keeps that mapping
silently active at query time with no way left to inspect, verify, or remove it
from the UI. Concretely, `DatabricksPythonConnectorEngineSpec` inherits `False`
from `BaseEngineSpec`, while the legacy `DatabricksHiveEngineSpec` inherits
`True` from `HiveEngineSpec` — both connect to Databricks, which does lay
tables out as partition directories, so this isn't the same case as an engine
that never had a validated transform default. Should the runtime path also
check this flag, or does the capability gate need a closer look for engines
with a pre-existing configured mapping?
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionColumnFields.tsx:
##########
@@ -106,14 +118,18 @@ export default function PartitionColumnFields({
data-test="partition-column-select"
/>
<Typography.Text type="secondary">
- {t(
- "Column used for partition pruning on this table. Selecting one
hides it from Explore's dimension and filter pickers by default.",
- )}
+ {t('Column used for partition pruning on this table.')}
Review Comment:
This removes one place claiming the partition column gets hidden from
Explore, but the warning further down in this same file ("%(partition)s is
hidden from Explore, but no filter is mirrored onto it…") still makes the same
now-inaccurate claim now that `applyPartitionColumnDefaults` no longer touches
`filterable`/`groupby`. Worth updating that string 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]
