hughhhh commented on code in PR #45848:
URL: https://github.com/apache/superset/pull/45848#discussion_r4415351917
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -1428,28 +1489,50 @@ function DatasourceEditor({
// column the mapped one, and a transform stranded there by an earlier
// writer would otherwise arrive live from a single select. Clearing the
// partition column leaves no mapped column at all, so no column may hold
- // a transform -- which is what the server does too.
+ // a transform -- which is what the server does too. When that costs the
+ // transform the mapping is mirroring, the owner confirms it first.
//
// A transform on a column that is mapped on both sides is untouched, so
// re-pointing the partition column does not cost the owner what they
- // wrote.
- setDatabaseColumns(prev =>
- clearUnmappedTransformsAcrossMove(datasource, next, prev),
- );
- setCalculatedColumns(prev =>
- clearUnmappedTransformsAcrossMove(datasource, next, prev),
+ // wrote, and does not ask.
+ confirmMappingDiscard(
+ [
+ ...clearUnmappedTransformsAcrossMove(
+ datasource,
+ next,
+ databaseColumns,
+ ),
+ ...clearUnmappedTransformsAcrossMove(
+ datasource,
+ next,
+ calculatedColumns,
+ ),
+ ],
+ () => {
+ setDatasource(prev => ({
+ ...prev,
+ partition_column: next.partition_column,
+ partition_mapped_column: next.partition_mapped_column,
+ }));
Review Comment:
{ai generated comment}
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of @hughhhh**
Declining. This needs a column sync still in flight when the dialog opens:
sync runs only on an explicit click and the confirm is modal. The pre-existing
handler already applied the same render-time `next`. If it did happen,
`partitionMappingErrors` reports "Partition column X is not a column on this
dataset" and blocks Save, so no bad mapping can persist.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/PartitionFilterMapping.test.tsx:
##########
@@ -1455,3 +1473,34 @@ test('the unsaved notice is not reported as a failed
verdict', async () => {
).toBeInTheDocument();
expect(onPreviewVerdict).not.toHaveBeenCalledWith(false);
});
+
+test('a stored pass about another partition column is not a confirmation', ()
=> {
+ // The stored verdict is keyed on the transform text alone, so after the
+ // partition column is re-pointed it still reads as a pass -- for a mirror
+ // onto a column nobody has checked it against.
+ render(
+ <PartitionColumnFields
+ datasource={{
+ main_dttm_col: 'event_time',
+ partition_column: 'dt_epoch',
+ partition_filter_mapping: {
+ evaluable: true,
+ evaluated_transform: 'unix_timestamp(:value)',
+ mapped_column: 'event_time',
+ partition_column: 'dt_other',
Review Comment:
{ai generated comment}
> [!NOTE]
> 🤖 **Claude Opus 5.5 responding on behalf of @hughhhh**
Declining. The test models an unsaved edit: the summary describes the saved
mapping (`dt_other`), and the editor's partition column has been re-pointed
(`dt_epoch`) but not saved. That is the case this frontend check closes. A
backend verdict reused across saved mappings that share a transform text is the
verdict-identity bug tracked separately (SC-128853) and is out of scope here.
--
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]
