sadpandajoe commented on code in PR #44431:
URL: https://github.com/apache/superset/pull/44431#discussion_r4449166286
##########
superset/connectors/sqla/partition_mapping.py:
##########
@@ -1139,7 +1195,7 @@ def resolve_partition_mapping(datasource: SqlaTable) ->
PartitionMapping | None:
function, freezing a snapshot of probe time into the predicate with nothing
on screen to say so.
"""
- if not feature_flag_manager.is_feature_enabled(FEATURE_FLAG):
+ if not partition_mapping_supported(datasource):
Review Comment:
The resolver now declines a mapping on engines without
`supports_partition_filter_mapping`, but `preview_partition_mapping()` still
builds its predicate from the request and never calls
`partition_mapping_supported()`. With the flag on, previewing mapped column `n`
to partition column `p` with `:value` on a SQLite or Postgres dataset returns
`valid: true` and `p = 5 OR p IS NULL`, while the equivalent chart emits no
partition predicate, so the preview shows a filter that never runs. Should the
preview apply the same capability check before it probes?
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -1497,8 +1532,9 @@ function DatasourceEditor({
applyImplicitMappingMove(prev, datasource.main_dttm_col, value),
);
// A calculated column can be the default datetime column but can never
- // show a transform, and anything already stored on one still has to go
- // either way, because it is saved and the query path reads it.
+ // show a transform, so the mapping never lands there -- but anything
+ // already stored on one still has to go, because it is saved and the
+ // query path reads it.
Review Comment:
The handler now leaves a hidden mapping alone, but saving still loses it:
`main_dttm_col` is in `PARTITION_MAPPING_ATTRIBUTES`, so `DatasetDAO.update()`
runs `clear_unmapped_partition_transforms()` before and after the update, and
that method is gated only on the feature flag. On a Postgres dataset that kept
`partition_column` with a transform on `a`, changing the default datetime
column to `b` and saving nulls `a`'s transform and monotonic flag server-side,
so pointing the dataset back at Hive or Impala cannot restore it. Should that
cleanup also skip engines where `supports_partition_filter_mapping` is false,
as the query path and the validator now do?
##########
tests/unit_tests/models/partition_mirroring_test.py:
##########
@@ -43,6 +43,9 @@
from superset.utils import json
from superset.utils.core import FilterOperator
+pytestmark =
pytest.mark.usefixtures("sqlite_supports_partition_filter_mapping")
Review Comment:
Opting only `SqliteEngineSpec` in here means the tests that call
`_on_postgres()` or patch in `ClickHouseEngineSpec` (the backslash value, the
IN-list member and the array element) now hit the capability gate first and
resolve no mapping before any escaping logic runs. Their "no probe" and
`region_key not in sql` assertions still pass if the `value_round_trips` guards
are removed. Could those tests opt their engines in (for example with
`every_engine_supports_partition_filter_mapping`) and first assert that the
same query mirrors with a plain value, so the decline is attributable to the
escaping check?
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorPartitionMapping.test.tsx:
##########
@@ -414,3 +423,88 @@ test("the mapped column's row is muted in the columns
table", async () => {
'num',
);
});
+
+const DATABASES = [
+ {
+ id: 1,
+ database_name: 'warehouse_hive',
+ backend: 'hive',
+ engine_information: {
+ supports_partition_filter_mapping: true,
+ partition_value_transform_default: 'unix_timestamp(:value)',
+ },
+ },
+ {
+ id: 2,
+ database_name: 'app_postgres',
+ backend: 'postgresql',
+ engine_information: {
+ supports_partition_filter_mapping: false,
+ partition_value_transform_default: null,
+ },
+ },
+];
+
+/** Render in edit mode with a database list the selector can switch between.
*/
+const renderWithDatabases = async (supportsPartitionMapping: boolean) => {
+ // Ahead of the catch-all `/api/v1/database/` mock, so the selector lists
+ // real options.
+ fetchMock.removeRoutes();
+ fetchMock.get(DATASOURCE_ENDPOINT, [], { name: DATASOURCE_ENDPOINT });
+ fetchMock.get('glob:*/api/v1/database/?q=*', {
+ result: DATABASES,
+ count: DATABASES.length,
+ });
+ setupDatasourceEditorMocks();
+
+ const props = createProps();
+ const current = DATABASES[supportsPartitionMapping ? 0 : 1];
+ // `database` and the transform default are on the editor's datasource but
+ // not on `DatasetObject`, hence the cast.
+ props.datasource = {
+ ...props.datasource,
+ database: {
+ id: current.id,
+ database_name: current.database_name,
+ backend: current.backend,
+ },
+ supports_partition_filter_mapping: supportsPartitionMapping,
+ partition_value_transform_default:
+ current.engine_information.partition_value_transform_default,
+ } as DatasourceEditorProps['datasource'];
+
+ fastRender(props);
+ await dismissDatasourceWarning();
+ await userEvent.click(await screen.findByRole('img', { name: /lock/i }));
+ return props;
+};
+
+const showsPartitionSection = async () => {
+ await userEvent.click(await screen.findByTestId('collection-tab-Columns'));
+ await screen.findByPlaceholderText('Search columns by name');
+ return screen.queryByTestId('partition-column-fields') !== null;
+};
+
+test('switching to a database whose engine lacks support hides partition
mapping', async () => {
+ // The capability arrives on the dataset payload for the database it was
+ // loaded with; switching databases in the editor has to re-derive it.
+ await renderWithDatabases(true);
+
+ await selectOption(
+ 'app_postgres',
+ 'Select database or type to search databases',
+ );
+
+ expect(await showsPartitionSection()).toBe(false);
+});
+
+test('switching to a database whose engine supports it offers partition
mapping', async () => {
Review Comment:
Understood on deferring it. Nothing in this PR currently guards that
`onDbChange` carries `partition_value_transform_default` through
`DatabaseSelector`, so a Postgres to Hive switch could pre-fill `:value`
instead of `unix_timestamp(:value)` without any test failing. Could the two
assertions land here, since this PR introduces that switch path?
--
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]
