Repository navigation
Conversation
|
Bito Automatic Review Skipped - Branch Excluded |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
949b0af to
413e5c6
Compare
e51df8a to
240191d
Compare
413e5c6 to
6142a9e
Compare
240191d to
de8d5b7
Compare
6142a9e to
eeb4218
Compare
de8d5b7 to
42d68c0
Compare
eeb4218 to
2e57d43
Compare
|
The flagged issue is correct: the To resolve this, you should ensure the indicator only renders when the Proposed FixUpdate // In OptionControlLabel component
{partitionMapping?.active && (
<PartitionPruningIndicator mapping={partitionMapping} />
)}This ensures the glyph only appears when the mapping is active, preventing misleading indicators for inactive or unrelated filters. Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset-frontend/src/explore/components/controls/OptionControls/index.tsx |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43870 +/- ##
===========================================
+ Coverage 65.62% 84.49% +18.87%
===========================================
Files 3059 3067 +8
Lines 202434 204522 +2088
Branches 47448 48054 +606
===========================================
+ Hits 132852 172821 +39969
+ Misses 66587 28399 -38188
- Partials 2995 3302 +307
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:
|
42d68c0 to
0118629
Compare
2e57d43 to
6132109
Compare
0118629 to
e68a360
Compare
6132109 to
63316d4
Compare
Regenerated tests/unit_tests/mcp_service/native_tool_inventory.json with SUPERSET_MCP_UPDATE_TOOL_INVENTORY=1 under master's new fastmcp 4.1.2 / mcp 2.3.0 pins rather than picking a side; against master only the dataset tools (the two partition fields) and the totals differ. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
aminghadersohi
left a comment
There was a problem hiding this comment.
One public-hygiene nit.
Nit: the test_a_postgres_array_partition_column_is_refused docstring (tests/unit_tests/connectors/sqla/partition_mapping_test.py:3740) still opens with an internal ticket id; drop it. Not inline: GitHub returns no patch for this file.
| statement = _parse_skeleton(transform, engine, _PLACEHOLDER_STANDIN) | ||
| if statement is None: | ||
| return False | ||
| return statement.count_bare_column_references(_PLACEHOLDER_STANDIN) == expected |
There was a problem hiding this comment.
The gate counts a :value that sits inside a quoted identifier as a bare column reference, because the stand-in becomes "superset_pfm_value_standin" and parses as one exp.Column. At probe time the value is bound inside those identifier quotes, so a sample value containing " can close the identifier and add its own expression. On DuckDB, a transform like list_extract(list_transform([0], lambda "'x", "y'": ":value"), 1) with a sample value of x" + nextval(chr(115)) + "y compiles to a single-projection probe with no comment or subquery, so the final shape check accepts it and nextval advances a sequence even though writing nextval directly is refused. A dataset editor without SQL Lab access can reach this through the preview endpoint. Should a placeholder inside a delimited identifier be rejected here, the same way one inside a string literal or comment is?
| return False | ||
| if generic_type != utils.GenericDataType.STRING: | ||
| return True | ||
| return bool(db_engine_spec.binary_string_comparison) |
There was a problem hiding this comment.
A PostgreSQL JSONB column maps to GenericDataType.STRING, so this returns the engine's binary_string_comparison (true) and equality mirroring is allowed. JSONB does not keep input spelling: the stored value {"a":1} reads back as {"a": 1}. With mapped column j JSONB, partition column p TEXT, and transform CAST(:value AS TEXT), a filter j == '{"a":1}' matches the row, but the probe renders the compact input and adds p = '{"a":1}' OR p IS NULL, which excludes the row whose p is {"a": 1}. Should JSON/JSONB decline here like the padded character types do?
| if start_dttm is not None: | ||
| start_dttm -= widen_bounds_by | ||
| if end_dttm is not None: | ||
| end_dttm += widen_bounds_by |
There was a problem hiding this comment.
end_dttm += widen_bounds_by is outside any handler, and adding a bucket width to a datetime near the maximum raises ValueError. A mapped TEMPORAL_RANGE filter with grain P1Y and a range ending 9999-12-31 builds valid bounds for the original predicate, then fails here on year 10000. The comment above says a widened range fails open, but this raises instead, so turning mapping on breaks a chart that ran fine without it. Should the widening skip the mirror on overflow rather than propagate?
|
|
||
| // A lower-bound total only covers the rows up to the current page, so count | ||
| // one row past it to keep the next page reachable. | ||
| const recordCount = |
There was a problem hiding this comment.
recordCount is derived only from the page currently shown. With 120 matching rows and 50-row pages, visiting page 3 caches an exact total of 120, but going back to page 1 uses that page's cached lower bound (50), so recordCount becomes 51. Page 3 disappears from the pager and the label reverts to "50+ rows" even though the exact total is already known. Should the highest known bound, or the exact total once seen, be kept separately from the displayed page?
| }, | ||
| ) | ||
| : t( | ||
| 'Equality and list filters on %(mapped)s will automatically apply an equivalent filter to %(partition)s. Time ranges and other range filters are not mirrored until "Transform preserves ordering" is on.', |
There was a problem hiding this comment.
This message says equality and list filters on the mapped column will be mirrored, but nothing here checks that those operators are allowed. Take a MySQL dataset mapping a VARCHAR through lower(:value) with ordering unchecked: mappingIsActive() accepts it, while the server's equality_mirrors_safely() rejects equality and IN, so no operator is mirrorable. The owner is told the mapping works, yet every filter stays unpruned. Could the help text use the server's operator eligibility instead of assuming equality is always available?
| ); | ||
| // Both sides are passed, in that order, and before the columns are set. | ||
| expect(src).toMatch( | ||
| /clearUnmappedTransformsAcrossMove\(\s*liveDatasource,\s*syncedDatasource,\s*columnChanges\.finalColumns,?\s*\)[\s\S]{0,400}?setColumns\(\{/, |
There was a problem hiding this comment.
These sync tests read DatasourceEditor.tsx as text and match regexes, so they pin the source's shape rather than its behavior. If syncMetadata cleared unmapped transforms unconditionally, a sync with PARTITION_FILTER_MAPPING off would silently drop a stored transform and its monotonicity flag, and these assertions would stay green. The existing flag-off test covers changing the default datetime column, not syncing. Could a render test cover this: flag off, a stored non-mapped transform, a mocked fetchSyncedColumns resolved, and an assertion that the resulting onChange payload keeps both fields?
Two conflicts, both additive: - superset/datasets/schemas.py: keep FilterOperator (partition mapping preview) and master's is_valid_d3_format import (#45633). - superset/models/helpers.py: the series-limit subquery keeps the branch's inner-window partition mirrors in its WHERE and takes master's _groupby_clause_exprs for its GROUP BY (#45271). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
geido
left a comment
There was a problem hiding this comment.
Ran this end to end on Postgres and Trino with the flag on and off, against a latest-master baseline. Left three notes inline with recordings; the Trino one is what I'd want fixed before customers turn the flag on.
| # have. Trino has no VARCHAR-to-DATE comparison and BigQuery coerces only a | ||
| # canonical ``YYYY-MM-DD`` *literal*, so on those even plain ISO text fails | ||
| # the whole chart rather than merely losing the pruning -- which is why the | ||
| # ISO forms are declared here too rather than assumed universal. | ||
| # | ||
| # Empty by default, so an engine Superset has not confirmed declines the | ||
| # mirror and keeps the query working. Declared on concrete specs, not on a | ||
| # shared base: `PostgresBaseEngineSpec` is inherited by Snowflake, whose | ||
| # AUTO date input reads an all-digit string as epoch seconds instead. | ||
| temporal_literal_formats: tuple[str, ...] = () |
There was a problem hiding this comment.
IIUC this guard only covers str results, while a date/datetime result is written by the Trino dialect as the same bare string. On Trino, date(CAST(:value AS TIMESTAMP)) onto a DATE dt previews as valid, then the chart fails with "Cannot apply operator: date <= varchar(10)" (=/IN too). Could these get the _reads_as_temporal check, or a typed literal, so only pruning is lost?
There was a problem hiding this comment.
Good catch, thanks. Fixed in 5615540384. A probed date/datetime is now written with the engine's own literal via convert_dttm (the same path as the chart's own time bounds), so on Trino this emits dt >= DATE '2026-01-01' for ranges, = and IN. Where the engine has no faithful literal for the column, the mirror is declined and only pruning is lost. Added dialect-level tests and a preview test.
|
|
||
| const columnChanges = updateColumns( | ||
| datasource.columns, | ||
| liveColumns, |
There was a problem hiding this comment.
I think merging against liveColumns has a side effect with the flag off. If I delete a column row that still exists in the source and then sync, it comes back with a nanoid() id, so the save fails with "One or more columns already exist" (master reused the old row and saved fine). Could the merge fall back to datasource.columns for names missing from liveColumns?
There was a problem hiding this comment.
Thanks, reproduced. Fixed in e8935b2abe. The merge still runs against the live rows, but a column that comes back as "added" and exists in datasource.columns is restored from its saved row (taking the source's type), so the save updates it instead of re-creating the name. Added a test for delete → sync → save.
| {popoverContent} | ||
| {/* The glyph sits outside the popover trigger so clicking it opens the | ||
| tooltip rather than the time-range editor. */} | ||
| <Flex align="center" gap={theme.sizeUnit}> |
There was a problem hiding this comment.
I think this Flex changes the layout with the flag off too. The trigger span becomes a flex item, so the Time Range box in the ad-hoc filter popover shrinks to its label instead of filling the popover like on master (250px vs 320px here). Would rendering the Flex only when partitionMapping is set keep the old markup?
There was a problem hiding this comment.
Agreed. Fixed in f94ce2fe3c. The Flex is now rendered only when partitionMapping is set, so the flag-off markup matches master. With a mapping, the trigger also gets flex: 1; min-width: 0, so the box fills the row instead of shrinking. Added tests for both cases.
- Translations: master re-synced every catalog (#45583), so the whole set conflicted. Took master's catalogs and re-ran scripts/translations/babel_update.sh on a tracked-files snapshot of the merged tree; against master the only changes are this branch's partition-mapping msgids and each file's POT-Creation-Date. - tests/unit_tests/dashboards/datasets_test.py: both sides appended tests; kept both. Master's new pin on DASHBOARD_DATASET_INACCESSIBLE_FIELDS now lists the three partition fields this branch redacts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A transform that answers with a DATE or TIMESTAMP was emitted through SQLAlchemy's literal processor, which writes it as plain quoted text on every dialect: `part >= '2026-01-01'`. Trino and Presto have no implicit varchar-to-date/timestamp coercion and refuse that comparison, and because the mirror is ANDed onto the chart query the whole chart failed while the preview reported the mapping as valid. Probed dates and timestamps are now written with the engine spec's `convert_dttm`, as every other temporal bound in a chart query is (`DATE '2026-01-01'` on Trino, `CAST(... AS DATE)` on Hive). The mirror is declined, and the preview says why, where the spec has no literal for the column's type or its literal would drop part of the value (SQLite floors to the second, Presto to the millisecond) -- a floored upper bound is a mirror narrower than the filter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 75b0ac0)
…finds it Merging a sync against the live columns meant a row deleted this session but still in the source came back as "added" with a fresh nanoid id, and the save failed with "One or more columns already exist". Master merged against the saved columns and reused the old row. A re-added name now gets its saved row back (with the source's type), as in #45560, so the save updates it. Applies with the flag off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pped The Flex wrapper around the time-range trigger and the partition glyph made the trigger a flex item on every dataset, so with the flag off the Time Range box in the ad-hoc filter popover shrank to its label (250px against 320px on master). The wrapper is now rendered only when the dataset has a partition mapping, and inside it the trigger fills the row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The column-sync fix in e8935b2 renamed the list the partition-mapping cleanup reads from columnChanges.finalColumns to syncedColumns. The source-pin test matched the old name, so its positive check failed and its negative check passed trivially; both now name syncedColumns. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
geido
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround, the Trino, sync and Time Range fixes all check out here. I tried Oracle against a real server and found two places where the mirror quietly drops rows, notes and recordings inline.
| class OracleEngineSpec(BaseEngineSpec): | ||
| engine = "oracle" | ||
| engine_name = "Oracle" | ||
| select_without_from_suffix = " FROM DUAL" |
There was a problem hiding this comment.
I think the new temporal literal path drops the time on Oracle. Its DATE keeps the time of day but maps to sa.Date, so holds_days_only skips the precision check and the bound becomes TO_DATE('2026-01-12', 'YYYY-MM-DD'). With CAST(:value AS DATE) (preview Valid), a 06:30 to 18:30 range returns 0 rows instead of 48. Could the exemption skip Oracle, whose dialect DATE is a DateTime?
There was a problem hiding this comment.
Good catch. Fixed in 6f37942f48: whether a key holds only days is now read from the database's own dialect, which makes Oracle's DATE a DateTime, so the precision check runs and a time-of-day bound is declined (an exact midnight still mirrors).
| ), | ||
| log_to_statsd=False, | ||
| ) | ||
| def partition_mapping_preview(self, pk: int) -> Response: |
There was a problem hiding this comment.
Not sure, but on Oracle the preview and the chart pass :value as different types: the preview sends a string, a chart's time range sends a typed TO_DATE(...). So TO_DATE(:value, 'YYYY-MM-DD HH24:MI:SS') previews Valid, yet with the default NLS_DATE_FORMAT the bounds land in year 0012 and the chart returns 0 rows. Could the preview probe with the same typed values the chart path uses?
There was a problem hiding this comment.
Fixed in 6f37942f48: a temporal sample is now previewed as a time range sends it (the typed literal), the text shape is probed too, and the preview refuses a transform whose two answers differ, showing both predicates.
There was a problem hiding this comment.
Thanks! The midnight sample is refused now, but the editor previews a TIMESTAMP column at 10:47:23, where the text probe is declined first, so it still says Valid. Charts don't run the check: set up in the editor, 06:30-18:30 returns 0 rows vs 48 with the flag off, and with NLS_DATE_FORMAT='YYYY-MM-DD' the preview looks right but totals are short. Could the chart path compare both shapes too?

mp4 · NLS_DATE_FORMAT='YYYY-MM-DD': preview looks right, 482 vs 554 rows (mp4)
There was a problem hiding this comment.
Fixed in 83f0660f2a: the check moved into build_mirrored_predicates, so charts run it too. Each date/timestamp is also asked as text in the same probe (still one round trip), bypassing the gates that declined it at 10:47:23, and a disagreement drops the mirror.
…migration The popover test added in #44843 asserted that a config stored under the display alias `Time` is ignored. #44955 deliberately reads legacy `Time`-keyed configs and migrates them onto `__timestamp`, so the assertion is stale. Assert the intended behaviour instead: the legacy config is read back and an edit is saved under `__timestamp`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 34d5617)
…er range
- An Oracle DATE keeps the time of day, but Superset maps it to sa.Date, so
the days-only exemption in the temporal literal path skipped the
precision check and wrote 06:30 as TO_DATE('2026-01-12', 'YYYY-MM-DD').
Whether a key holds only days is now read from the database's own
dialect, which makes Oracle's DATE a DateTime; the mirror is declined
where the literal would drop the time, and an exact midnight still
mirrors.
- A chart hands the transform a temporal value as text (simple filters) or
as a datetime the dialect writes as a typed literal (time ranges). The
preview probed only the text, so TO_DATE(:value, fmt) previewed valid on
Oracle while every time range landed in year 0012. A temporal sample is
now previewed as a time range sends it, the text shape is probed too, and
the preview refuses a transform whose two answers differ.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hard Each case mounts the whole dataset modal, edits two collections and saves. That takes about 8s locally, the same on this branch and on master, but it crossed the global 20s testTimeout on loaded CI shards. Raise it for this file to 60s, as other heavy modal suites do with jest.setTimeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| ): | ||
| # The format is unreachable in `dttm_sql_literal` for this column, | ||
| # so reproducing it here would describe a different bound. | ||
| return dttm |
There was a problem hiding this comment.
When the engine answers convert_dttm for a DATE column, the filter compares against the date-only literal ('2026-01-01' on SQLite), but this returns the widened datetime, which the probe binds as '2026-01-01 00:00:00.000000'. With a DATE column event_date, a text partition key p = CAST(event_date AS TEXT) and a monotonic CAST(:value AS TEXT) transform, a January range mirrors as p >= '2026-01-01 00:00:00.000000', which sorts above the key '2026-01-01' and drops every January 1 row the filter keeps. Could the probe use the same rendering the engine gives the original bound (for example the date itself when the literal carries no time of day)?
| - escaping | ||
| - transform_escaping | ||
| - engine | ||
| - type |
There was a problem hiding this comment.
_preview_probe() also returns HTTP 200 with reason: "shape" when the range and text probes emit different predicates (for example an Oracle TO_DATE(:value, 'YYYY-MM-DD HH24:MI:SS') transform on a temporal sample), but shape is not in this enum, the generated openapi.json entry, or the TypeScript union. A client generated from the spec with a strict enum would reject that response. Could shape be added to all three and the spec regenerated?
| enforced on every write rather than only inside `UpdateDatasetCommand`. A | ||
| transform arriving on any other column is dropped -- by the deprecated | ||
| `POST /datasource/save/`, and by `PUT /api/v1/dataset/<pk>` and dataset import | ||
| even when `PARTITION_FILTER_MAPPING` is off. An API client that was storing one there |
There was a problem hiding this comment.
This says a transform sent on a non-mapped column is dropped and the field reads null afterwards, including with the flag off, and partition-filter-mapping.mdx repeats it. But drop_unmapped_value_transforms() deliberately keeps an unchanged echo of the stored value (stored_by_name), which is exactly what a GET-then-PUT, a legacy save, or an overwrite import sends. An operator who follows this note to clear a parked transform by re-saving the dataset will still find it there. Could both places state the unchanged-echo exception and say how to clear it (send null)?
geido
left a comment
There was a problem hiding this comment.
Thanks for the Oracle fixes, CAST(:value AS DATE) checks out here. I did a last pass on 81c5e54 against six engines: with the flag off I found no regressions against master, and with it on I found one new issue on ClickHouse (inline), while the TO_DATE thread still reproduces from the editor.
| seen.add(key) | ||
| deduped.append((operator, value)) | ||
|
|
||
| return build_mirrored_predicates(cast("SqlaTable", self), mapping, deduped) |
There was a problem hiding this comment.
I think this can now fail every chart filtering the mapped column on ClickHouse. _holds_days_only (from 6f37942) calls .get on dialect.ischema_names, a list in clickhouse-connect 1.10/1.11, so toDate(:value) onto a Date key returns 500 with the flag on (200 off) and the preview shows "Fatal error". Could the lookup accept only a Mapping, and this call fail open, as the docs say?
There was a problem hiding this comment.
Thanks, my bug. Fixed in 83f0660f2a: the lookup only reads a Mapping and treats any failure as "may hold a time of day", and the chart path (and preview) now fail open on any error building the mirror.
…ral shapes on charts - _holds_days_only read dialect.ischema_names with .get, but clickhouse-connect 1.10/1.11 make it a list, so every chart filtering the mapped column returned 500 and the preview showed a fatal error. It now reads only a Mapping and treats any lookup failure as "may hold a time of day". The chart path's mirror build and the preview also fail open: an unexpected error costs the pruning, not the chart. - The text-vs-timestamp check moves from the preview into build_mirrored_predicates, so charts run it too. evaluate_transform takes a text_answers sink and asks each date or timestamp as text in the same round trip (one probe per query still), retrying with the typed values alone if the text is refused. The text bypasses the mirror request gates, which declined it before the comparison on a TIMESTAMP column at a time of day, so the preview said valid for Oracle's TO_DATE(:value, fmt). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| continue | ||
| # `bool` is checked separately because it satisfies every numeric | ||
| # `isinstance` there is while rendering as `true`. | ||
| held = isinstance(value, allowed) and ( |
There was a problem hiding this comment.
A probe result of inf or nan passes this type check, so a transform such as CASE WHEN :value = 0 THEN CAST('Infinity' AS DOUBLE PRECISION) ELSE :value END on a PostgreSQL double partition column returns inf for n = 0. The mirrored predicate is then rendered as p = inf, which the database rejects (column "inf" does not exist), while preview still reports the mapping as valid, so the chart fails instead of just losing pruning. Should a non-finite float be treated as not held here so the mirror declines?
| app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"] = False | ||
|
|
||
| with app.app_context(): | ||
| with patch(PROBE, side_effect=AssertionError("probe must not run")): |
There was a problem hiding this comment.
Since the mirror builder now swallows any exception and returns [], the AssertionError raised by this booby-trapped probe is caught too, so the test passes whether or not the probe runs. If the chart path regressed and probed with the flag off, "dt_epoch" not in sql would still hold. Could this bind the patch (as probe) and assert probe.assert_not_called() after _query()? The same pattern is at the other side_effect=AssertionError(...) tests in this file.
aminghadersohi
left a comment
There was a problem hiding this comment.
The new typed-vs-text probe check leaves the stored mirror verdict wrong in both directions.
| if isinstance(value, date) and not is_unmirrorable(value) | ||
| ] | ||
| if indexes: | ||
| combined = evaluate_transform( |
There was a problem hiding this comment.
On Oracle, CAST(:value AS DATE) and TRUNC(:value, 'HH') evaluate a typed bound but fail on its text form (ORA-01861, ORA-01722), so every time-range chart reruns this failing probe (failures are not cached) and stores evaluable=false, hiding the glyph while queries still prune.
| if not _bounds_are_ordered(evaluated, spans): | ||
| return [] | ||
|
|
||
| if _declined_for_reading_by_type(evaluated, text_answers, shape_errors): |
There was a problem hiding this comment.
When this declines (Oracle TO_DATE(:value, fmt) puts the typed bound in year 0012), the stored verdict stays evaluable=true, so the time-range glyph keeps promising pruning that no time-range query gets.
sha174n
left a comment
There was a problem hiding this comment.
LGTM — the mirror is a strictly-narrowing redundant predicate; round-trip checks on the probe look thorough.
- DrillDetailPane.test.tsx and tests/unit_tests/views/datasource/test_utils.py: both sides appended tests (this branch's lower-bound Samples paging, master's sunburst chart-type tests from #45571); kept both. - superset/views/datasource/utils.py: #45571 retyped get_samples' payload from SamplesPayloadSchema to dict[str, Any] and dropped the import; the branch's _count_star_query_context now takes the same type, and the test that needed a type: ignore for the old one no longer does. - Translations: took master's catalogs and re-ran scripts/translations/babel_update.sh on a tracked-files snapshot of the merged tree; against master only this branch's msgids and each file's POT-Creation-Date change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| parsed = make_url_safe(db_obj.sqlalchemy_uri) | ||
| if parsed.password and parsed.password != PASSWORD_MASK: | ||
| db_obj.set_sqlalchemy_uri(db_obj.sqlalchemy_uri) | ||
| imported.extend(db_obj.tables or []) |
There was a problem hiding this comment.
db_obj.tables is every dataset attached to the database, not only the ones this bundle imported, so _repair_partition_mappings() also runs on unrelated datasets. Importing a legacy bundle that contains only dataset B therefore sends dataset A through clear_unmapped_partition_transforms, which drops A's parked transform and monotonicity declaration (for example one saved while the feature flag was off). Should this collect only the datasets the import actually wrote, rather than expanding the database relationship?
| return True | ||
|
|
||
| rendered = _compile_literal(sa.literal(value), dialect) | ||
| if rendered == f"'{value}'" and "'" not in value and "\\" not in value: |
There was a problem hiding this comment.
A probe result containing a NUL passes this short-circuit, since it has no quote or backslash and renders as '<value>' unchanged. On SQLite, mapping text column c to text partition column p with :value || char(0) returns 'a\x00' for c = 'a'; the guard accepts it, and the mirrored statement then fails with ProgrammingError: the query contains a null character, so the chart errors instead of just losing pruning. Should a value containing \x00 be treated as not round-tripping here?
| transform, | ||
| values + [_as_filter_text(values[index]) for index in indexes], | ||
| ) | ||
| if combined is not None: |
There was a problem hiding this comment.
I think a transform that refuses the text still slips through here. TO_DATE(:value, 'YYYY-MM-DD') onto a day key raises ORA-01830 on '2026-01-12 06:30:00', so this falls back to the typed answer (year 0012) unchecked: the preview says Valid and the range returns 0 rows (48 with the flag off), Last week too. Could a refused text form decline the temporal mirror instead?







SUMMARY
Adds partition filter mapping: a dataset owner names one partition column and one business column whose filters are mirrored onto it, and every query then carries an equivalent predicate on the partition column, so chart authors change nothing and queries prune. Behind
PARTITION_FILTER_MAPPING, off by default.Datasets on Hadoop-family engines are commonly partitioned on a technical column — an epoch integer, a lowercased region key — that no analyst would filter on. Unless a query carries a predicate on that column the engine scans every partition, and the only workaround today is hand-writing the predicate as custom SQL in a virtual dataset, which pushes a performance concern onto every chart author and takes the dataset out of the physical/syncable path.
Storage
Four columns, following the
always_filter_main_dttm/currency_code_columnprecedent for "a dataset-level setting that names a column":tables.partition_columntables.partition_mapped_columnNULLfollowsmain_dttm_coltable_columns.partition_value_transform:valueexpressiontable_columns.partition_transform_is_monotonicThe effective mapped column is
partition_mapped_column or main_dttm_col, so "re-pointing the default datetime column moves the mapping with it, unless overridden" falls out of the model rather than needing code. A JSON blob insidetables.extrawas considered and rejected:extrais a user-editable free-text box,buildExtraJsonObjectrebuildscolumn.extrafrom a hardcoded key list on every save, and there is no validation layer for it. (#44816 explored that route and is closed; if the migration is the blocker, that is the conversation to reopen, not this storage choice.)Operator safety is the heart of the query rewrite
A mirrored predicate
P2may only beAND-ed onto a query when the originalP1implies it. Getting this wrong is silent — it shows up as wrong chart numbers, not an error.col = v,col IN (…)Tis a functioncol >=,>,<,<=,TEMPORAL_RANGETis monotoniccol != v,NOT IN,LIKE,IS NULL, …Negations are never safe because
Tneed not be injective:lower(:value)withcountry != 'US'mirrors toregion_key != 'us', excluding rows whosecountryis already lowercase'us'— rows the original filter keeps.Monotonicity is declared, not inferred. An earlier draft inferred it from the column type; that is wrong on partition schemes people actually use —
hour(:value),date_format(:value, 'dd')anddayofweek(:value)are all reasonable transforms on aTIMESTAMPand none preserve ordering.T(lower) <= T(upper)is asserted at runtime as a backstop.Three more properties the rewrite holds to:
NULLpartition value isNULL, so the emitted clause is(preds) OR partition_col IS NULL— Hive and Impala park rows with a NULL key in__HIVE_DEFAULT_PARTITION__, and those rows must survive. A grained range compares the truncated column, so its raw bounds do not describe the rows it keeps; those mirror by widening both bounds one bucket, and grained=/INdo not mirror at all.get_time_filterapplied the dataset timezone and legacy hour offset internally; that is extracted intoadjust_time_boundsrather than duplicated.PARTITION_FILTER_MAPPINGmust be a static boolean.FEATURE_FLAGSalso accepts per-request callables, and since the mapping only enters the cache key when active, a flag resolving per user would let a flag-off user read a cache entry written from pruned SQL by a flag-on user.The dataset editor
A Partition column select in the Columns tab's Default Column Settings, plus a read-only computed Maps to partition — deliberately not a peer dropdown, since it reflects
partition_mapped_column ?? main_dttm_coland a second select would let it drift silently. The partition column's row is muted, carries aPARTITIONtag, and defaultsIs filterable/Is dimensionoff. A Partition filter mapping section in the row expand has three states: the mapped column holds the transform, every other column offers to take the mapping over, the partition column itself shows nothing.A live Preview panel shows sample input → emitted predicate, backed by
POST /api/v1/dataset/<pk>/partition_mapping/preview/:It fires a real warehouse query from a text input, so the order of its guards is the point: validation and the stored-expression policy run before anything reaches the engine (so a half-typed transform costs zero queries, and a transform cannot smuggle a subquery past the gate that governs every other stored expression); it shares the evaluator and probe cache with the query path, so preview and runtime cannot drift; and it is rate-limited per user and dataset in a fixed window, because client debouncing is a courtesy, not a guard.
Two places the spec and the backend disagreed, resolved in the UI's favour:
dt_epochbounds) was unreachable. Hence Transform preserves ordering.unix_timestamp(:value)as the temporal default, but that is Hive syntax and would not parse on Postgres, Trino or BigQuery. The default moves to the engine spec (partition_value_transform_default); engines without one offer no pre-fill rather than a wrong one.The Explore indicator
A small glyph on any filter whose column the dataset mirrors, with the tooltip "This filter is also applied to a partition column for faster queries. See 'View query' for the generated SQL." Chart authors configure nothing and ideally never learn the word "partition"; the indicator exists only to explain the speed-up and point at the SQL.
It shows only where the query path really does mirror — it checks the operator against the set the server computes from declared monotonicity, and excludes grained filters, the
<NULL>sentinel, and a time range with no temporal column selected. The one gap is deliberate and commented: a virtual dataset's Jinjaget_filters(..., remove_filter=True)makes the backend skip both the filter and its mirror, butremoved_filtersis built during query generation and is not inform_data, so the browser cannot know. The glyph is advisory; "View query" is the authority.partitionColumnalready exists on these components as the unrelated Prestolatest_partitionfeature, so the prop ispartitionMappingthroughout — two meanings of one word, one prop apart, is not worth the saving.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
See the screenshots on the closed PRs, which remain valid: the editor controls and preview panel on #43891, and the indicator on #43870 —
02-adhoc-operator-matrix.pngthere is the clearest single frame, one chart carrying eight filters with the glyph on exactly the two that mirror.TESTING INSTRUCTIONS
pytest tests/unit_tests/connectors/sqla/partition_mapping_test.py \ tests/unit_tests/models/partition_mirroring_test.py \ tests/unit_tests/datasets/partition_mapping_preview_test.py \ tests/unit_tests/datasets/partition_mapping_serialization_test.py \ tests/unit_tests/dao/dataset_test.py \ tests/unit_tests/commands/dataset/ \ tests/unit_tests/datasets/commands/importers/v1/import_test.py \ tests/unit_tests/sql/parse_tests.py cd superset-frontend && npm run test -- \ src/components/Datasource \ src/explore/components/PartitionPruningIndicator \ packages/superset-ui-chart-controls/test/shared-controlsManual, in a dev stack with
PARTITION_FILTER_MAPPING: True, against a physical table partitioned on a technical column:PARTITIONtag, and drops out of Explore's dimension and filter pickers.unix_timestamp(:value) and confirm it reports a parse error without hitting the warehouse.!=on the mapped column) and confirm no glyph and no mirrored predicate.ADDITIONAL INFORMATION
PARTITION_FILTER_MAPPING(off by default)ADD COLUMNs with no backfill and no index, plus the matching Continuum shadow columns so dataset version history and restore keep working; effectively instant on Postgres/MySQL, no downtime expected