feat(datasets): add partition filter mapping model, migration and validation - #43757
feat(datasets): add partition filter mapping model, migration and validation#43757hughhhh wants to merge 1 commit into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
588f71c to
d909e49
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43757 +/- ##
==========================================
+ Coverage 79.81% 79.82% +0.01%
==========================================
Files 2913 2914 +1
Lines 170326 170491 +165
Branches 39485 39514 +29
==========================================
+ Hits 135945 136094 +149
- Misses 31859 31869 +10
- Partials 2522 2528 +6
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:
|
d909e49 to
d092638
Compare
…idation Datasets on Hadoop-family engines are often 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 today the only workaround is hand-writing the predicate as custom SQL in a virtual dataset. This is the first of four PRs making that a dataset setting. It adds the storage and the save-time validation; nothing reads the mapping yet. Four columns, following the `always_filter_main_dttm` / `currency_code_column` precedent for "a dataset-level setting that names a column": tables.partition_column the physical partition column tables.partition_mapped_column override; NULL follows main_dttm_col table_columns.partition_value_transform the `:value` expression table_columns.partition_transform_is_monotonic gates range mirroring The monotonic flag is NOT NULL DEFAULT false rather than a nullable tri-state, matching `normalize_columns` -- a nullable boolean invites `if x:` bugs where None and False need distinguishing and don't get it. Validation runs in two tiers. Structural and safety errors block the save: unknown columns, a column mapped onto itself, Jinja in the transform, and non-deterministic functions. Everything else -- an unparseable transform, a transform missing `:value` -- saves and leaves the mapping inactive, so a half-written transform doesn't cost the owner the rest of their edits. Note the self-mapping check validates the *effective* mapped column. Checking only the explicit override misses the case an owner actually hits: pointing `partition_column` at the column that is already `main_dttm_col`. `SQLStatement.get_niladic_functions` is added because the denylist cannot be purely name-based: on Hive and Impala `unix_timestamp()` means "now" while `unix_timestamp(x)` -- the canonical transform for this feature -- is pure. Gated behind the `PARTITION_FILTER_MAPPING` feature flag, off by default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d092638 to
f21009f
Compare
Code Review Agent Run #e33814Actionable Suggestions - 0Additional Suggestions - 4
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
| columns_by_name = {column.column_name: column for column in self.columns} | ||
| mapped_column_name = self.partition_mapped_column or self.main_dttm_col | ||
| mapped_column = columns_by_name.get(mapped_column_name or "") | ||
| active = bool( |
There was a problem hiding this comment.
A nonblank transform that fails validation (for example, one without :value) is deliberately saved as inactive, but this summary marks it active anyway. Should the summary use the same validation result so the Explore indicator does not report a mapping that will never mirror filters?
|
The current implementation deliberately separates validation into two tiers to ensure that non-blocking issues (like a missing While the summary could theoretically use the same validation result to reflect the mapping's active status, the current approach prioritizes user experience by allowing partial saves. The Explore indicator's behavior is consistent with this design, as it reports the configuration as it is stored, even if the mapping is currently inactive due to validation issues. superset/connectors/sqla/partition_mapping.py |
Stack (review bottom-up):
SUMMARY
First of a four-PR stack adding partition filter mapping. This one adds the storage and the save-time validation; nothing reads the mapping yet.
Datasets on Hadoop-family engines are often partitioned on a technical column — an epoch integer, a lowercased region key — that no analyst would ever filter on. Unless a query carries a predicate on that column the engine scans every partition, and today the only workaround 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_monotonicEffective mapped column is
partition_mapped_column or main_dttm_col, which makes "re-pointing the default datetime column moves the mapping with it, unless overridden" fall out of the model rather than needing code.The monotonic flag is
NOT NULL DEFAULT falserather than a nullable tri-state, matchingnormalize_columns— a nullable boolean invitesif x:bugs whereNoneandFalseneed distinguishing and don't get it.A JSON blob inside
tables.extrawas considered and rejected:extrais a user-editable free-text box in the dataset editor,buildExtraJsonObjectrebuildscolumn.extrafrom a hardcoded key list on every save, and there is no validation layer for it today. Every comparable setting shipped in the last year chose a real column.Validation runs in two tiers, because the PRD wants a mapping to "stay inactive until it parses" while some errors must still hard-block:
:value, no transform.Two details worth a look:
partition_columnat the column that is alreadymain_dttm_col.SQLStatement.get_niladic_functionsis added because the denylist can't be purely name-based: on Hive and Impalaunix_timestamp()means "now" whileunix_timestamp(x)— the canonical transform for this feature — is pure. Note sqlglot's Hive dialect already resolves the zero-arg form toCURRENT_TIMESTAMP; this is the backstop for dialects that don't normalize.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no UI in this PR.
TESTING INSTRUCTIONS
pytest tests/unit_tests/connectors/sqla/partition_mapping_test.py \ tests/unit_tests/datasets/partition_mapping_serialization_test.py \ tests/unit_tests/sql/parse_tests.py \ tests/unit_tests/commands/dataset/update_test.py \ tests/unit_tests/datasets/commands/export_test.pyMigration, verified on sqlite in both directions:
superset db upgrade # adds all four columns to tables/table_columns + both _version shadow tables superset db downgrade 1072de5ed955The serialization tests assert the mapping survives every layer it passes through (
export_fields, thedatapayload, PUT/import-v1 schemas, the API column lists) — a field missing from any one of them is dropped silently, which is the failure mode they exist to catch.ADDITIONAL INFORMATION
PARTITION_FILTER_MAPPING(off by default)ADD COLUMNs with no backfill and no index; effectively instant on Postgres/MySQL, no downtime expected