eschutho commented on PR #43870: URL: https://github.com/apache/superset/pull/43870#issuecomment-6087229460
Rebased and updated on Hugh's behalf (I'm helping out at Elizabeth's request). **Rebase.** The branch is rebased onto current `master` and is now **d808efd1f0**. The history is linear: the stacked-branch and `master` merges are gone, and Hugh's 103 commits are replayed on top. Conflicts were only in `UPDATING.md` (both sides kept), the message catalogs, and test files that both sides had appended to. Before adding any new commits, I checked that the rebased tree was **identical** to the old head merged with `master`. Where linearizing left an intermediate commit differing from Hugh's own merge result, I folded the correction into the last commit that touches that file. A backup of the old head (b83e550984) is kept locally. **New commits for open review threads** (each has a regression test that fails without the fix; replies are on the threads): | Thread | Commit | Change | |---|---|---| | [`helpers.py` unflagged DATE column](https://github.com/apache/superset/pull/43870#discussion_r4227028394) | f1dce85766 | the resolution check keys on `type_generic`, not `is_dttm` | | [migration `Revises:`](https://github.com/apache/superset/pull/43870#discussion_r4227028421) | 8f4085c62a | docstring now names `e4a7c2b9d1f3` | | [offset-bearing string bound](https://github.com/apache/superset/pull/43870#discussion_r4226585536) | 1ee6c37feb | declined before the full-resolution return | | [numeric equality vs. representation](https://github.com/apache/superset/pull/43870#discussion_r4226585540) | 159185ed03 | probe value coerced losslessly to the column's int/float type | | [preview reason for text equality](https://github.com/apache/superset/pull/43870#discussion_r4226585543) | a533814e7b | separate message when `equality_mirrors_safely` withdraws `=`/`IN` | | [preview on array columns](https://github.com/apache/superset/pull/43870#discussion_r4226585547) | 1fa334d289 | preview probes the same array literal as the chart path | | [sample for numeric temporal columns](https://github.com/apache/superset/pull/43870#discussion_r4226585551) | dee9cca8f8 | epoch sample for `epoch_s`/`epoch_ms` BIGINT columns | | [partition column help text](https://github.com/apache/superset/pull/43870#discussion_r4200141981) | e61585070e | help text no longer says the column is hidden from Explore | | — | d808efd1f0 | regenerated message catalogs for the strings above | **Left open for Hugh / reviewers:** - [Per-column collation for equality mirroring](https://github.com/apache/superset/pull/43870#discussion_r4174107534). Engine-level semantics are handled (f3f49af437), but a column-level `COLLATE` on a byte-exact engine can't be detected. Closing it needs a product decision (an owner opt-in, or collation reflection), so I didn't guess. Details are on the thread. **Unresolved threads already addressed by Hugh's earlier commits.** These had no reply. I checked each one against the current code and found it fixed; SHAs refer to the rebased history. I haven't resolved any threads, so that's for the reviewers. <details><summary>59 threads</summary> | Thread | Finding | Fixed in | |---|---|---| | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4174107532) | On MySQL, this compilation doubles the percent signs in `date_format(:value, '%Y%m%d')`, but the probe… | 1f65535fe4 | | [DatasourceEditor.tsx](https://github.com/apache/superset/pull/43870#discussion_r4174107536) | With partition filter mapping disabled, changing the default datetime column away and back still clears… | 24ca0e49b2 | | [schemas.py](https://github.com/apache/superset/pull/43870#discussion_r4174107539) | PUT accepts a null monotonicity flag and export includes that null, but this import field rejects it, so… | b9e388c6c4 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4174107543) | Preview can return a valid emitted predicate for a mapped column with an active advanced data type,… | 3fc2465e6b | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4175105706) | A transform such as `secret \|\| :value FROM vault` passes the single-expression and subquery checks even… | 681400f504 | | [helpers.py](https://github.com/apache/superset/pull/43870#discussion_r4175105710) | These outer-window partition predicates also enter the series-limit subquery through `where_clause_and`,… | 84439af53b | | [helpers.py](https://github.com/apache/superset/pull/43870#discussion_r4175105716) | The probe uses this Python datetime, while the original filter renders it through `dttm_sql_literal` and… | 2bc3d778d3 | | [utils.py](https://github.com/apache/superset/pull/43870#discussion_r4175105718) | PUT deliberately saves an unfinished transform such as `unix_timestamp(:value` as inactive, but exporting… | 4e9e76c629 | | [useDebouncedCommit.ts](https://github.com/apache/superset/pull/43870#discussion_r4175105723) | This queues the commit callback from the input event, whose `CollectionTable.onFieldsetChange` still… | 24ca0e49b2 | | [DatasourceEditor.tsx](https://github.com/apache/superset/pull/43870#discussion_r4175105726) | These partition defaults and the edited transform live in `databaseColumns`, but `syncMetadata` merges… | 24ca0e49b2 | | [create.py](https://github.com/apache/superset/pull/43870#discussion_r4175105730) | Passing `main_dttm_col=None` lets POST accept `partition_column='event_time'` without an override when… | 85b1addb68 | | [api.py](https://github.com/apache/superset/pull/43870#discussion_r4175105733) | Saving the dataset from Explore reloads this GET response and replaces the Explore datasource with it, but… | e722491958 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4175105736) | Reading the probe through `frame.iloc[0]` coerces mixed numeric columns to a common float dtype. On… | 1f65535fe4 | | [dataset.py](https://github.com/apache/superset/pull/43870#discussion_r4175105739) | When one PUT renames the partition column from `old_part` to `new_part` and updates `partition_column`,… | fced8ba878 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4179203234) | With a transform of `':value'`, a sample of ` \|\| (SELECT secret FROM vault LIMIT 1) \|\| ` compiles into… | e84b6d2abb | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4179203236) | Mixed probe results such as `[33, 29.02]` make SQLAlchemy infer an integer IN bind from the first value,… | e84b6d2abb | | [helpers.py](https://github.com/apache/superset/pull/43870#discussion_r4179203238) | For a numeric IN filter containing `['9007199254740993', '1.5']`, the original predicate later normalizes… | e84b6d2abb | | [helpers.py](https://github.com/apache/superset/pull/43870#discussion_r4179203241) | On ClickHouse, `time_groupby_inline` skips the ranking subquery's time predicate, but these mirrors still… | e84b6d2abb | | [DatasourceEditor.tsx](https://github.com/apache/superset/pull/43870#discussion_r4179203243) | A calculated mapped-column override accepted by PUT/import is absent from `databaseColumns`, so reopening… | 3ba0f3cce6 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4179203246) | Preview passes string samples directly to the probe, while chart filters first coerce numeric-column… | e84b6d2abb | | [views.py](https://github.com/apache/superset/pull/43870#discussion_r4179203248) | This legacy save accepts a valid transform longer than 1,024 characters, such as `lower(` plus 1,100… | e84b6d2abb | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4179203251) | This error and seven other new strings are absent from `messages.pot`, leaving them outside the… | 6b25a733cb | | [update_test.py](https://github.com/apache/superset/pull/43870#discussion_r4179203253) | The pre-commit mypy check fails here because `_model` is typed as `SqlaTable \| None`; setting it in the… | e84b6d2abb | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4185020713) | A bare scalar can still read tables: PostgreSQL's `schema_to_xml` passes these checks and the default… | 90bc18f54a | | [helpers.py](https://github.com/apache/superset/pull/43870#discussion_r4185020733) | On a mapped virtual dataset using `current_username()`, cache-key extraction calls `get_sqla_query()`… | 380956a582 | | [DatasourceEditor.tsx](https://github.com/apache/superset/pull/43870#discussion_r4185020751) | Clicking Customize the value transform now filters to the mapped column but leaves its row collapsed: the… | ea3ecf6514 | | [models.py](https://github.com/apache/superset/pull/43870#discussion_r4185020761) | Duplicating a mapped virtual dataset copies these references through `table.override()`, but… | e278dd0368 | | [update.py](https://github.com/apache/superset/pull/43870#discussion_r4185020773) | After the dedicated metadata-refresh endpoint removes a dropped partition column, its stored mapping… | 4b93825cd9 | | [partition-filter-mapping.mdx](https://github.com/apache/superset/pull/43870#discussion_r4185020783) | With the default `CACHE_CONFIG` (NullCache), enabling this feature leaves previews unthrottled and probe… | 9c93e1072d | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4186147475) | For an array filter whose rendered literal contains escaped backslashes, this regex replacement changes… | 0690d1eb10 | | [helpers.py](https://github.com/apache/superset/pull/43870#discussion_r4186147491) | For TIMESTAMP equality/IN filters supplied as epoch milliseconds, the handler returns SQLAlchemy… | 63cb0cdd98 | | [schemas.py](https://github.com/apache/superset/pull/43870#discussion_r4186147507) | On overwrite import, replacing a declared-monotonic transform with `hour(:value)` while omitting this… | 605276f407 | | [utils.py](https://github.com/apache/superset/pull/43870#discussion_r4186147517) | An overwrite import that clears the mapped-column override can activate an old transform parked on the… | 52c758a4dd | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4191232092) | The count check can be satisfied by the owner writing `superset_pfm_value_standin` themselves. In… | 770a0e00c8 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4191232095) | The exemption for `STRING` partition columns assumes every engine accepts a number in a text comparison,… | e61f2905af | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4191232101) | Preview passes `_probe_input` straight into `build_mirrored_predicates`, but chart filters go through… | 8dd7a5449d | | [helpers.py](https://github.com/apache/superset/pull/43870#discussion_r4191232106) | This adds an inner-window partition mirror for `main_dttm_col`, but the series-limit subquery only gets a… | b6df89506e | | [refresh.py](https://github.com/apache/superset/pull/43870#discussion_r4198455452) | A metadata refresh can switch on a transform nobody chose: if `fetch_metadata` or the dangling-reference… | 403df88c48 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4198455460) | A transform such as `:value::bigint` passes validation (`:value\b` matches) but never binds here:… | 403df88c48 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4198455475) | Accepting any `str` for a temporal partition column means a mirror can fail the chart rather than decline:… | 403df88c48 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4198455483) | The probe cache key covers the database record but not the identity the warehouse runs the query as, so… | 403df88c48 | | [DatasourceEditor.tsx](https://github.com/apache/superset/pull/43870#discussion_r4198455490) | `handleNavigateToColumn` sets `activeTabKey`, but the tabs only receive `defaultActiveKey`, which antd… | 403df88c48 | | [PartitionMappingSection.tsx](https://github.com/apache/superset/pull/43870#discussion_r4198455495) | Every expanded row reports to the same `onPreviewVerdict`, and rows that aren't the mapped column report… | 403df88c48 | | [PartitionColumnFields.tsx](https://github.com/apache/superset/pull/43870#discussion_r4198455505) | `datasource.partition_filter_mapping?.evaluable !== false` is the summary from when the editor opened, so… | 403df88c48 | | [utils.ts](https://github.com/apache/superset/pull/43870#discussion_r4198455517) | This rejects every resolved self-mapping, but the backend deliberately lets the implicit one through as a… | 403df88c48 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4200141993) | `_probe_input` always returns `handled` and never `UNMIRRORABLE`; the decline happens in… | 403df88c48 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4202018855) | The shape gates here inspect sqlglot's parse of the SQL, but this executes the original text, and sqlglot… | 403df88c48 | | [partition_mapping.py](https://github.com/apache/superset/pull/43870#discussion_r4202018858) | This records a dataset-specific outcome (this partition column can't hold the probed type) under… | 403df88c48 | | [usePartitionMappingPreview.ts](https://github.com/apache/superset/pull/43870#discussion_r4202018863) | A transform longer than 1,024 characters that contains `:value` makes the preview schema return HTTP 400… | 403df88c48 | | [UPDATING.md](https://github.com/apache/superset/pull/43870#discussion_r4202018869) | The note lists only the four newly denied XML functions, but this PR also adds `ts_rewrite` and `ts_stat`… | 403df88c48 | | [partition-filter-mapping.mdx](https://github.com/apache/superset/pull/43870#discussion_r4202018871) | `FEATURE_FLAGS` values aren't evaluated per request: `FeatureFlagManager` returns configured values… | 403df88c48 | | [partition_mapping_preview_test.py](https://github.com/apache/superset/pull/43870#discussion_r4202018874) | Every preview endpoint test runs with `raise_for_editorship` patched out here, and… | 403df88c48 | | [helpers.py](https://github.com/apache/superset/pull/43870#discussion_r4202688638) | The mirror is probed with `dttm.strftime(tf)` whenever a `python_date_format` is set, but… | 403df88c48 | | [partition_mapping_test.py](https://github.com/apache/superset/pull/43870#discussion_r4202688644) | `lower(:value), 'x'` is refused by `stored_expression_error` (not a single expression) before `get_df` is… | 403df88c48 | | [views.py](https://github.com/apache/superset/pull/43870#discussion_r4202688646) | The legacy save now refuses an unsafe transform, but `partition_column` and `partition_mapped_column` ride… | 403df88c48 | | [helpers.py](https://github.com/apache/superset/pull/43870#discussion_r4224320536) | This rounds the bound for any mapped column whose engine renders datetimes, not only temporal ones.… | 32f997b826 | | [views.py](https://github.com/apache/superset/pull/43870#discussion_r4224320545) | The legacy save only runs `stored_expression_error` on a changed transform, while the REST path and the… | 32f997b826 | | [utils.py](https://github.com/apache/superset/pull/43870#discussion_r4224320560) | This cleanup only drops references to columns that no longer exist. A bundle that sets both… | 32f997b826 | | [partition_mapping_test.py](https://github.com/apache/superset/pull/43870#discussion_r4224320575) | On `sqlite://` both `database.backend` and `database.db_engine_spec.engine` are `"sqlite"`, so this passes… | 32f997b826 | </details> **Testing.** Backend unit suites for the touched areas pass locally: connectors, models, datasets, dataset DAO and commands, SQL parsing, datasource views, translations, engine specs and migrations — 5,483 passed. Ruff, pylint and mypy pre-commit hooks pass on the changed files, and `check_pot_drift.py` is clean. `node_modules` wasn't available here, so the frontend change (dee9cca8f8, e61585070e) relies on CI for jest/tsc. -- 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]
