Repository navigation
Conversation
PR #43891 added `partition_value_transform_default` on the engine spec so the dataset editor can pre-fill a temporal column's value transform, and set it only on Hive/Impala (Spark inheriting from Hive). That gates *which transform to suggest*, not *whether the feature applies to the engine at all*: the editor still offered the "Partition column" dropdown on every database whenever the `PARTITION_FILTER_MAPPING` flag was on, and `validate_partition_mapping` only used the engine to check the transform text parsed for that dialect -- never to reject the feature on engines where partition-directory pruning does not exist. Mirroring a filter onto a physical partition column only prunes work on engines whose tables are laid out as partition directories (Hive-family). On Postgres the concept does not exist; on Snowflake micro-partitions are automatic and invisible; BigQuery and Redshift use different mechanisms entirely. There the dropdown is clutter that maps to nothing. Add a `supports_partition_filter_mapping` capability flag on `BaseEngineSpec` (default `False`), set `True` on the same two specs that carry `partition_value_transform_default` -- `HiveEngineSpec` and `ImpalaEngineSpec`, with `SparkEngineSpec` inheriting it from Hive. Thread it to the client the same way the transform default travels: a read-only `SqlaTable` property sourced from the engine spec, exposed through `DatasetRestApi.show_columns`, and added to the frontend datasource types. The editor's `partitionMappingEnabled` gate now also requires the flag, so the partition mapping UI only appears on an engine that advertises it. Scope is kept identical to the existing stack: Hive/Impala/Spark only. Presto/Trino and everything else stay `False`, matching the decision that left `partition_value_transform_default` off Presto. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 2ada8a5)
The "Maps to partition" row rendered the mapped column as a filled `Label` pill with the "Default datetime column" note floating beside it as loose text, so the two values had no shared border and no divider between them. For the implicit case -- where there is a second value to show -- render both values as two segments inside a single outlined, rounded container separated by a vertical divider (`[ event_time | Default datetime column ]`): the column name in monospace on the left, the note in muted secondary text on the right. This follows the existing bordered-container idiom in `PartitionMappingSection` and the `Divider type="vertical"` pattern used elsewhere (e.g. SqlLab's ResultSet), overriding the divider height/margin via a descendant selector. The lock icon and its tooltip stay outside the box, to its right, unchanged. The explicit-mapping case (no second value) keeps the plain single pill, so the segmented treatment only appears when there is a second segment to divide from. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 8e0d1fe)
The label row sat directly under the partition-column description text with only the container's default gap between them. Add 12px of padding above it (theme.sizeUnit * 3) so the mapped-column section reads as its own group. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit c9886cf)
The engine-capability gate was applied to `ColumnCollectionTable` (the PARTITION tag and the per-column mapping row-expand) but NOT to the "Partition column" / "Maps to partition" block in Default Column Settings, which was still gated on the feature flag alone. So on a non-partition-directory engine (e.g. Postgres) with the flag on, that section still rendered even though the per-column UI was correctly hidden. The three-part condition was inlined at the one call site and simply missed at the other -- exactly the kind of drift duplication invites. Extract it into a single `partitionFilterMappingEnabled(datasource)` helper in `PartitionFilterMapping/utils.ts` (next to the other predicates) and use it at both call sites, so a future third site can't diverge. Add regression tests asserting the Default Column Settings partition section (`partition-column-fields`) is present when the engine supports the feature and absent when it does not -- the existing tests only covered the per-column tag. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit bb6158d)
… name
`StyledLabelWrapper` spaced its children with a `span { margin-right }` rule,
but the column name is rendered via `EditableTitle`/`TextControl`, not a plain
span, so nothing guaranteed space before the PARTITION tag and the two rendered
flush against each other. Replace the span-only rule with a flex `gap` on the
container, which spaces every child (certified badge, name control, PARTITION
tag) regardless of what element renders the name.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
(cherry picked from commit fbb4728)
…elect The "Partition column" Select built a rich `customLabel` per option, but this Select ignores `customLabel` entirely -- its closed/selected box and dropdown both render `option.label`, which was the bare `column_name` string, so the type pill never appeared anywhere. Move the rich node into `label` (the field antd renders in both the open dropdown and the closed box, since `optionLabelProp` defaults to `label`), so the selected value now shows the column name in monospace followed by its type pill -- e.g. `dt_epoch [BIGINT]` -- consistent with the dropdown. `value` stays the plain column name, so typed search still matches (`optionFilterProps` defaults to `['label', 'value']`). The column name uses `theme.fontFamilyCode` and the type keeps the existing `Label` pill. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 7dcee6d)
Exposing `supports_partition_filter_mapping` in `DatasetRestApi.show_columns` adds a read-only field to the dataset show schema, so the checked-in spec went stale and `check-openapi-spec-drift` failed. Regenerated with `superset update-api-docs`; the diff matches CI's expected output exactly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit a4668da)
… default The mapped column's "Remove mapping" action clears the override by setting `partition_mapped_column` to null. A null override means "follow `main_dttm_col`", so on a dataset with a default datetime column the mapping moved to that column rather than going away -- silently, under a label that read as switching mapping off. When the row is an explicit override and a default datetime column exists, the action is now labelled "Reset to default datetime column", with a note naming the column the mapping returns to. "Remove mapping" is kept where it is accurate: when there is no default datetime column to fall back to. The default column's own row keeps its label, since there is nowhere to move back to. The comment on `handleRemoveMapping` is corrected to describe both outcomes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit b011140)
…-select test The base made `allColumns` a required prop; this branch's closed-select test predates that and rendered without it, crashing on `columns.find`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 7d26b5e)
`resolve_partition_mapping()` only checked the feature flag, so a mapping stored on an engine without `supports_partition_filter_mapping` -- set before the editor gate existed, imported, or left behind by a database change -- kept mirroring filters with no UI left to inspect or remove it. `partition_mapping_supported()` is the backend twin of the editor's `partitionFilterMappingEnabled`: flag on and the engine advertises support. Both `resolve_partition_mapping()` and `partition_filter_mapping_summary` read it, so the Explore indicator never promises a mirror the query declines. SQLite-backed tests opt in through a `sqlite_supports_partition_filter_mapping` fixture so they keep exercising the mapping rather than the gate; new tests cover the gate itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 1d60edb)
…pping editor tests They predate this branch's engine-capability gate, so after the merge the mapping UI they drive is hidden and the lookups miss. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 7cda2ed)
…se changes The editor read `supports_partition_filter_mapping` and `partition_value_transform_default` off the dataset payload for the database it was loaded with, so switching databases left the editor offering (or hiding) partition mapping according to the engine it had just left. Both are engine properties, so they now ride on the database's `engine_information` (as `supports_schemas` already does), DatabaseSelector carries them on its options, and the editor's database-change handler sets them on the edited datasource. The OpenAPI spec is regenerated for the two new `EngineInformationSchema` fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit a32a179)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 354468d)
… datasource `DatasetObject` carries neither `database` nor the transform default, so assigning them field by field failed tsc in lint-frontend. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit cd39e67)
…ument the gate A stored partition mapping on an engine without `supports_partition_filter_mapping` is ignored with nothing on screen to say so. Log a warning, once per dataset/mapping/engine per process, when the gate drops an existing mapping. Behavior is unchanged. Update the partition filter mapping docs for the engine gate (only Hive and Impala opt in; elsewhere the editor hides the settings and nothing is mirrored) and add an UPDATING.md entry for mappings on other engines that stop applying. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 71b8d5b)
…pported engines - UpdateDatasetCommand only checks the mapping's column references on engines with `supports_partition_filter_mapping`. A mapping kept from before the engine gate whose partition column a sync removed no longer 422s every save on an engine where the editor hides the controls to clear it. Transform safety checks and the stored-expression gate still apply everywhere, since the engine can change with the database connection, without a dataset save. - The default datetime column handler is gated on `partitionFilterMappingEnabled(datasource)`, so re-pointing it on an unsupported engine no longer clears a hidden stored mapping as explicit nulls. - Docs and UPDATING.md list the engines that inherit support from Hive and Impala (Spark SQL, Databricks Interactive Cluster, Ascend) and name the Databricks connectors that do not support it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 9c44b0e)
…e support The base added tests that render the partition mapping UI or resolve a mapping on SQLite/Postgres, neither of which supports it under this branch's engine gate. They now opt in the way this branch's other fixtures do. The test for moving a mapping onto a column with a parked transform now expects the `:value` pre-fill this branch introduced, and still checks that the parked `lower(:value)` is not picked up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit c1a1a3e)
…ine support Master added tests that check how the mirror reads and writes values on Trino, Presto, BigQuery and Oracle. They are about those dialects' literals rather than which engines the gate admits, so they opt in through a fixture that lets every engine spec advertise support; a spec that sets the flag explicitly keeps its own value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 8722033)
Master already carries this entry, reworded, from #45558. The cherry-pick of the gate's documentation commit brought back the older copy from the partition stack's history. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ests The editor only offers partition mapping on an engine that advertises it, so the base's DatasourceEditor mapping tests opt in. The removal test finds the action by its test id, because on a dataset with a default datetime column the action reads "Reset to default datetime column". Carried over from the conflict resolution in ccf35ee (the merge of the partition stack into pfm-7-engine-capability-gate), which has no standalone commit to cherry-pick. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The two database-switch tests only checked that the partition section shows or hides, so they kept passing if DatabaseSelector stopped flattening partition_value_transform_default or onDatabaseChange stopped copying it. Postgres -> Hive now moves the mapping onto a DATETIME column after the switch and asserts it arrives with unix_timestamp(:value); Hive -> Postgres asserts the emitted dataset carries neither Hive's capability nor its default. Both fail with the default left stale. Requested in review of #44431. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"Reset to default datetime column" and "The mapping returns to the default datetime column (%(column)s)." come from the clear-mapping label fix and were not yet in messages.pot or the language catalogs. Purely additive: one untranslated entry per string per catalog. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
i18n
github-actions
Bot
added
i18n:russian
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+591
to
+602
| issues = ( | ||
| validate_partition_mapping( | ||
| column_names=column_names, | ||
| partition_column=partition_column, | ||
| partition_mapped_column=partition_mapped_column, | ||
| main_dttm_col=main_dttm_col, | ||
| transform=transform, | ||
| engine=database.backend, | ||
| ) | ||
| if database.db_engine_spec.supports_partition_filter_mapping | ||
| else validate_transform(transform, database.backend) | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggestion: This engine gate only affects REST updates; the deprecated save route still validates references unconditionally and returns 422 for dangling mappings on unsupported engines.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Api mismatch
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dataset/update.py
**Line:** 591:602
**Comment:**
*Api Mismatch: This engine gate only affects REST updates; the deprecated save route still validates references unconditionally and returns 422 for dangling mappings on unsupported engines.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+1345
to
+1353
| setDatasource(prev => ({ | ||
| ...prev, | ||
| database: db, | ||
| supports_partition_filter_mapping: Boolean( | ||
| db.supports_partition_filter_mapping, | ||
| ), | ||
| partition_value_transform_default: | ||
| db.partition_value_transform_default ?? null, | ||
| })); |
Contributor
There was a problem hiding this comment.
Suggestion: Switching databases retains the old catalog, schema, and physical table_name through ...prev; saving can target stale objects in the newly selected database.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Stale reference
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
**Line:** 1345:1353
**Comment:**
*Stale Reference: Switching databases retains the old `catalog`, `schema`, and physical `table_name` through `...prev`; saving can target stale objects in the newly selected database.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #45929 +/- ##
==========================================
+ Coverage 84.76% 84.78% +0.01%
==========================================
Files 3072 3072
Lines 206119 206220 +101
Branches 48539 48568 +29
==========================================
+ Hits 174718 174835 +117
+ Misses 28101 28084 -17
- Partials 3300 3301 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Supersedes #44431 by @eschutho, rebuilt on current
masterso it can merge. Her 17 commits are cherry-picked with authorship and trailers intact, and all credit for the gate goes to her.With
PARTITION_FILTER_MAPPINGon, every database showed the partition mapping controls and mirrored filters at query time. That includes engines where nothing prunes on a partition column. This PR adds asupports_partition_filter_mappingengine capability, set on Hive and Impala and inherited by Spark SQL, Databricks Interactive Cluster and Ascend. It then applies that capability in four places:resolve_partition_mappingdeclines a stored mapping on an unsupported engine, logs it once, and the Explore glyph agrees.UPDATING.md: both describe the behaviour change.Changes from #44431:
master: the earlier partition-stack history, which landed via feat(datasets): partition filter mapping #43870, and the column-flag half of1f08a34700, whichmasteralready enforces.:valuefallback default and its auto-ticked "preserves ordering" flow (the other half of1f08a34700, plus76fec9ba78from the old branch).master's default-selection rules stay as they are.unix_timestamp(:value), and Hive → Postgres asserts the emitted capability and default are cleared. Both fail if the switch leaves the default stale.masteradded since fix(datasets): gate partition filter mapping UI by engine capability #44431 branched.UPDATING.mdentry.Separate from #45846, #45848, #45833 and #45851, which are left untouched.
Open question for review: the gate admits only Hive and Impala and the specs that inherit from them. On any other engine, including Postgres, Oracle, Trino and BigQuery, existing mappings stop mirroring after upgrade, as
UPDATING.mddescribes. Widening the list is a one-line change per spec if reviewers want it.TESTING INSTRUCTIONS
PARTITION_FILTER_MAPPING.pytest tests/unit_tests/connectors/sqla/partition_mapping_test.py tests/unit_tests/models/partition_mirroring_test.py tests/unit_tests/datasets tests/unit_tests/commands/dataset tests/unit_tests/db_engine_specsnpm run test -- src/components/Datasource src/components/DatabaseSelectorADDITIONAL INFORMATION
PARTITION_FILTER_MAPPINGCreated with Claude Opus 5.5 in Claude Code.