hughhhh commented on code in PR #45929:
URL: https://github.com/apache/superset/pull/45929#discussion_r4458050779
##########
superset/commands/dataset/update.py:
##########
@@ -576,14 +588,19 @@ def _validate_partition_mapping(self, exceptions:
list[ValidationError]) -> None
effective_mapped_column = partition_mapped_column or main_dttm_col
transform = self._effective_transform(columns, effective_mapped_column)
- for issue in validate_partition_mapping(
- column_names=column_names,
- partition_column=partition_column,
- partition_mapped_column=partition_mapped_column,
- main_dttm_col=main_dttm_col,
- transform=transform,
- engine=database.backend,
- ):
+ issues = (
+ validate_partition_mapping(
+ column_names=column_names,
+ partition_column=partition_column,
+ partition_mapped_column=partition_mapped_column,
+ main_dttm_col=main_dttm_col,
+ transform=transform,
+ engine=database.backend,
+ )
+ if database.db_engine_spec.supports_partition_filter_mapping
+ else validate_transform(transform, database.backend)
+ )
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
Fixed in c681738a8b. The claim was correct:
`_partition_mapping_reference_error` in `superset/views/datasource/views.py`
ran the column-reference checks on every engine, so a dangling stored mapping
answered 422 on the deprecated route while the REST PUT accepted it.
The route now skips those checks when the engine lacks
`supports_partition_filter_mapping`, matching
`UpdateDatasetCommand._validate_partition_mapping`. Transform validation stays
unconditional on both paths.
On a save that moves the dataset, the check reads the destination database,
because `save` assigns only `database_id` and `orm_datasource.database` still
answers for the source. This also matches the REST path, which reads the
requested database.
Tests in `tests/unit_tests/views/datasource/views_test.py`:
- `test_save_ignores_a_dangling_reference_on_an_unsupported_engine` failed
before the fix.
-
`test_save_judges_a_dangling_reference_by_the_destination_engine[out-of-a-supported-engine]`
failed before the fix.
- The `[into-a-supported-engine]` case guards the opposite direction, which
must still be refused.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -1329,6 +1335,24 @@ function DatasourceEditor({
});
}, []);
+ // The partition mapping capability and its transform pre-fill belong to the
+ // engine, and arrive on the dataset payload for the database it was loaded
+ // with. Pointing the dataset at another database has to re-derive both from
+ // the selected database's engine, or the editor keeps offering (or keeps
+ // hiding) partition mapping according to the engine it just left.
+ const onDatabaseChange = useCallback((db: DatabaseObject | undefined) => {
+ if (db === undefined) return;
+ setDatasource(prev => ({
+ ...prev,
+ database: db,
+ supports_partition_filter_mapping: Boolean(
+ db.supports_partition_filter_mapping,
+ ),
+ partition_value_transform_default:
+ db.partition_value_transform_default ?? null,
+ }));
Review Comment:
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of hughhhh**
The behaviour is real, but this PR doesn't introduce it, so I'm not fixing
it here and I'm leaving this thread open for a maintainer decision.
I reproduced it with a throwaway test on this branch and on unmodified
master (730016b861). Both give the same result after switching database:
`{"db":2,"catalog":"old_catalog","schema":"old_schema","table_name":"old_table"}`.
Master's handler was `onDatasourcePropChange('database', db)`, which also
spreads `...prev`. The selector's follow-up `onCatalogChange(undefined)` and
`onSchemaChange(undefined)` calls are dropped by the `value === undefined`
guard in that function. `onDatabaseChange` adds only the engine capability and
transform default to that update.
A fix would change how every dataset move behaves on every engine, with or
without this feature, and this PR is scoped to the engine gate. It also needs
decisions the gate doesn't answer, such as what happens to `table_name`, which
is the dataset's name on a virtual dataset and the pointer to the table on a
physical one. That fits better in its own PR.
--
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]
