hughhhh commented on PR #43870: URL: https://github.com/apache/superset/pull/43870#issuecomment-5943828536
## Review comments carried over from the five-PR stack Partition filter mapping shipped as five stacked PRs (#43757, #43758, #43759, #43891, #43870) and was then collapsed into one: #43870, re-pointed at `master`. Closing the four parents orphans their review threads, so every thread from **Joe** (`sadpandajoe`) and **Evan** (`rusackas`) that GitHub still showed as unresolved is reproduced here, with a link back to the original and what became of it. Most are marked *addressed*: the fix landed in the review pass of 2026-10-01 and the thread is unresolved only because nobody clicked Resolve. The exceptions say so in their own words. **47 threads — 46 addressed, 1 left as a stated limitation.** ## PR #43757 — model, migration and validation *closed as superseded by #43870. 1 unresolved thread.* ### `superset/connectors/sqla/models.py`:1987 [thread](https://github.com/apache/superset/pull/43757#discussion_r3947007438) · hughhhh, sadpandajoe > 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? **Addressed** in `17a5f68312 (earlier pass)` — `models.py` calls `is_transform_active`; pinned by `test_activity_is_exactly_the_absence_of_issues` ## PR #43758 — query-time mirroring *closed as superseded by #43870. 2 unresolved threads.* ### `superset/models/helpers.py`:5832 [thread](https://github.com/apache/superset/pull/43758#discussion_r3946865281) · sadpandajoe > This can silently remove rows whenever a mapped Hive or Impala column has a supported NULL/special partition: the original filter can match the row, but the appended partition comparison cannot. Should mappings reject that case or preserve those rows before adding this predicate? **Addressed** in `1987cb8cfd (earlier pass)` — every mirror is wrapped in `OR partition_col IS NULL` ### `superset/models/helpers.py` *(outdated)* [thread](https://github.com/apache/superset/pull/43758#discussion_r3946874912) · sadpandajoe > This mirrors the raw range even when the real predicate uses `flt_grain` and filters a truncated timestamp expression. For a weekly upper bound, rows later in the final week still match `DATE_TRUNC(...) < until` but the raw partition predicate excludes them. Should grain-bearing filters skip mirroring (or use bounds that preserve the truncated predicate)? **Addressed** in `1987cb8cfd + 3d028a015a (earlier pass)` — grained non-ranges skip mirroring; grained ranges widen both bounds ## PR #43759 — preview endpoint and editor plumbing *closed as superseded by #43870. 6 unresolved threads.* ### `superset/datasets/api.py` *(outdated)* [thread](https://github.com/apache/superset/pull/43759#discussion_r3942064511) · sadpandajoe > This read-then-set counter is not atomic. Concurrent preview requests can all read the same value below the limit, each write the same incremented value, and all reach the warehouse probe, so a burst can bypass the query-load guard this endpoint relies on. Could this use an atomic cache increment/limit operation and cover concurrent requests? **Addressed** in `d9125ebe19 (earlier pass) + 17f8f28b41` — `add`+`inc`, and the window now comes from a bucketed key ### `superset/datasets/api.py` *(outdated)* [thread](https://github.com/apache/superset/pull/43759#discussion_r4054141949) · codeant-ai-for-open-source, sadpandajoe > Agreed—the current `inc()` still resets the expiry on SimpleCache and other read-modify-write backends, using the default cache timeout rather than 60 seconds. With the one-day default, retries after the budget is spent keep renewing a day-long lockout; could the fixed-window behavior be enforced for supported backends or the Redis-only requirement be explicit? **Addressed** in `17f8f28b41` — the window is carried by a per-window key, so a backend whose `inc` restamps its own default lifetime can only affect the window it belongs to. A `SimpleCache` test drives two adjacent windows. ### `superset/connectors/sqla/partition_mapping.py`:948 [thread](https://github.com/apache/superset/pull/43759#discussion_r4155597487) · sadpandajoe > A successful integer probe returns a NumPy scalar from the pandas row, but `sa.literal()` cannot infer its SQL type, so the canonical epoch transform raises `CompileError` here and preview returns 500. Could the probed scalar be normalized or rendered with an explicit type, with coverage through a real DataFrame result rather than a mocked Python `int`? **Addressed** in `17f8f28b41` — `_to_python_scalar` at the probe boundary; the test patches `Database.get_df` ### `superset-frontend/src/components/Datasource/DatasourceModal/index.tsx`:243 [thread](https://github.com/apache/superset/pull/43759#discussion_r4155597505) · sadpandajoe > The Dataset List editor loads `/api/v1/dataset/{id}`, whose column response omits both transform fields, so an unrelated save now sends `null`/`false` and overwrites an existing mapping, silently disabling pruning. Could those fields be included in the GET response and the GET→edit→PUT round trip be covered? **Addressed** in `earlier pass` — both fields are in `show_select_columns` / `show_columns` ### `superset/connectors/sqla/partition_mapping.py`:914 [thread](https://github.com/apache/superset/pull/43759#discussion_r4155597514) · sadpandajoe > A dataset editor without SQL Lab can submit a scalar subquery as the unsaved transform and receive its warehouse result in `emitted_predicate`; this path never applies the subquery/RLS checks used for stored expressions. Could the shared SQL policy and row-level-security validation run before the probe, so preview cannot read rows the editor is not authorized to retrieve? **Addressed** in `17f8f28b41` — `validate_stored_expression` runs before the probe ### `superset/datasets/api.py`:216 [thread](https://github.com/apache/superset/pull/43759#discussion_r4155597527) · sadpandajoe > If the Redis key expires after `add()` returns false but before `inc()`, `INCR` recreates it without a TTL. That counter then never resets, so this user eventually gets permanent 429s for the dataset; could creation/increment/expiry be atomic or missing expiry be repaired? **Addressed** in `17f8f28b41` — same per-window key: a TTL-less counter can only affect its own window ## PR #43891 — editor UI and docs *closed as superseded by #43870. 22 unresolved threads.* ### `docs/admin_docs/configuration/partition-filter-mapping.mdx` *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r3938586515) · sadpandajoe > Hive and Impala both support special partitions for NULL partition-key values. A mapped filter such as `event_time >= ...` then appends `dt_epoch >= ...` and drops those matching rows, so this feature can return an incomplete chart result. Should this explicitly reject/handle NULL partition values (and update the documentation) rather than assert they cannot occur? **Addressed** in `1987cb8cfd (earlier pass)` — every mirror is wrapped in `OR partition_col IS NULL` ### `superset/connectors/sqla/partition_mapping.py`:1070 [thread](https://github.com/apache/superset/pull/43891#discussion_r3939219865) · codeant-ai-for-open-source, sadpandajoe > `TEMPORAL_RANGE` is allowed by the preview schema, but this path forwards the single transformed value to `handle_comparison_filter`, whose base implementation only handles scalar comparison operators and raises `ValueError` for `TEMPORAL_RANGE`. Should the preview either reject this operator or build both range bounds with the same temporal-range path as chart queries? **Addressed** in `6113387c98` — `PREVIEWABLE_OPERATORS` excludes it, in both the request schema and the function ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionMappingSection.tsx`:81 [thread](https://github.com/apache/superset/pull/43891#discussion_r4129728947) · sadpandajoe > In `usePartitionMappingPreview.ts`, neither the success handler (`.then()`, line 93) nor the early-return guard (lines 66–74) checks whether the in-flight request is still the current one, so the preview panel this returns can get stuck or show stale data. If `transformCanPreview` starts failing while a request is pending (for example the owner clears the transform), the pending request's `.catch`/`.finally` both skip `setLoading(false)` because `controlle **Addressed** in `6113387c98` — request-id check in `.then`/`.catch`/`.finally`, and `loading` cleared on the early return ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionMappingSection.tsx`:81 [thread](https://github.com/apache/superset/pull/43891#discussion_r4129728956) · sadpandajoe > `usePartitionMappingPreview.ts` uses a literal NUL byte as an array delimiter (`sampleValues.join('\0')` at line 62, `sampleKey.split('\0')` at line 87). That's why `git diff`/GitHub render that file as "Binary files differ" instead of text — every future change to it will show the same way, hiding it from normal diff and blame tooling. Could the delimiter be something that isn't a valid string character in practice (or the round-trip through a string key **Addressed** in `6113387c98` — `JSON.stringify` key plus a `useMemo`; the file diffs as text again ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionColumnFields.tsx` *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4129728963) · sadpandajoe > None of the four new `Typography.Link` action triggers this section introduces have an `href` — here, and also at line 171 and line 200 in this file, plus `PartitionMappingSection.tsx:106`. Ant Design's `Typography.Link` without `href` renders an `<a>` that's outside the natural tab order and doesn't respond to Enter/Space, so a keyboard-only user can't reach "Map a different column instead", "Customize the value transform", "Map a column", or "Move mappin **Addressed** in `6113387c98` — all four are link-styled `Button`s, with keyboard tests ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/utils.ts`:77 [thread](https://github.com/apache/superset/pull/43891#discussion_r4129728972) · sadpandajoe > This treats any non-blank `partition_value_transform` as an active mapping, but the backend's own `partition_filter_mapping_summary` (models.py) requires `is_transform_active()` — parseable and passing the denylist — before calling a mapping active. `PartitionColumnFields.tsx` uses this frontend `isActive` to choose between the green "Filters on X will automatically apply..." message and the "No value transform is set" warning, so a transform that's non-bl **Addressed** in `6113387c98` — `mappingIsActive` shares `transformCanPreview`'s conditions ### `superset/datasets/api.py`:2231 [thread](https://github.com/apache/superset/pull/43891#discussion_r4129728980) · sadpandajoe > The response schema documents `sample_input`, `emitted_predicate`, `error`, and `valid`, but not `reason` — even though every invalid result this endpoint returns now includes one of `parse`, `validation`, `operator`, `engine`, or `unconfigured` (see `preview_partition_mapping` in `partition_mapping.py`), and the frontend's `PartitionMappingPreview` type reads and branches on it. Generated OpenAPI clients have no typed way to see this field. Could `reason` **Addressed** in `6113387c98` — `reason` documented with its enum; spec regenerated ### `docs/admin_docs/configuration/partition-filter-mapping.mdx` *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4129728988) · sadpandajoe > This names the zero-argument `unix_timestamp()`, but the paragraph right above says that form is rejected at save time — the one that can actually be saved and used is `unix_timestamp(:value)`, which is the one whose result actually depends on session timezone. Could the warning name `unix_timestamp(:value)` instead, so it's clear which usable transform needs care? **Addressed** in `6113387c98` — the docs name `unix_timestamp(:value)` ### `superset/connectors/sqla/partition_mapping.py`:1065 [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611386) · sadpandajoe > `mapping.mirrors(operator)` only rejects `TEMPORAL_RANGE` when the transform is *not* declared monotonic. When `is_monotonic=True`, a `TEMPORAL_RANGE` preview passes this guard, then falls into `value = sample_values[0]` (a single scalar, since only `IN` takes the list branch) and reaches `handle_comparison_filter(col, TEMPORAL_RANGE, value)`, which has no case for that operator and raises `ValueError` — a 500 with a valid transform and a monotonic declara **Addressed** in `6113387c98` — `PREVIEWABLE_OPERATORS` excludes it, in both the request schema and the function ### `superset/connectors/sqla/partition_mapping.py` *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611395) · sadpandajoe > This query-time gate checks blank/placeholder/Jinja/parseable but not non-deterministic functions, while `is_transform_active()` (used for the UI's active/inactive indicator) also runs that check. `CreateDatasetCommand`/import do not validate the mapping at all, so an imported or pre-existing dataset can carry a transform like `rand() + 0*:value`: the editor reports it inactive, but this function still lets it reach the probe, so a chart query mirrors a fr **Addressed** in `7951536198` — `_transform_is_usable` deleted; the query-time gate is `is_transform_active` ### `superset/connectors/sqla/partition_mapping.py`:612 [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611403) · sadpandajoe > Nothing here rejects a transform whose SELECT list has more than one expression (e.g. `lower(:value), :value`). For an `IN` filter with N distinct values, `build_probe_sql` compiles N pairs of columns into one row, but this only ever reads `row.iloc[0..len(distinct)-1]` — the first `len(distinct)` columns of the *first* value's own row, not one column per distinct value. For a 2-value `IN` filter that becomes both returned values coming from the first inpu **Addressed** in `7951536198` — `is_parseable` requires one select expression; the probe guard is `!=` ### `superset/datasets/api.py`:217 *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611417) · sadpandajoe > `add` and `inc` aren't atomic together: if the key is about to expire, `add` can return `False` (key still present) but then expire before this `inc` runs. Redis's `INCR` on a now-missing key creates it fresh with no TTL, so the counter keeps incrementing but never expires again. Once it's past `limit`, `_consume_preview_rate_limit` returns `False` permanently for that user/dataset instead of resetting after `PREVIEW_RATE_LIMIT_WINDOW` — previews stay 429' **Addressed** in `17f8f28b41` — same per-window key: a TTL-less counter can only affect its own window ### `superset/connectors/sqla/partition_mapping.py`:507 *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611434) · sadpandajoe > Only `:value` is bound as a parameter here — the rest of `transform` is raw SQL text that gets compiled and executed as-is, and nothing in `validate_transform`/`validate_partition_mapping` rejects a transform containing a subquery (only blank/Jinja/parseable/placeholder/non-determinism are checked). A user with dataset-editor access but no read access to another table could set a transform such as `(SELECT secret FROM protected_table LIMIT 1) || :value` an **Addressed** in `17f8f28b41` — `validate_stored_expression` runs before the probe ### `superset/commands/dataset/update.py`:316 *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611447) · sadpandajoe > This runs before `DatasetDAO.clear_dangling_partition_mapping` (called later, from `update_columns` during `run()`), and validates the *current* `partition_mapped_column`/`partition_column` against the submitted column list. So an `override_columns=true` sync that legitimately drops the currently-mapped column fails validation with a blocking error before the cleanup this PR added ever gets a chance to run — exactly the "clobbered, blocked or orphaned" cas **Addressed** in `0b4ba99bdc` — validation reads the post-cleanup mapping when a column payload is present ### `superset/connectors/sqla/partition_mapping.py` *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611458) · sadpandajoe > This builds a bare `SELECT <expr> AS v0, ...` with no `FROM`. Not every supported engine accepts a `FROM`-less `SELECT` — Oracle requires `FROM DUAL` (Superset's own Oracle engine spec already special-cases this elsewhere). On Oracle, a syntactically valid, correctly-declared transform would fail every probe, so the preview reports an engine error and the chart query path never prunes, even though the mapping is otherwise fine. Should this route through an **Addressed** in `7951536198` — `select_without_from_suffix` on the engine spec ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx`:1408 [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611469) · sadpandajoe > This only clears the transform on `resolveMappedColumn(datasource)` — whichever column is *currently* effective (the explicit override if set, else `main_dttm_col`) — then nulls `partition_mapped_column`. If an owner previously had an implicit mapping on `main_dttm_col` with its own transform, then explicitly overrode to a different column with a separate transform, clicking "Remove mapping" only clears the explicit column's transform. Once the override is **Addressed** in `earlier pass` — `clearMappingTransforms` clears every column ### `superset/datasets/api.py`:2173 *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611478) · sadpandajoe > This new method isn't added to the class's `method_permission_name` mapping, so `@protect()` requires its own `can_partition_mapping_preview` permission rather than inheriting from `can_write`. A custom role that already has `can_write` on Dataset — enough to save a mapping via this same PUT-based flow — is denied when calling preview, unless an operator separately grants the new permission. Could `partition_mapping_preview` map to `write` in `method_permi **Addressed** in `17f8f28b41` — mapped to `write` ### `superset/datasets/api.py`:2245 *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611495) · sadpandajoe > The 429 response here points at `#/components/responses/400` ("Bad request"), but a throttled caller gets a rate-limit response, not a bad-request one — and the endpoint's own editorship check can also return a 403, which isn't documented in this response list at all. A generated API client would show a throttled caller as having sent bad input, and wouldn't know a 403 is possible. Could the 429 reference the correct shared response and a 403 entry be adde **Addressed** in `17f8f28b41` — 429 documented properly, and a 403 added ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/utils.ts` *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611508) · sadpandajoe > Non-temporal columns always get sample values `['US', 'CA']` regardless of the mapped column's actual type. For an integer-typed mapped column with a transform like `CAST(:value AS INTEGER)`, these string samples fail to evaluate and preview reports a transform error, even though a real `IN (2025, 2026)` filter on that column would mirror successfully. Could the sample values be derived from the column's declared type instead of a temporal/non-temporal bin **Addressed** in `6113387c98` — samples follow the column's declared type ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionColumnFields.tsx` *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611522) · sadpandajoe > This navigates to `mappedColumn` — the column *currently* mapped — not a different one. When the effective mapped column is the implicit default datetime column and it also equals the partition column (the self-mapping state backend validation rejects), this link opens the partition column's own row, whose mapping section is defined to show nothing for the partition column itself. The one guided path meant to help an owner out of this state leads to a dead **Addressed** in `6113387c98` — falls back to `suggestedMappedColumn` on a self-mapping ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx` *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611529) · sadpandajoe > Clicking a "customize the value transform"-style link for the same column twice in a row (e.g. after manually collapsing that row) calls this with the same `columnName` both times. Nothing resets `columnToReveal` (or `columnSearchTerm`) to `null` when a row collapses, so the second call sets `setColumnToReveal(columnName)` to the value it already holds — React bails out of the state update, the consumer relying on `expandedColumnName={columnToReveal}` neve **Addressed** in `6113387c98` — the reveal request carries a nonce — **no test**, see the commit message ### `superset/config.py` *(outdated)* [thread](https://github.com/apache/superset/pull/43891#discussion_r4137611535) · sadpandajoe > This says "no query consumer emits the mirrored predicate yet," but this PR wires `resolve_partition_mapping`/`build_mirrored_predicates` into the chart-query path (`superset/models/helpers.py`), so enabling the flag now does change query SQL, not just storage/validation. An operator enabling this flag on the strength of this description would be surprised that charts start mirroring filters immediately. Could the comment be updated to reflect that the que **Addressed** in `7951536198` — flag description rewritten; `feature-flags.json` regenerated ## PR #43870 — Explore pruning indicator *re-pointed at `master`; this is the surviving PR. 16 unresolved threads.* ### `superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx`:429 [thread](https://github.com/apache/superset/pull/43870#discussion_r3939218324) · codeant-ai-for-open-source, sadpandajoe > Agreed—the standalone time control renders any active mapping even for `No filter` or a different temporal column, so the query has no matching partition predicate. Could this gate the glyph on an actual mapped time range? **Addressed** in `earlier pass` — gated by `timeRangePartitionMapping` ### `superset-frontend/src/explore/components/PartitionPruningIndicator/index.tsx`:132 [thread](https://github.com/apache/superset/pull/43870#discussion_r3939838584) · sadpandajoe > This labels every active mapped-column filter as mirrored, but the query path also rejects unsupported operators (and range filters for non-monotonic transforms). For example, a `country != 'US'` chip gets the glyph even though no partition predicate is emitted. Could this consume an applicability contract that includes the operator and monotonicity? **Addressed** in `earlier pass` — `mirrorable_operators` gate in `isMirroredFilter` ### `superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx`:265 [thread](https://github.com/apache/superset/pull/43870#discussion_r3970149881) · sadpandajoe > Changing the time range is handled by `SET_FIELD_VALUE`, which evaluates this mapper against the pre-action form data; and this control does not opt into render-time recalculation. After switching a mapped range to `No filter` (or the reverse), `partitionMapping` can therefore retain its old value and leave the glyph opposite the generated SQL. Could this opt into recalculation and add a field-update regression case? **Addressed** in `earlier pass` — `shouldMapStateToProps` plus `validationDependencies` ### `superset/daos/dataset.py` *(outdated)* [thread](https://github.com/apache/superset/pull/43870#discussion_r4129803967) · sadpandajoe > This runs on every `DatasetDAO.update()` call, including one that only changes an unrelated field like `description` -- there's no check that `attributes` actually touches partition/mapping state. Any dataset that already has a `partition_value_transform` stored on a column other than the current mapped one (a state the client editor alone couldn't fully prevent before this PR) will have that value silently and permanently cleared the next time anything ab **Addressed** in `1e132aefc9` — gated on `PARTITION_MAPPING_ATTRIBUTES` ### `superset/daos/dataset.py`:553 [thread](https://github.com/apache/superset/pull/43870#discussion_r4129803968) · sadpandajoe > `model.columns` here is whatever was already materialized earlier in this same `update()` call by `_upsert_columns`/`_override_columns`, which insert new rows via `db.session.add(TableColumn(..., table_id=model.id))` rather than appending to that relationship collection -- so a column added and given `partition_value_transform` in the same request as this call is invisible to this loop, and its transform is never cleared. Since that's exactly the "stray tr **Addressed** in `1e132aefc9` — `flush()` + `expire(model, ['columns'])` before the loop ### `superset-frontend/src/explore/components/PartitionPruningIndicator/index.tsx`:224 [thread](https://github.com/apache/superset/pull/43870#discussion_r4129803970) · sadpandajoe > This can show the pruning glyph on a filter the query path does not actually mirror. Two concrete cases: (1) a filter with an applied time grain (drill-to-detail) is explicitly excluded from mirroring in `models/helpers.py` ("Mirroring it raw would keep only the bucket's first instant"), but `MirrorCandidateFilter` carries no grain information, so a grained equality filter is always treated as mirrored here; (2) an `IN` filter containing the `<NULL>` senti **Addressed** in `1e132aefc9` — the grain rides on the candidate, and `<NULL>` is recognised ### `superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx`:251 [thread](https://github.com/apache/superset/pull/43870#discussion_r4129803977) · sadpandajoe > `mirrorsMainDttm` doesn't check that a temporal column is even selected. The backend only collects this `always_filter_main_dttm` mirror inside the `if granularity:` branch (`models/helpers.py`), so with no granularity selected the query never adds it -- but here the glyph shows purely from `always_filter_main_dttm && main_dttm_col === mapping.mapped_column`, regardless of `formData.granularity_sqla`. Could this also require a selected granularity before m **Addressed** in `1e132aefc9` — `mirrorsMainDttm` requires a selected granularity ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/useDebouncedCommit.ts`:88 [thread](https://github.com/apache/superset/pull/43870#discussion_r4129803986) · sadpandajoe > This can't tell an echo of its own last commit from a genuinely new external value, so it can still drop a keystroke: if `commit` fires and the resulting prop update (say "AB") arrives after the owner has already typed further ("ABC"), this resets `localValue` back to "AB", discarding the extra keystroke -- the next character then commits on top of the reverted text. The new round-trip test only re-syncs after typing has already finished, so this specific **Addressed** in `1e132aefc9` — the last committed value is recorded; the test flushes with Enter mid-typing ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/types.ts`:73 [thread](https://github.com/apache/superset/pull/43870#discussion_r4129803989) · sadpandajoe > `field` is asserted in the tests but neither production consumer reads it -- `DatasourceEditor`'s validation maps straight to `.message`, and `PartitionMappingSection` renders `.message` without checking `.field`. Could this wire `field` into error placement at the named input, or drop it so it isn't carrying metadata nothing uses? **Addressed** in `1e132aefc9` — wired into placement at both selects ### `superset-frontend/src/explore/components/PartitionPruningIndicator/index.tsx` *(outdated)* [thread](https://github.com/apache/superset/pull/43870#discussion_r4137602283) · sadpandajoe > `hasMirrorableValue` treats an empty string as not mirrorable (`comparator !== ''`), but the backend's mirror collector (`_collect_partition_mirror_filter` in `superset/models/helpers.py`) only excludes `None`, not `''`. A valid `column == ''` filter is mirrored server-side while Explore hides the indicator for it. Could this align with the backend's null-only exclusion? **Addressed** in `1e132aefc9` — aligned with the backend; the test that pinned it the wrong way was flipped ### `superset-frontend/src/explore/components/PartitionPruningIndicator/index.tsx`:191 [thread](https://github.com/apache/superset/pull/43870#discussion_r4137602299) · sadpandajoe > A virtual dataset's SQL can consume a filter via a Jinja `get_filters('col', remove_filter=True)` (or `get_time_filter(..., remove_filter=True)`) template call; the query builder then skips both the filter's own predicate and any partition mirror for that column entirely (`should_skip_filter` in `superset/models/helpers.py`). This function only looks at the filter's own shape, so it still shows the glyph for a filter on the mapped column that produces no S **Left as a known limitation**, written down where the check lives: `removed_filters` is built during query generation and is not in `form_data`, so the browser cannot know. The glyph is advisory; "View query" is the authority. ### `superset/connectors/sqla/partition_mapping.py` *(outdated)* [thread](https://github.com/apache/superset/pull/43870#discussion_r4137602310) · sadpandajoe > This trusts `_transform_is_usable` as the gate for real query-time mirroring, but that function (below) doesn't call `find_non_deterministic_functions`, while `validate_transform` (used both for PUT-time validation and for `is_transform_active`, which drives the Explore indicator) treats a non-deterministic function as blocking. Since import and `CreateDatasetCommand` don't run `validate_partition_mapping`, a transform calling e.g. `NOW()` can reach the da **Addressed** in `7951536198` — `_transform_is_usable` deleted; the query-time gate is `is_transform_active` ### `superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx` *(outdated)* [thread](https://github.com/apache/superset/pull/43870#discussion_r4137602318) · sadpandajoe > This checks `formData?.time_range === NO_TIME_RANGE`, but when `time_range` is simply absent from `form_data` (e.g. a legacy saved chart predating this control, or before defaults populate the state), the value is `undefined` rather than the sentinel, so this falls through to the mirrors-check below instead of returning null. Could a missing `time_range` be treated the same as `NO_TIME_RANGE` here? **Addressed** in `1e132aefc9` — a missing `time_range` counts as `NO_TIME_RANGE` ### `superset-frontend/src/explore/components/PartitionPruningIndicator/index.tsx` *(outdated)* [thread](https://github.com/apache/superset/pull/43870#discussion_r4137602324) · sadpandajoe > The tooltip's explanation is reachable only by mouse hover: the icon sits in a non-focusable `<span>` with no accessible name, so a keyboard or screen-reader user has no way to discover why the glyph is there. Could this add a focusable target with an accessible label (e.g. an `aria-label` matching the tooltip text)? **Addressed** in `1e132aefc9` — a text-variant `Button` with an `aria-label` ### `superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/useDebouncedCommit.ts`:54 [thread](https://github.com/apache/superset/pull/43870#discussion_r4137602330) · sadpandajoe > This comment says the commit callback reads the column's monotonic flag to decide what to write, but `commit` here is just the plain `(next: string) => void` passed in by the caller -- this hook doesn't reference monotonicity anywhere. Looks like a leftover from an earlier, more specific version of this code; could this be corrected or removed? **Addressed** in `1e132aefc9` — the stale claim is gone ### `superset/daos/dataset.py`:553 [thread](https://github.com/apache/superset/pull/43870#discussion_r4137602342) · sadpandajoe > Concretely: `_upsert_columns` and `_override_columns` attach a brand-new column via `db.session.add(TableColumn(..., table_id=model.id))` rather than through the `columns` relationship, and both already read `model.columns` earlier in the same call to build their by-id/by-name maps. When that collection was loaded before the insert, the new row isn't reflected in it when this loop runs right after -- so a single request that both adds a column with a `part **Addressed** in `1e132aefc9` — `flush()` + `expire(model, ['columns'])` before the loop -- 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]
