hughhhh opened a new pull request, #44816:
URL: https://github.com/apache/superset/pull/44816
### SUMMARY
Consolidates the four-PR partition-filter-mapping stack (#43757, #43758,
#43759, #43760) into one PR, and **replaces its four new database columns with
a storage interface, so the feature ships with no Alembic migration.** The
migration was the only thing blocking deployment, and it gated the other three
PRs behind it.
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.
A dataset owner names one partition column, one business column whose
filters are mirrored onto it, a value transform (a SQL expression containing
`:value`), and whether that transform preserves ordering. Superset then appends
an equivalent predicate on the partition column to every query: chart authors
change nothing and queries prune. Behind `PARTITION_FILTER_MAPPING`, off by
default.
#### The storage interface
This follows the pattern already two screens away in
`superset/datasets/api.py`, which exposes four properties over `tables.extra`
(`is_certified`, `certified_by`, `certification_details`, `warning_markdown`)
through `show_columns`, with a comment saying why. Here the same blob carries
`partition_filter_mapping`.
Behind `PartitionMappingStore` — `load`/`save`, one adapter today
(`ExtraJsonPartitionMappingStore`) and one that activates with the migration,
selected by `PARTITION_MAPPING_STORE`. The model, the DAO, validation, the
query rewriter and the preview endpoint are written against the port and never
learn which store answered. A store-swap test runs the behavioural surface
against a second, dict-backed implementation, so "we can move this" is a claim
with evidence rather than an intention.
`tables.extra` turned out to be a reasonable home rather than only an
expedient one:
- it is already in `SqlaTable.export_fields` and already one of
`ExportDatasetsCommand`'s `JSON_KEYS`, so a mapping travels through
**import/export with no bundle-format change**;
- Continuum already versions the column, so **dataset version history and
restore work untouched** — and no shadow-table migration is needed, which is
the part the repo documents as unreliable to autogenerate;
- the dataset editor round-trips dataset-level `extra` verbatim, and its one
mutating path (`setDatasetCertification`) merges rather than rebuilds, so an
unknown key survives a save.
It has to be the *dataset*-level blob, though, never `table_columns.extra` —
`buildExtraJsonObject` rebuilds every column's and metric's `extra` from two
keys on every editor save.
#### Two details worth a look
**Transforms are keyed by column name, not flattened onto the mapping.**
Flattening is unsound: an owner who declares a transform on `A` (then
`main_dttm_col`) and later re-points `main_dttm_col` to `B` would have `B`'s
values mirrored through `A`'s expression. Keyed, `B` has no transform and the
mapping goes inactive — which is exactly what four real columns would do. That
parity is what makes the later migration a mechanical per-column copy.
**The model's four attributes are read-only properties, and `DatasetDAO`
extracts the mapping from a payload before anything is applied and writes it
after everything is.** The editor's PUT carries both the typed
`partition_column` field *and* a verbatim copy of `extra`, and under this store
those are the same bytes — so a `setattr` loop that happens to apply `extra`
last silently discards the typed field, and which order that is depends on
marshmallow's field ordering, not on anything visible at the call site.
Read-only properties make that loss impossible rather than merely unlikely;
`set_partition_mapping` is the one supported write. There is a test for each
assignment raising, so nobody reintroduces setters later.
#### Two bugs in the original stack, fixed here
- `_transform_is_usable` omitted the non-deterministic denylist, so a
transform calling `now()` that reached storage without passing
`UpdateDatasetCommand` — via create, via import, or via the free-text Extra
box, which every dataset owner can reach — *was* mirrored at query time and its
result cached for a day. Replaced by `is_transform_active`, which is
`validate_transform` with the messages discarded and therefore cannot drift
from the save path. Net −13 lines.
- `columns.partition_value_transform` was missing from the dataset GET
payload while `DatasourceModal` PUT it back for every column, so **every
dataset-editor save cleared the transform.**
Also new here: `partition_column` is a physical column name, so it joins
`DASHBOARD_DATASET_INACCESSIBLE_FIELDS` alongside `columns` and `sql`;
`DatasetPutSchema.extra` gains a structural validator for the mapping key,
because the Extra box is a second door into the same storage and a mistyped
value there would otherwise be silently ignored; and
`SqlaTable.update_from_object` preserves the mapping when a legacy payload
omits `extra`.
#### The follow-up migration
Not just `ADD COLUMN`. In one revision it must: add the four columns to
`tables`/`table_columns` **and** to the `*_version` shadow tables; backfill in
a Python batch loop over rows whose `extra` holds the key (not raw JSON SQL —
migrations run on sqlite/MySQL/Postgres), leaving a blob it cannot parse in
place rather than dropping an owner's configuration; strip the key; **delete
the facade properties and declare the real columns in the same commit** (a
property and a `Column` of the same name cannot coexist); fill in
`ColumnPartitionMappingStore`; add the four names to `export_fields`; flip
`PARTITION_MAPPING_STORE`. `ColumnPartitionMappingStore`'s docstring carries
that list, and its `NotImplementedError` names the migration.
It is deliberately not implemented now: it would read
`table.partition_column`, which today is the facade property, which calls the
store — unbounded recursion. Written against private attribute names instead it
could only ever be exercised against a hand-built stand-in, which proves
nothing about the parts that are actually hard.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no UI. As in the original stack, there are no editor form fields for
this; a mapping is configured through the REST API or the Extra box, and the
preview endpoint ships available but uncalled. The docs say so explicitly.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/connectors/sqla/partition_mapping_test.py \
tests/unit_tests/connectors/sqla/partition_mapping_storage_test.py \
tests/unit_tests/models/partition_mirroring_test.py \
tests/unit_tests/datasets/partition_mapping_serialization_test.py \
tests/unit_tests/datasets/partition_mapping_preview_test.py \
tests/unit_tests/dao/dataset_test.py \
tests/unit_tests/commands/dataset/update_test.py \
tests/unit_tests/sql/parse_tests.py
```
`partition_mapping_storage_test.py` is the file to read first:
round-tripping, defensive parsing of a blob an owner typed by hand, the memo,
read-only enforcement, the factory, and the store-swap parity test.
Two tests in `tests/unit_tests/dao/dataset_test.py` are the ones this design
exists for:
`test_a_typed_field_beats_a_stale_extra_in_the_same_payload` and
`test_a_mapping_written_through_extra_alone_lands`. Both fail if the funnel
is ever "simplified" back into property setters.
Manually, with `PARTITION_FILTER_MAPPING: True`:
1. `PUT /api/v1/dataset/<id>` with `partition_column`, and
`partition_value_transform` + `partition_transform_is_monotonic` on a column.
2. `SELECT extra FROM tables WHERE id = <id>` — the blob holds
`partition_filter_mapping` and every pre-existing key is intact.
3. Open the dataset editor, change a column description, save. Re-check step
2: the mapping is still there. *This is the ordering hazard; if the funnel were
wrong, this is where the mapping would disappear.*
4. Add a certification and save. Both the certification and the mapping are
present.
5. Build a chart filtered on the mapped column; **View query** shows the
mirrored predicate on the partition column.
6. Export the dataset to a zip, delete it, re-import — the mapping comes
back.
7. Type `{"partition_filter_mapping": 42}` into the Extra box and save —
rejected with a readable message rather than silently ignored.
8. Set `PARTITION_MAPPING_STORE = "columns"` and restart — a clear
`NotImplementedError` naming the migration, not a mystery 500.
Verified: 2949 backend unit tests green across the touched surface, plus a
full `tests/unit_tests` sweep at 8479 passed / 4 failed, those 4 being a
pre-existing `mcp_service` trio-parameterised failure confirmed identical on
pristine `master`. Frontend `partitionMapping.test.ts` green. `ruff check` and
`ruff format --check` clean. `docs/static/resources/openapi.json` regenerated
with `superset update-api-docs` (+165 lines, purely additive).
`git diff origin/master -- superset/migrations/` is empty, which is the
point.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [x] Required feature flags: `PARTITION_FILTER_MAPPING` (off by default)
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
#### Notes for review
- **Message catalogs are not regenerated here.** Nothing in
`.pre-commit-config.yaml` or CI enforces extraction, and 1,900 lines of
generated churn would make this unreviewable. Happy to add it as a follow-up or
to this PR, whichever you prefer.
- `superset/connectors/sqla/partition_mapping.py` was left as one module
with the new storage layer as a sibling, `partition_mapping_storage.py`, rather
than split into a package. The split is defensible on size alone, but it would
have turned a reviewable diff against the original stack into a
moved-everything diff, and it carries a specific trap:
`partition_mirroring_test.py` patches
`...partition_mapping.evaluate_transform`, which a package split silently turns
into a no-op that fails as a confusing assertion error.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
