sadpandajoe commented on code in PR #44431:
URL: https://github.com/apache/superset/pull/44431#discussion_r4171377756
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/utils.ts:
##########
@@ -34,6 +35,36 @@ import type {
*/
const JINJA_PATTERN = /\{\{|\{%|\{#/;
+/**
+ * Whether the dataset editor should offer partition filter mapping at all.
+ *
+ * The single source of truth for the gate, so every place that shows partition
+ * mapping UI stays in lockstep: it needs the feature flag on, a datasource to
+ * read, and an engine that advertises support
(`supports_partition_filter_mapping`,
+ * true only for partition-directory engines like Hive/Impala/Spark). This
lived
+ * inline at one call site and was missed at another, which showed the section
on
+ * engines that do not support it -- hence one predicate both sites share.
+ */
+export function partitionFilterMappingEnabled(
+ datasource: PartitionMappingDatasource | undefined,
+): boolean {
+ return (
+ isFeatureEnabled(FeatureFlag.PartitionFilterMapping) &&
+ Boolean(datasource) &&
+ Boolean(datasource?.supports_partition_filter_mapping)
Review Comment:
The configuration guide still tells owners to enable the feature flag and
select Partition column, but that control now stays absent on engines such as
Presto. Could the guide document the supported engine scope and this additional
prerequisite?
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionFilterMapping.test.tsx:
##########
@@ -281,6 +304,150 @@ test('the ordering checkbox reports back which column it
belongs to', async () =
expect(onMonotonicChange).toHaveBeenCalledWith('event_time', true);
});
+test('editing the transform to the bare :value auto-declares it monotonic',
async () => {
+ // The identity transform provably preserves ordering, so typing it back in
+ // re-checks the box rather than leaving the owner to assert what cannot be
+ // false. Both writes ride one debounced commit, so the assertion waits.
+ fetchMock.post(PREVIEW_URL, { result: { valid: true } });
+ const onChange = jest.fn();
+ const onMonotonicChange = jest.fn();
+
+ render(
+ <PartitionMappingSection
+ item={{ column_name: 'event_time', is_dttm: true }}
+ value="unix_timestamp(:value)"
+ onChange={onChange}
+ datasource={{
+ id: 1,
+ main_dttm_col: 'event_time',
+ partition_column: 'dt_epoch',
+ }}
+ onMoveMappingHere={jest.fn()}
+ onRemoveMapping={jest.fn()}
+ onMonotonicChange={onMonotonicChange}
+ />,
+ );
+
+ fireEvent.change(screen.getByLabelText('Value transform'), {
+ target: { value: ':value' },
+ });
+
+ await waitFor(() => {
+ expect(onMonotonicChange).toHaveBeenCalledWith('event_time', true);
+ });
+ expect(onChange).toHaveBeenCalledWith(':value');
+});
+
+test('editing the transform away from :value clears the monotonic auto-check',
async () => {
+ // Monotonicity is a property of the expression, so once the transform is no
+ // longer the identity the prior auto-check must not linger on it.
+ fetchMock.post(PREVIEW_URL, { result: { valid: true } });
+ const onChange = jest.fn();
+ const onMonotonicChange = jest.fn();
+
+ render(
+ <PartitionMappingSection
+ item={{
+ column_name: 'event_time',
+ is_dttm: true,
+ partition_value_transform: ':value',
+ partition_transform_is_monotonic: true,
+ }}
+ value=":value"
+ onChange={onChange}
+ datasource={{
+ id: 1,
+ main_dttm_col: 'event_time',
+ partition_column: 'dt_epoch',
+ }}
+ onMoveMappingHere={jest.fn()}
+ onRemoveMapping={jest.fn()}
+ onMonotonicChange={onMonotonicChange}
+ />,
+ );
+
+ fireEvent.change(screen.getByLabelText('Value transform'), {
+ target: { value: 'unix_timestamp(:value)' },
+ });
+
+ await waitFor(() => {
+ expect(onMonotonicChange).toHaveBeenCalledWith('event_time', false);
+ });
+ expect(onChange).toHaveBeenCalledWith('unix_timestamp(:value)');
+});
+
+test('a hand-declared transform keeps its ordering flag through an edit',
async () => {
+ // The auto-declaration belongs to `:value` alone. A flag the owner ticked
+ // themselves on their own expression is an assertion about that expression,
+ // and fixing a typo in it is not a retraction.
+ fetchMock.post(PREVIEW_URL, { result: { valid: true } });
+ const onChange = jest.fn();
+ const onMonotonicChange = jest.fn();
+
+ render(
+ <PartitionMappingSection
+ item={{
+ column_name: 'event_time',
+ is_dttm: true,
+ partition_value_transform: 'unix_timestamp(:value)',
+ partition_transform_is_monotonic: true,
+ }}
+ value="unix_timestamp(:value)"
+ onChange={onChange}
+ datasource={{
+ id: 1,
+ main_dttm_col: 'event_time',
+ partition_column: 'dt_epoch',
+ }}
+ onMoveMappingHere={jest.fn()}
+ onRemoveMapping={jest.fn()}
+ onMonotonicChange={onMonotonicChange}
+ />,
+ );
+
+ fireEvent.change(screen.getByLabelText('Value transform'), {
+ target: { value: 'unix_timestamp(:value) ' },
+ });
+
+ await waitFor(() => {
+ expect(onChange).toHaveBeenCalledWith('unix_timestamp(:value) ');
+ });
+ expect(onMonotonicChange).not.toHaveBeenCalled();
+});
+
+test('the transform commits before the ordering flag', async () => {
+ // Load-bearing order: CollectionTable rebuilds the column record from a
+ // snapshot of its own last render, so the functional setDatabaseColumns
+ // behind onMonotonicChange has to land on top of that snapshot. Reversed,
+ // the snapshot wipes the flag back out and the auto-declaration is lost.
+ fetchMock.post(PREVIEW_URL, { result: { valid: true } });
+ const order: string[] = [];
+ const onChange = jest.fn(() => order.push('transform'));
+ const onMonotonicChange = jest.fn(() => order.push('monotonic'));
+
+ render(
+ <PartitionMappingSection
+ item={{ column_name: 'event_time', is_dttm: true }}
+ value="unix_timestamp(:value)"
+ onChange={onChange}
+ datasource={{
+ id: 1,
+ main_dttm_col: 'event_time',
+ partition_column: 'dt_epoch',
+ }}
+ onMoveMappingHere={jest.fn()}
+ onRemoveMapping={jest.fn()}
+ onMonotonicChange={onMonotonicChange}
+ />,
+ );
+
+ fireEvent.change(screen.getByLabelText('Value transform'), {
+ target: { value: ':value' },
+ });
+
+ await waitFor(() => expect(order).toEqual(['transform', 'monotonic']));
Review Comment:
This checks callback order but never exercises the editor snapshot/save
round trip that can drop the ordering flag. Could the editor regression test
edit a transform to `:value` and assert that the emitted column retains both
the transform and `partition_transform_is_monotonic: true`?
--
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]
