rusackas commented on code in PR #40907:
URL: https://github.com/apache/superset/pull/40907#discussion_r3976126260
##########
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/charts/data/api.py:
##########
@@ -343,6 +390,16 @@ def data( # noqa: C901
try:
query_context = self._create_query_context_from_form(json_body)
+ # Validate sort parameters before executing the query so bad sort
+ # directions are rejected early, before the query builder runs.
+ orderby = (
+ json_body.get("queries", [{}])[0].get("orderby", [])
+ if isinstance(json_body, dict) and json_body.get("queries")
+ else []
+ )
+ sort_error = validate_sort_params(orderby)
+ if sort_error is not None:
Review Comment:
Fixed, now validates every query object in the request instead of just the
first.
##########
superset/charts/data/api.py:
##########
@@ -343,6 +390,16 @@ def data( # noqa: C901
try:
query_context = self._create_query_context_from_form(json_body)
+ # Validate sort parameters before executing the query so bad sort
+ # directions are rejected early, before the query builder runs.
+ orderby = (
+ json_body.get("queries", [{}])[0].get("orderby", [])
+ if isinstance(json_body, dict) and json_body.get("queries")
+ else []
+ )
+ sort_error = validate_sort_params(orderby)
+ if sort_error is not None:
+ return sort_error
Review Comment:
Fixed, now validates every query object in the request instead of just the
first.
##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -128,13 +128,18 @@ const buildQuery: BuildQuery<TableChartFormData> = (
if (queryMode === QueryMode.Aggregate) {
metrics = metrics || [];
- // override orderby with timeseries metric when in aggregation mode
- if (sortByMetric) {
- orderby = [[sortByMetric, !orderDesc]];
- } else if (metrics?.length > 0) {
- // default to ordering by first metric in descending order
- // when no "sort by" metric is set (regardless if "SORT DESC" is set
to true)
- orderby = [[metrics[0], false]];
+ // Fall back to a metric-based default sort only when no explicit orderby
+ // was supplied (e.g. a column sort from the "View as table" results
pane).
+ // An explicit orderby from form data takes precedence.
+ if (orderby.length === 0) {
Review Comment:
Good call, but I don't have a working JS test env handy right now to add and
verify these safely, so I'll leave it as a follow-up rather than push test code
I can't run.
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -131,13 +131,18 @@ const buildQuery: BuildQuery<TableChartFormData> = (
if (queryMode === QueryMode.Aggregate) {
metrics = metrics || [];
- // override orderby with timeseries metric when in aggregation mode
- if (sortByMetric) {
- orderby = [[sortByMetric, !orderDesc]];
- } else if (metrics?.length > 0) {
- // default to ordering by first metric in descending order
- // when no "sort by" metric is set (regardless if "SORT DESC" is set
to true)
- orderby = [[metrics[0], false]];
+ // Fall back to a metric-based default sort only when no explicit orderby
+ // was supplied (e.g. a column sort from the "View as table" results
pane).
+ // An explicit orderby from form data takes precedence.
+ if (orderby.length === 0) {
Review Comment:
Good call, but I don't have a working JS test env handy right now to add and
verify these safely, so I'll leave it as a follow-up rather than push test code
I can't run.
##########
superset-frontend/src/components/GridTable/types.ts:
##########
@@ -59,4 +59,12 @@ export interface TableProps<RecordType> {
usePagination?: boolean;
striped?: boolean;
+
+ /**
+ * Called when the sort state changes, with it translated to a query
+ * `orderby` (`[[columnId, isAscending]]`) so the consumer can re-request
+ * server-sorted data. Only the primary sort column is included, matching the
+ * chart data API's single-column sort constraint.
+ */
Review Comment:
Good catch, fixed the docstring to reflect that multi-column sorts pass
through in priority order.
##########
superset-frontend/src/components/GridTable/types.ts:
##########
@@ -59,4 +59,12 @@ export interface TableProps<RecordType> {
usePagination?: boolean;
striped?: boolean;
+
+ /**
+ * Called when the sort state changes, with it translated to a query
+ * `orderby` (`[[columnId, isAscending]]`) so the consumer can re-request
+ * server-sorted data. Only the primary sort column is included, matching the
+ * chart data API's single-column sort constraint.
+ */
Review Comment:
Good catch, fixed the docstring to reflect that multi-column sorts pass
through in priority order.
##########
superset/views/datasource/schemas.py:
##########
@@ -83,6 +83,22 @@ class SamplesPayloadSchema(Schema):
metadata={"description": "Extra parameters to add to the query."},
allow_none=True,
)
+ orderby = fields.List(
+ fields.Tuple(
+ (
+ fields.Raw(allow_none=False),
+ fields.Boolean(),
+ )
+ ),
+ required=False,
+ allow_none=True,
+ metadata={
+ "description": "Expects a list of lists where the first element is
the "
+ "column name to sort by, and the second element is a boolean "
+ "(true = ascending).",
+ "example": [("my_col_1", False), ("my_col_2", True)],
+ },
Review Comment:
Fixed, the example now uses JSON-shaped lists instead of Python tuples.
##########
superset-frontend/src/components/Chart/DrillDetail/DrillDetailPane.tsx:
##########
@@ -308,7 +328,10 @@ export default function DrillDetailPane({
useEffect(() => {
if (!responseError && !isLoading && !resultsPages.has(pageIndex)) {
setIsLoading(true);
- const jsonPayload = getDrillPayload(formData, filters) ?? {};
+ const jsonPayload = {
+ ...getDrillPayload(formData, filters),
+ ...(orderby.length > 0 && { orderby }),
+ };
Review Comment:
Good catch, fixed with a sort-generation guard so a slower request from
before the sort change can't land after and overwrite the fresh page cache.
--
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]
