codeant-ai-for-open-source[bot] commented on code in PR #40907:
URL: https://github.com/apache/superset/pull/40907#discussion_r3382267178
##########
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:
**Suggestion:** Initializing post-processing detection to `false`
temporarily enables server-side sorting before async detection finishes, which
can send incorrect `orderby + row_limit` requests for post-processed queries.
Start in a disabled/unknown state and enable server sort only after detection
confirms no post-processing. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Post-processed charts can briefly issue unsafe sorted queries.
- ⚠️ Early header clicks may show incorrect cumulative/rolling values.
- ⚠️ Behavior contradicts documented intent to disable server sort.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a chart whose query uses post-processing (e.g., a cumulative or
rolling
operation) so that the backend query context includes a non-empty
`post_processing` list
alongside `orderby`, as reflected in
`tests/unit_tests/common/test_query_context_processor.py:1086-1092` (showing
`"orderby":
[("Net Amount In", False)], "post_processing": [...]` in the same query).
2. Open the Explore results data panel for that chart: `DataTablesPane`
instantiates
`useResultsPane` (`DataTablesPane.tsx:185-198`), which initializes
`hasPostProcessing` to
`false` (`useResultsPane.tsx:73`) and immediately kicks off two effects: one
that fetches
results via `getChartDataRequest` (`useResultsPane.tsx:108-147`) and one that
asynchronously detects `post_processing` by calling
`buildV1ChartDataPayload` and updating
`hasPostProcessing` based on `payload.queries[].post_processing`
(`useResultsPane.tsx:155-186`).
3. As soon as the first result set comes back, `isLoading` is set to false
and
`resultResp` is populated (`useResultsPane.tsx:130-145`), causing the hook
to return
`SingleQueryResultPane` instances (`useResultsPane.tsx:228-258`); until the
`buildV1ChartDataPayload` promise resolves and `setHasPostProcessing(true)`
runs,
`hasPostProcessing` remains `false`.
4. In that window, because `!hasPostProcessing && result.rowcount >=
effectiveRowLimit` is
true, `onServerSort={handleServerSort}` is passed down
(`useResultsPane.tsx:247-253` →
`SingleQueryResultPane.tsx:95-105` → `GridTable/index.tsx:49-65`), so
clicking a column
header sends a server-side `orderby` while `row_limit` is applied to the raw
SQL feeding
post-processing, potentially truncating or reordering the input rows for
cumulative/rolling operations and yielding incorrect post-processed values,
even though
the design comment in `useResultsPane.tsx:68-72` indicates post-processed
queries should
never be server-sorted.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=889df2dc60fb4ff18cfb853c48673084&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=889df2dc60fb4ff18cfb853c48673084&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx
**Line:** 73:73
**Comment:**
*Incorrect Condition Logic: Initializing post-processing detection to
`false` temporarily enables server-side sorting before async detection
finishes, which can send incorrect `orderby + row_limit` requests for
post-processed queries. Start in a disabled/unknown state and enable server
sort only after detection confirms no post-processing.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40907&comment_hash=e2c9ea832abc0e76bbef4823ce53725686f6ef9727ca5b4f7b3621f6acf8f542&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40907&comment_hash=e2c9ea832abc0e76bbef4823ce53725686f6ef9727ca5b4f7b3621f6acf8f542&reaction=dislike'>👎</a>
##########
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:
**Suggestion:** Omitting `orderby` when the sort model becomes empty makes a
"clear sort" action indistinguishable from "no explicit sort provided".
Downstream query builders then re-apply their metric-based default ordering, so
the grid can show `aria-sort="none"` while rows are still server-sorted. Keep
an explicit empty `orderby` (or track a "user sorted" flag) so clearing sort
actually clears server ordering. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Results pane clear-sort reverts to metric default ordering.
- ⚠️ Grid header shows no sort while backend still orders.
- ⚠️ Users may misinterpret "unsorted" as raw row order.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Explore view and ensure the results data panel is visible so that
`DataTablesPane` renders (entry component at
`superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx:80-90`).
2. With the "Results" tab active, `DataTablesPane` calls `useResultsPane`
(`DataTablesPane.tsx:185-198`), which builds `cappedFormData` without an
`orderby` field
when `orderby` state is empty (`useResultsPane.tsx:85-92`).
3. Because no `orderby` is provided in `cappedFormData`,
`getChartDataRequest`
(`useResultsPane.tsx:121-128`) sends form data without `orderby`, and the
backend query
builders apply their metric-based default ordering as verified by
`tests/unit_tests/mcp_service/chart/test_chart_helpers.py:860-868` (default
`orderby` from
first metric when omitted).
4. Click a grid column header to sort: `GridTable` translates the Ag-Grid
sort state into
an `orderby` array and calls `onServerSort(orderby)`
(`GridTable/index.tsx:57-64`), which
`SingleQueryResultPane` passes through (`SingleQueryResultPane.tsx:97-105`)
to
`useResultsPane.handleServerSort` (`useResultsPane.tsx:104-106`), updating
`orderby` so
the next request includes an explicit `orderby`
(`useResultsPane.tsx:85-92`); clicking the
same header again to clear the sort causes `GridTable` to send an empty `[]`,
`handleServerSort` resets `orderby` to `[]`, `cappedFormData` drops the
`orderby` field
entirely (`...(orderby.length > 0 && { orderby })` at
`useResultsPane.tsx:91`), and the
subsequent server request again uses the backend's implicit metric-based
ordering while
the grid's header shows no active sort.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9f595c3d60134618b21fb7952ce37b83&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9f595c3d60134618b21fb7952ce37b83&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx
**Line:** 91:91
**Comment:**
*Logic Error: Omitting `orderby` when the sort model becomes empty
makes a "clear sort" action indistinguishable from "no explicit sort provided".
Downstream query builders then re-apply their metric-based default ordering, so
the grid can show `aria-sort="none"` while rows are still server-sorted. Keep
an explicit empty `orderby` (or track a "user sorted" flag) so clearing sort
actually clears server ordering.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40907&comment_hash=4c39a90efea2100d178bb0dce84cbea97d241e292ee1d37fec074daa026265f7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40907&comment_hash=4c39a90efea2100d178bb0dce84cbea97d241e292ee1d37fec074daa026265f7&reaction=dislike'>👎</a>
##########
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:
**Suggestion:** The new server-sort path can fire multiple overlapping
requests (e.g., rapid header clicks), but there is no request sequencing or
cancellation, so a slower older response can overwrite a newer sort result. Add
a request id/AbortController guard and only apply the latest response to
prevent stale data races. [race condition]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Results pane can display rows for an old sort.
- ⚠️ Grid sort indicators may not match underlying row subset.
- ⚠️ Fast header clicks can cause confusing result "flapping."
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In Explore, open the results data panel so `DataTablesPane` renders and
calls
`useResultsPane` (`DataTablesPane.tsx:80-90` and `185-198`), and ensure the
result set is
large enough that `result.rowcount >= effectiveRowLimit` so `onServerSort`
is wired
(`useResultsPane.tsx:82-84` and `232-255`).
2. With a non–post-processed chart (so `hasPostProcessing` is false),
`SingleQueryResultPane` passes the `onServerSort` callback from
`useResultsPane` down to
`GridTable` (`SingleQueryResultPane.tsx:49-65` and `95-105`), where Ag-Grid's
`onSortChanged` builds an `orderby` array from the current column sort state
and invokes
`onServerSort(orderby)` (`GridTable/index.tsx:49-65`).
3. Rapidly click two different column headers (or repeatedly toggle sort) so
that
`GridTable` fires multiple `onSortChanged` events in quick succession; for
each event,
`handleServerSort` in `useResultsPane` updates `orderby`
(`useResultsPane.tsx:104-106`),
which changes `cappedFormData` (`useResultsPane.tsx:85-93`) and triggers the
`useEffect`
that starts a new `getChartDataRequest` without cancelling the previous one
(`useResultsPane.tsx:108-147`).
4. If the earlier request's network response arrives after the later one
(e.g., the "old"
sort query is slower), its `.then` handler still calls `setResultResp(...)`
and
`cache.set(cappedFormData, json.result)` (`useResultsPane.tsx:130-134`), and
its
`.finally` sets `isLoading` false (`useResultsPane.tsx:143-145`),
overwriting the more
recent sort's data because there is no AbortController or request-id check
here (unlike
the guarded, abortable flow in `exploreJSON` at `chartAction.ts:221-249` and
the
stale-response dropping at `chartAction.ts:278-79`), leaving the grid
showing sort UI for
the last header clicked but row data corresponding to an earlier `orderby`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cf1502d729ac4c049ac3c8974ee0075d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cf1502d729ac4c049ac3c8974ee0075d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx
**Line:** 104:106
**Comment:**
*Race Condition: The new server-sort path can fire multiple overlapping
requests (e.g., rapid header clicks), but there is no request sequencing or
cancellation, so a slower older response can overwrite a newer sort result. Add
a request id/AbortController guard and only apply the latest response to
prevent stale data races.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40907&comment_hash=84f83d8241aae09041c54eb587352f338a50811b3f979a25ee2a1c9f5c854151&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40907&comment_hash=84f83d8241aae09041c54eb587352f338a50811b3f979a25ee2a1c9f5c854151&reaction=dislike'>👎</a>
--
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]
