rusackas commented on code in PR #40907:
URL: https://github.com/apache/superset/pull/40907#discussion_r3976125146
##########
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx:
##########
@@ -62,6 +64,13 @@ export const useResultsPane = ({
const chartRowLimit = Number(queryFormData?.row_limit) || 10000;
const [rowLimit, setRowLimit] = useState(1000);
+ const [orderby, setOrderby] = useState<[string, boolean][]>([]);
+ // Server-side sort is only valid when the displayed columns map directly to
+ // the SQL result. When the query has post-processing (e.g.
pivot/cum/rolling),
+ // `orderby` + `row_limit` are applied to the raw SQL *before*
post-processing,
+ // which changes the rows that feed those operations and corrupts the result.
+ // In that case we fall back to client-side sorting of what the chart
produced.
+ const [hasPostProcessing, setHasPostProcessing] = useState(false);
Review Comment:
Good catch, fixed. `hasPostProcessing` now starts `true` so a sort click
during the async detection window can't slip a server-side orderby past a
post-processed query.
##########
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx:
##########
@@ -74,6 +76,13 @@ export const useResultsPane = ({
const chartRowLimit = Number(queryFormData?.row_limit) || 10000;
const [rowLimit, setRowLimit] = useState(1000);
+ const [orderby, setOrderby] = useState<[string, boolean][]>([]);
+ // Server-side sort is only valid when the displayed columns map directly to
+ // the SQL result. When the query has post-processing (e.g.
pivot/cum/rolling),
+ // `orderby` + `row_limit` are applied to the raw SQL *before*
post-processing,
+ // which changes the rows that feed those operations and corrupts the result.
+ // In that case we fall back to client-side sorting of what the chart
produced.
+ const [hasPostProcessing, setHasPostProcessing] = useState(false);
Review Comment:
Good catch, fixed. `hasPostProcessing` now starts `true` so a sort click
during the async detection window can't slip a server-side orderby past a
post-processed query.
##########
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx:
##########
@@ -74,8 +83,14 @@ export const useResultsPane = ({
const effectiveRowLimit = Math.min(rowLimit, chartRowLimit);
const cappedFormData = useMemo(
- () => ({ ...queryFormData, row_limit: effectiveRowLimit }),
- [queryFormData, effectiveRowLimit],
+ () => ({
+ ...queryFormData,
+ row_limit: effectiveRowLimit,
+ // A new `orderby` produces a new object, missing the cache below and
+ // triggering a server-side re-query in the sorted order.
+ ...(orderby.length > 0 && { orderby }),
Review Comment:
Fair point. Clearing the sort does fall back to the same metric-default
order the pane always used before this feature existed, so it's not a new
regression, just an indicator mismatch. Fixing it properly means teaching
buildQuery to tell 'no explicit order' apart from 'user cleared it, show
default', which feels like a bigger call than I want to make in this pass.
Leaving it as is for now.
##########
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx:
##########
@@ -97,8 +106,14 @@ export const useResultsPane = ({
: undefined;
const cappedFormData = useMemo(
- () => ({ ...queryFormData, row_limit: effectiveRowLimit }),
- [queryFormData, effectiveRowLimit],
+ () => ({
+ ...queryFormData,
+ row_limit: effectiveRowLimit,
+ // A new `orderby` produces a new object, missing the cache below and
+ // triggering a server-side re-query in the sorted order.
+ ...(orderby.length > 0 && { orderby }),
Review Comment:
Fair point. Clearing the sort does fall back to the same metric-default
order the pane always used before this feature existed, so it's not a new
regression, just an indicator mismatch. Fixing it properly means teaching
buildQuery to tell 'no explicit order' apart from 'user cleared it, show
default', which feels like a bigger call than I want to make in this pass.
Leaving it as is for now.
##########
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx:
##########
@@ -86,6 +101,10 @@ export const useResultsPane = ({
[cappedFormData],
);
+ const handleServerSort = useCallback((nextOrderby: [string, boolean][]) => {
+ setOrderby(nextOrderby);
+ }, []);
Review Comment:
Fixed with a request-id guard so an older in-flight fetch can't overwrite a
newer sort's result if it resolves out of order.
##########
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx:
##########
@@ -109,6 +124,10 @@ export const useResultsPane = ({
[cappedFormData],
);
+ const handleServerSort = useCallback((nextOrderby: [string, boolean][]) => {
+ setOrderby(nextOrderby);
+ }, []);
Review Comment:
Fixed with a request-id guard so an older in-flight fetch can't overwrite a
newer sort's result if it resolves out of order.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1524,10 +1536,13 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
const modifiedOwnState = {
...serverPaginationData,
sortBy,
+ // Changing the sort re-queries the full dataset, so the
+ // previous page offset is meaningless — return to the first page.
+ currentPage: 0,
};
updateTableOwnState(setDataMask, modifiedOwnState);
},
- [serverPagination, serverPaginationData, setDataMask],
+ [serverPaginationData, setDataMask],
Review Comment:
Good catch, fixed. Added `serverPagination` back to the dependency array.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1524,10 +1536,13 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
const modifiedOwnState = {
...serverPaginationData,
sortBy,
+ // Changing the sort re-queries the full dataset, so the
+ // previous page offset is meaningless — return to the first page.
+ currentPage: 0,
};
updateTableOwnState(setDataMask, modifiedOwnState);
},
- [serverPagination, serverPaginationData, setDataMask],
+ [serverPaginationData, setDataMask],
Review Comment:
Good catch, fixed. Added `serverPagination` back to the dependency array.
--
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]
