eschutho commented on code in PR #44431:
URL: https://github.com/apache/superset/pull/44431#discussion_r4075988020
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionMappingSection.tsx:
##########
@@ -178,7 +184,16 @@ export default function PartitionMappingSection({
</Flex>
<Input
value={transform}
- onChange={event => onChange?.(event.target.value || null)}
+ onChange={event => {
+ const next = event.target.value || null;
+ onChange?.(next);
+ // Monotonicity is a property of the expression, so editing the
+ // transform re-opens the question. The identity `:value` provably
+ // preserves ordering and stays auto-declared; anything else is the
+ // owner's to declare, and editing away from `:value` must not
leave
+ // a stale auto-check behind.
+ onMonotonicChange(columnName, next === IDENTITY_TRANSFORM);
Review Comment:
Agreed this is a real tradeoff, but I think the current behavior is
intentional rather than a bug, and worth keeping.
Resetting `isMonotonic` to `false` on any edit away from the auto-managed
`:value` identity default is consistent with this codebase's existing design
principle (see the original stack's rationale in #43757/#43758): monotonicity
is a property of the *specific transform expression*, declared explicitly
rather than inferred, because it can't be safely assumed to survive an edit.
Silently preserving a stale `true` across an edited transform is the riskier
failure mode — a user could change `unix_timestamp(:value)` to something like
`dayofweek(:value)` (not monotonic) and have range mirroring proceed unsafely
if the checkbox stayed checked from before.
The friction (re-checking the box after an edit) is real, but it's the same
friction the rest of this feature already imposes by design. Open to being
convinced otherwise, but I'd want a concrete case where preserving the flag
across an edit is actually safe before changing this.
##########
superset/db_engine_specs/base.py:
##########
@@ -722,6 +722,17 @@ def get_extended_aggregation_func(
# editor offers no pre-fill and the owner writes the transform themselves.
partition_value_transform_default: str | None = None
+ # Whether the dataset editor should offer partition filter mapping at all.
+ # This is a structural property of the engine, not a pre-fill: mapping a
+ # filter onto a physical partition column only prunes work on engines whose
+ # tables are laid out as partition directories (Hive-family). Elsewhere the
+ # concept either does not exist (Postgres) or is handled invisibly by the
+ # engine (Snowflake micro-partitions, BigQuery/Redshift internals), so the
+ # dropdown would be clutter that maps to nothing. Default `False` so an
+ # engine only advertises the feature by opting in, rather than inheriting
it
+ # and having to remember to opt out.
+ supports_partition_filter_mapping: bool = False
Review Comment:
Fair point, and worth being explicit about the scoping decision here rather
than leaving it implicit.
This PR intentionally limits `supports_partition_filter_mapping = True` to
the exact same engines (`HiveEngineSpec`, `ImpalaEngineSpec`, and
`SparkEngineSpec` via inheritance) that already got
`partition_value_transform_default` set in #43891 — i.e., the set the original
stack's author had already validated. Athena (Trino/Presto-based) is a
legitimate candidate for `True` too, same as Trino/Presto themselves, but
extending coverage there is a scope decision for the stack's author/reviewers,
not something this gap-fix should decide unilaterally.
To be clear about what changed: before this PR, *every* engine showed the
partition-mapping UI (gated only by the feature flag), including engines where
partition-directory pruning doesn't structurally apply at all (Postgres,
Snowflake, BigQuery, Redshift) — that was the bug this PR fixes. For those
engines, `False` is correct, not a regression. Athena/Trino/Presto are a
different case — genuinely partition-capable engines that just weren't in the
original stack's scope — and I'd support a follow-up PR adding them explicitly
(with their own transform-default syntax, same as Hive/Impala got) rather than
folding that expansion into this fix.
--
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]
