codeant-ai-for-open-source[bot] commented on code in PR #38080:
URL: https://github.com/apache/superset/pull/38080#discussion_r2991375972
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -167,6 +190,125 @@ const naturalSort: SortFunction = (as, bs) => {
return aArr.length - bArr.length;
};
+/**
+ * Precomputes aggregate sums for all group levels using safe numeric
conversion
+ */
+function buildGroupAggregates(
+ keys: string[][],
+ dataFunc: DataFunction,
+): GroupNode {
+ const root: GroupNode = { auto_agg_sum: 0 } as GroupNode;
+
+ const terminalKeys = keys.filter(
+ key =>
+ !keys.some(
+ ancestorKey =>
+ ancestorKey.length > key.length &&
+ key.every((segment, idx) => ancestorKey[idx] === segment),
+ ),
+ );
+ for (const key of terminalKeys) {
+ let current: GroupNode = root;
+
+ for (let i = 0; i < key.length - 1; i += 1) {
+ const segment = key[i];
+
+ if (!current[segment]) {
+ current[segment] = { auto_agg_sum: 0 } as GroupNode;
+ }
+
+ const childNode = current[segment] as GroupNode;
+ childNode.auto_agg_sum += toAggregationNumber(dataFunc(key, []));
+ current = childNode;
+ }
+ }
+
+ return root;
+}
+
+/**
+ * Creates a comparator function for hierarchical keys with subtotal awareness
+ */
+function createHierarchicalComparator(
+ groups: GroupNode,
+ top: boolean | undefined,
+ asc: boolean,
+ dataFunc: DataFunction,
+): (a: string[], b: string[]) => number {
+ const hierarchyMultiplier = top ? 1 : -1;
+ const valueMultiplier = asc ? 1 : -1;
+
+ return (a: string[], b: string[]) => {
+ const minLen = Math.min(a.length, b.length);
+ let currentGroup: GroupNode = groups;
+ let diffIndex = -1;
+
+ for (let i = 0; i < minLen; i++) {
+ if (a[i] !== b[i]) {
+ diffIndex = i;
+ break;
+ }
+ currentGroup = currentGroup[a[i]] as GroupNode;
+ }
+
+ if (diffIndex === -1 && a.length !== b.length) {
+ return (a.length < b.length ? -1 : 1) * hierarchyMultiplier;
+ }
+
+ const isLastLevelComparison =
+ diffIndex === -1 ||
+ (diffIndex === a.length - 1 && diffIndex === b.length - 1);
+
+ if (isLastLevelComparison) {
+ const valA = dataFunc(a, []) as string | number | null;
+ const valB = dataFunc(b, []) as string | number | null;
+
+ let result = naturalSort(valA, valB) * valueMultiplier;
+
+ if (result === 0) {
+ const lastA = a[a.length - 1] ?? '';
+ const lastB = b[b.length - 1] ?? '';
+ return lastA.localeCompare(lastB);
Review Comment:
**Suggestion:** The tie-break path for equal terminal values always uses
ascending lexical order, so descending sorts still return ascending order for
equal-valued items. Apply the same sort direction multiplier in the tie-breaker
to keep ordering consistent with the requested direction. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Descending value sort has ascending tie-breaks.
- ⚠️ Row/column order appears inconsistent for equal metrics.
```
</details>
```suggestion
return lastA.localeCompare(lastB) * valueMultiplier;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Select descending value sort in pivot controls (`value_z_to_a`) from
`superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:320-321`
(same for columns at `348-349`).
2. Props flow into PivotTable (`src/PivotTableChart.tsx:690-691`) and call
`groupingValueSort(..., asc=false)` in `PivotData.sortKeys()` at
`utilities.ts:1058-1063`
/ `1083-1088`.
3. In `createHierarchicalComparator()`, descending mode sets
`valueMultiplier = -1`
(`utilities.ts:239`), but terminal equal-values branch ignores it: `if
(result === 0) ...
return lastA.localeCompare(lastB)` (`268-272`).
4. Reuse terminal-equal dataset pattern from tests
(`test/react-pivottable/tableRenders.test.tsx:775-793`) and call
`groupingValueSort(keys,
createMockAggregator(data), false, false)`; equal-valued items remain
alphabetical
ascending, not descending-aligned tie-break.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts
**Line:** 271:271
**Comment:**
*Logic Error: The tie-break path for equal terminal values always uses
ascending lexical order, so descending sorts still return ascending order for
equal-valued items. Apply the same sort direction multiplier in the tie-breaker
to keep ordering consistent with the requested direction.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=f6500392cf87c71bce2b704233926c8988db99778bb3a5e6c53ba0ca7c48f106&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=f6500392cf87c71bce2b704233926c8988db99778bb3a5e6c53ba0ca7c48f106&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -167,6 +190,125 @@ const naturalSort: SortFunction = (as, bs) => {
return aArr.length - bArr.length;
};
+/**
+ * Precomputes aggregate sums for all group levels using safe numeric
conversion
+ */
+function buildGroupAggregates(
+ keys: string[][],
+ dataFunc: DataFunction,
+): GroupNode {
+ const root: GroupNode = { auto_agg_sum: 0 } as GroupNode;
+
+ const terminalKeys = keys.filter(
+ key =>
+ !keys.some(
+ ancestorKey =>
+ ancestorKey.length > key.length &&
+ key.every((segment, idx) => ancestorKey[idx] === segment),
+ ),
+ );
+ for (const key of terminalKeys) {
+ let current: GroupNode = root;
+
+ for (let i = 0; i < key.length - 1; i += 1) {
+ const segment = key[i];
+
+ if (!current[segment]) {
+ current[segment] = { auto_agg_sum: 0 } as GroupNode;
+ }
+
+ const childNode = current[segment] as GroupNode;
+ childNode.auto_agg_sum += toAggregationNumber(dataFunc(key, []));
+ current = childNode;
+ }
+ }
+
+ return root;
+}
+
+/**
+ * Creates a comparator function for hierarchical keys with subtotal awareness
+ */
+function createHierarchicalComparator(
+ groups: GroupNode,
+ top: boolean | undefined,
+ asc: boolean,
+ dataFunc: DataFunction,
+): (a: string[], b: string[]) => number {
+ const hierarchyMultiplier = top ? 1 : -1;
+ const valueMultiplier = asc ? 1 : -1;
+
+ return (a: string[], b: string[]) => {
+ const minLen = Math.min(a.length, b.length);
+ let currentGroup: GroupNode = groups;
+ let diffIndex = -1;
+
+ for (let i = 0; i < minLen; i++) {
+ if (a[i] !== b[i]) {
+ diffIndex = i;
+ break;
+ }
+ currentGroup = currentGroup[a[i]] as GroupNode;
+ }
+
+ if (diffIndex === -1 && a.length !== b.length) {
+ return (a.length < b.length ? -1 : 1) * hierarchyMultiplier;
+ }
+
+ const isLastLevelComparison =
+ diffIndex === -1 ||
+ (diffIndex === a.length - 1 && diffIndex === b.length - 1);
+
+ if (isLastLevelComparison) {
+ const valA = dataFunc(a, []) as string | number | null;
+ const valB = dataFunc(b, []) as string | number | null;
+
+ let result = naturalSort(valA, valB) * valueMultiplier;
+
+ if (result === 0) {
+ const lastA = a[a.length - 1] ?? '';
+ const lastB = b[b.length - 1] ?? '';
+ return lastA.localeCompare(lastB);
+ }
+ return result;
+ }
+
+ const segmentA = a[diffIndex];
+ const segmentB = b[diffIndex];
+
+ const nodeA = currentGroup[segmentA] as GroupNode | undefined;
+ const nodeB = currentGroup[segmentB] as GroupNode | undefined;
+
+ const sumA = nodeA?.auto_agg_sum ?? 0;
+ const sumB = nodeB?.auto_agg_sum ?? 0;
+
+ if (sumA === sumB) {
+ return segmentA.localeCompare(segmentB);
Review Comment:
**Suggestion:** When sibling group totals are equal, the comparator always
falls back to ascending lexical order, which violates descending sort semantics
for grouped comparisons. Multiply the fallback comparison by the current sort
direction to preserve consistent ordering. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Grouped descending sort keeps ascending tie order.
- ⚠️ Equal-total parent groups ignore selected sort direction.
```
</details>
```suggestion
return segmentA.localeCompare(segmentB) * valueMultiplier;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use grouped value sorting path (`rowOrder/colOrder = value_z_to_a`) which
is wired in
`controlPanel.tsx:311-349` and executed in `PivotData.sortKeys()` at
`utilities.ts:1058-1063` / `1083-1088`.
2. Comparator enters grouped (non-terminal) branch at
`createHierarchicalComparator()`
(`utilities.ts:276-289`) when first differing segment is above leaf level.
3. When sibling group aggregates are equal (`sumA === sumB` at `285`), code
always returns
`segmentA.localeCompare(segmentB)` (`286`) without applying descending
direction.
4. With realistic grouped keys (nested structures are covered by tests around
`tableRenders.test.tsx:848-889`) and equal parent totals, descending value
mode still
orders tied sibling groups alphabetically ascending.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts
**Line:** 286:286
**Comment:**
*Logic Error: When sibling group totals are equal, the comparator
always falls back to ascending lexical order, which violates descending sort
semantics for grouped comparisons. Multiply the fallback comparison by the
current sort direction to preserve consistent 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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=25fe71e4f7d61d6638f6c26163548752e8574d7941871d19fbd1209d94965297&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=25fe71e4f7d61d6638f6c26163548752e8574d7941871d19fbd1209d94965297&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]
