aminghadersohi commented on code in PR #44806:
URL: https://github.com/apache/superset/pull/44806#discussion_r4190050153
##########
superset-frontend/plugins/plugin-chart-table/test/buildQuery.test.ts:
##########
@@ -116,6 +116,19 @@ test.each(['2__semantic_view', '11__table'])(
},
);
+test('semantic raw table drops ordering for a column no longer selected', ()
=> {
+ const query = buildQueryUncached({
+ ...basicFormData,
+ datasource: '2__semantic_view',
+ query_mode: QueryMode.Raw,
+ all_columns: ['played_at', 'song_name'],
+ order_by_cols: ['["artist_name",false]', '["played_at",false]'],
+ }).queries[0];
+
+ expect(query.columns).toEqual(['played_at', 'song_name']);
+ expect(query.orderby).toEqual([['played_at', false]]);
+});
Review Comment:
Nothing pins what the `effectiveOrderby` guard must leave alone: widening it
to `queryMode === QueryMode.Raw` (drops valid dataset sorts) or to
`isSemanticView` (drops Aggregate's `[[metrics[0], false]]`) keeps all 204
table-plugin tests green.
```suggestion
});
test.each([
{
datasource: '11__table',
query_mode: QueryMode.Raw,
orderby: [['artist_name', false]],
},
{
datasource: '2__semantic_view',
query_mode: QueryMode.Aggregate,
orderby: [['count', false]],
},
])(
'$datasource $query_mode keeps its ordering',
({ datasource, query_mode, orderby }) => {
const query = buildQueryUncached({
...basicFormData,
datasource,
query_mode,
groupby: ['song_name'],
metrics: ['count'],
all_columns: ['played_at'],
order_by_cols: ['["artist_name",false]'],
}).queries[0];
expect(query.orderby).toEqual(orderby);
},
);
```
##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -270,14 +269,24 @@ export const buildQuery: BuildQuery<TableChartFormData> =
(
sortByFromOwnState = [[sortByItem?.key, !sortByItem?.desc]];
}
+ const requestedOrderby =
+ formData.server_pagination && sortByFromOwnState
+ ? sortByFromOwnState
+ : orderby;
+ const selectedColumns = new Set(columns.filter(isPhysicalColumn));
+ const effectiveOrderby =
+ queryMode === QueryMode.Raw && isSemanticView
Review Comment:
Raw-mode sort leaks into Aggregate: sort `played_at` in Raw, switch to
Aggregate with groupby-only or percent-only, and `orderby` stays
`[['played_at', false]]` with columns `['song_name']`, which MetricFlow
rejects. Fix is at L150, outside this hunk: `} else if (isSemanticView) {
orderby = []; }`.
--
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]
