codeant-ai-for-open-source[bot] commented on code in PR #38080:
URL: https://github.com/apache/superset/pull/38080#discussion_r2943345523
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -589,3 +590,186 @@ test('values ββfrom the 3rd level of the hierarchy with a
subtotal at the to
},
});
});
+
+type TestData = {
+ [key: string]: number | string | null;
+};
+
+const createMockAggregator =
+ (data: TestData) =>
+ (key: string[], _context: never[]): unknown => {
+ const keyStr = key.join('|');
+ return data[keyStr] ?? null;
+ };
+
+test('should sort flat keys in ascending order', () => {
+ const keys: string[][] = [['A'], ['C'], ['B']];
+ const data = {
+ A: 30,
+ B: 10,
+ C: 20,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, true);
+
+ expect(keys).toEqual([['B'], ['C'], ['A']]);
+});
+
+test('should sort flat keys in descending order', () => {
+ const keys: string[][] = [['A'], ['C'], ['B']];
+ const data = {
+ A: 30,
+ B: 10,
+ C: 20,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, false);
+
+ expect(keys).toEqual([['A'], ['C'], ['B']]);
+});
+
+test('should place subtotal at top when top=true and ascending', () => {
+ const keys: string[][] = [
+ ['Region', 'City1'],
+ ['Region'],
+ ['Region', 'City2'],
+ ];
+ const data = {
+ 'Region|City1': 100,
+ 'Region|City2': 50,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), true, true);
+
+ expect(keys[0]).toEqual(['Region']);
+ expect(keys[1]).toEqual(['Region', 'City2']);
+ expect(keys[2]).toEqual(['Region', 'City1']);
+});
+
+test('should place subtotal at bottom when top=false and descending', () => {
+ const keys: string[][] = [
+ ['Region', 'City1'],
+ ['Region'],
+ ['Region', 'City2'],
+ ];
+ const data = {
+ 'Region|City1': 100,
+ 'Region|City2': 50,
+ };
Review Comment:
**Suggestion:** This descending subtotal test also omits the subtotal value,
which makes ordering depend on `null` handling rather than true subtotal
sorting logic and can hide regressions. Include the subtotal aggregate value so
the test reflects real `PivotData.getAggregator(...).value()` behavior. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β οΈ Descending subtotal test exercises missing-key behavior, not runtime.
- β οΈ Sorting contract confidence reduced for pivot subtotal rows.
```
</details>
```suggestion
const data = {
Region: 150,
'Region|City1': 100,
'Region|City2': 50,
};
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Execute test `should place subtotal at bottom when top=false and
descending` in
`.../tableRenders.test.tsx:649-665`.
2. The fixture at `:655-658` omits `Region`, so `createMockAggregator`
(`:598-603`) yields
`null` for subtotal key `['Region']`.
3. Comparator in `groupingValueSort` (`utilities.ts:207-237`) evaluates
`valA/valB`
(`:187-189`); for descending (`orderBasis=-1`), `null` is pushed to the end
by
`naturalSort` null handling (`:44-49`), matching assertion for the wrong
reason.
4. Production path (`sortKeys` at `utilities.ts:1022-1069`) gets subtotal
aggregators from
`getAggregator`; fallback `null` only occurs when key not found
(`utilities.ts:1178-1181`), which this test artificially creates.
```
</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/test/react-pivottable/tableRenders.test.tsx
**Line:** 655:658
**Comment:**
*Logic Error: This descending subtotal test also omits the subtotal
value, which makes ordering depend on `null` handling rather than true subtotal
sorting logic and can hide regressions. Include the subtotal aggregate value so
the test reflects real `PivotData.getAggregator(...).value()` behavior.
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=4efea7af1d6355827584e7b80707aa0b48352c538cce9ce8137d2ef3f638882c&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=4efea7af1d6355827584e7b80707aa0b48352c538cce9ce8137d2ef3f638882c&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -92,6 +92,130 @@ const numberFormat = function (optsIn?:
NumberFormatOptions): Formatter {
};
};
+/**
+ * Safely converts any value to a number for aggregation purposes
+ * Handles null/undefined, strings, and non-numeric values
+ */
+function toAggregationNumber(value: unknown): number {
+ if (value == null) return 0;
+ if (typeof value === 'number') {
+ return Number.isNaN(value) ? 0 : value;
+ }
+ if (typeof value === 'string') {
+ const num = parseFloat(value.trim());
+ return Number.isNaN(num) ? 0 : num;
+ }
+ return 0;
+}
+
+type DataFunction = (key: string[], context: never[]) => unknown;
+
+interface GroupNode {
+ auto_agg_sum: number;
+ [childKey: string]: GroupNode | number;
+}
+
+/**
+ * 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;
+
+ for (const key of keys) {
Review Comment:
**Suggestion:** Group aggregate totals are built from every key, including
subtotal keys that already contain rolled-up values. Because
`rowKeys`/`colKeys` include both subtotal and leaf paths, parent totals get
counted multiple times and sibling group ordering can be wrong when hierarchy
depths differ. Build aggregates from terminal keys only to avoid double
counting. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β οΈ Value-based hierarchical row ordering becomes numerically incorrect.
- β οΈ Column group ordering can drift for deep hierarchies.
- β Pivot subtotal sorting contradicts user-selected metric ordering.
```
</details>
```suggestion
const terminalKeys = keys.filter(
key =>
!keys.some(
other =>
other.length > key.length &&
key.every((segment, idx) => other[idx] === segment),
),
);
for (const key of terminalKeys) {
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. In Pivot Table controls, enable row subtotals (`controlPanel.tsx:218`)
and choose
value-based row sorting (`controlPanel.tsx:311`, options include
`value_a_to_z`/`value_z_to_a`).
2. Render chart with 3+ row hierarchy levels so subtotal and leaf keys
coexist;
`processRecord()` adds every prefix key to `rowKeys` via loop
`ri=1..rowKey.length`
(`utilities.ts:1107-1113`), and each subtotal key has an aggregated value
(`utilities.ts:1118`).
3. Sorting path calls `groupingValueSort` from `sortKeys()` when value sort
is selected
(`utilities.ts:1019`, `1032-1046`), triggered from renderer flow `new
PivotData(...);
getRowKeys()` (`TableRenderers.tsx:366`, `372`).
4. In `buildGroupAggregates`, looping all keys (`utilities.ts:127`) adds
`dataFunc(key,
[])` to parent sums (`utilities.ts:138`), so subtotal keys and their
descendants both
contribute to ancestors, inflating parent totals and misordering sibling
groups.
```
</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:** 127:127
**Comment:**
*Logic Error: Group aggregate totals are built from every key,
including subtotal keys that already contain rolled-up values. Because
`rowKeys`/`colKeys` include both subtotal and leaf paths, parent totals get
counted multiple times and sibling group ordering can be wrong when hierarchy
depths differ. Build aggregates from terminal keys only to avoid double
counting.
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=c5b6bc04ecc4c39981e1cd93736cbb576670e715c0f99cebb4dbcb345025a5de&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=c5b6bc04ecc4c39981e1cd93736cbb576670e715c0f99cebb4dbcb345025a5de&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -589,3 +590,186 @@ test('values ββfrom the 3rd level of the hierarchy with a
subtotal at the to
},
});
});
+
+type TestData = {
+ [key: string]: number | string | null;
+};
+
+const createMockAggregator =
+ (data: TestData) =>
+ (key: string[], _context: never[]): unknown => {
+ const keyStr = key.join('|');
+ return data[keyStr] ?? null;
+ };
+
+test('should sort flat keys in ascending order', () => {
+ const keys: string[][] = [['A'], ['C'], ['B']];
+ const data = {
+ A: 30,
+ B: 10,
+ C: 20,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, true);
+
+ expect(keys).toEqual([['B'], ['C'], ['A']]);
+});
+
+test('should sort flat keys in descending order', () => {
+ const keys: string[][] = [['A'], ['C'], ['B']];
+ const data = {
+ A: 30,
+ B: 10,
+ C: 20,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, false);
+
+ expect(keys).toEqual([['A'], ['C'], ['B']]);
+});
+
+test('should place subtotal at top when top=true and ascending', () => {
+ const keys: string[][] = [
+ ['Region', 'City1'],
+ ['Region'],
+ ['Region', 'City2'],
+ ];
+ const data = {
+ 'Region|City1': 100,
+ 'Region|City2': 50,
+ };
Review Comment:
**Suggestion:** The subtotal test data omits the subtotal key value, so the
comparator receives `null` for the subtotal row and the test passes for the
wrong reason (null ordering) instead of validating subtotal-placement behavior.
Add the subtotal aggregate value to mirror real pivot data and make the test
assert the intended contract. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β οΈ Pivot subtotal sorting test validates null fallback path.
- β οΈ CI may miss subtotal-placement regressions in value sorting.
```
</details>
```suggestion
const data = {
Region: 150,
'Region|City1': 100,
'Region|City2': 50,
};
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run the subtotal test `should place subtotal at top when top=true and
ascending` in
`superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:631-647`.
2. In this test, `createMockAggregator`
(`.../tableRenders.test.tsx:598-603`) returns
`null` for key `['Region']` because `Region` is missing from fixture data
(`:637-640`).
3. `groupingValueSort` (`.../src/react-pivottable/utilities.ts:207-237`)
compares
`['Region']` vs children and falls back to `valA/valB` (`:187-189`), where
`naturalSort`
treats `null` as first (`:44-49`), so subtotal placement is driven by
null-ordering.
4. In real runtime, `PivotData.sortKeys()` uses `vr =
this.getAggregator(...).value()`
(`utilities.ts:1024-1059`), and subtotal keys are materialized in
`processRecord()`
(`:1123-1140`), so this test does not validate production-like subtotal
values.
```
</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/test/react-pivottable/tableRenders.test.tsx
**Line:** 637:640
**Comment:**
*Logic Error: The subtotal test data omits the subtotal key value, so
the comparator receives `null` for the subtotal row and the test passes for the
wrong reason (null ordering) instead of validating subtotal-placement behavior.
Add the subtotal aggregate value to mirror real pivot data and make the test
assert the intended contract.
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=4c2158a833f7bbfb325d4ce3a58b4d634c3a00e7e80b9d94e3af1034d34b2ff3&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=4c2158a833f7bbfb325d4ce3a58b4d634c3a00e7e80b9d94e3af1034d34b2ff3&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -589,3 +590,186 @@ test('values ββfrom the 3rd level of the hierarchy with a
subtotal at the to
},
});
});
+
+type TestData = {
+ [key: string]: number | string | null;
+};
+
+const createMockAggregator =
+ (data: TestData) =>
+ (key: string[], _context: never[]): unknown => {
+ const keyStr = key.join('|');
+ return data[keyStr] ?? null;
+ };
+
+test('should sort flat keys in ascending order', () => {
+ const keys: string[][] = [['A'], ['C'], ['B']];
+ const data = {
+ A: 30,
+ B: 10,
+ C: 20,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, true);
+
+ expect(keys).toEqual([['B'], ['C'], ['A']]);
+});
+
+test('should sort flat keys in descending order', () => {
+ const keys: string[][] = [['A'], ['C'], ['B']];
+ const data = {
+ A: 30,
+ B: 10,
+ C: 20,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, false);
+
+ expect(keys).toEqual([['A'], ['C'], ['B']]);
+});
+
+test('should place subtotal at top when top=true and ascending', () => {
+ const keys: string[][] = [
+ ['Region', 'City1'],
+ ['Region'],
+ ['Region', 'City2'],
+ ];
+ const data = {
+ 'Region|City1': 100,
+ 'Region|City2': 50,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), true, true);
+
+ expect(keys[0]).toEqual(['Region']);
+ expect(keys[1]).toEqual(['Region', 'City2']);
+ expect(keys[2]).toEqual(['Region', 'City1']);
+});
+
+test('should place subtotal at bottom when top=false and descending', () => {
+ const keys: string[][] = [
+ ['Region', 'City1'],
+ ['Region'],
+ ['Region', 'City2'],
+ ];
+ const data = {
+ 'Region|City1': 100,
+ 'Region|City2': 50,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, false);
+
+ expect(keys[0]).toEqual(['Region', 'City1']);
+ expect(keys[1]).toEqual(['Region', 'City2']);
+ expect(keys[2]).toEqual(['Region']);
+});
+
+test('should use alphabetical order for terminals with equal values', () => {
+ const keys: string[][] = [
+ ['Group', 'Apple'],
+ ['Group', 'Banana'],
+ ['Group', 'Cherry'],
+ ];
+ const data = {
+ 'Group|Apple': 50,
+ 'Group|Banana': 50,
+ 'Group|Cherry': 50,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, true);
+
+ expect(keys).toEqual([
+ ['Group', 'Apple'],
+ ['Group', 'Banana'],
+ ['Group', 'Cherry'],
+ ]);
+});
+
+test('should handle null values gracefully', () => {
+ const keys: string[][] = [['A'], ['B'], ['C']];
+ const data = {
+ A: 100,
+ B: null,
+ C: 50,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, true);
+ expect(keys).toEqual([['B'], ['C'], ['A']]);
+});
+
+test('should handle string numbers', () => {
+ const keys: string[][] = [['A'], ['B'], ['C']];
+ const data = {
+ A: '100',
+ B: '50',
+ C: '200',
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, false);
+ expect(keys).toEqual([['C'], ['A'], ['B']]);
+});
+
+test('should handle NaN values', () => {
+ const keys: string[][] = [['A'], ['B'], ['C']];
+ const data = {
+ A: 100,
+ B: NaN,
+ C: 50,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), false, true);
+ expect(keys).toEqual([['B'], ['C'], ['A']]);
+});
+
+test('should handle single key', () => {
+ const keys: string[][] = [['OnlyKey']];
+ const data = { OnlyKey: 42 };
+
+ groupingValueSort(keys, createMockAggregator(data), false, true);
+ expect(keys).toEqual([['OnlyKey']]);
+});
+
+test('should handle empty keys array', () => {
+ const keys: string[][] = [];
+ const data = {};
+
+ groupingValueSort(keys, createMockAggregator(data), false, true);
+ expect(keys).toEqual([]);
+});
+
+test('should handle product categories with subcategories', () => {
+ const keys: string[][] = [
+ ['Electronics'],
+ ['Electronics', 'Phones'],
+ ['Electronics', 'Phones', 'iPhone'],
+ ['Electronics', 'Phones', 'Samsung'],
+ ['Electronics', 'Laptops'],
+ ['Electronics', 'Laptops', 'MacBook'],
+ ['Clothing'],
+ ['Clothing', 'Shirts'],
+ ['Clothing', 'Shirts', 'T-Shirt'],
+ ['Clothing', 'Pants'],
+ ['Clothing', 'Pants', 'Jeans'],
+ ];
+ const data = {
+ 'Electronics|Phones|iPhone': 500,
+ 'Electronics|Phones|Samsung': 400,
+ 'Electronics|Laptops|MacBook': 1200,
+ 'Clothing|Shirts|T-Shirt': 1400,
+ 'Clothing|Pants|Jeans': 1150,
+ };
Review Comment:
**Suggestion:** The category hierarchy fixture only provides leaf values, so
comparisons between subtotal keys at the same depth can fall back to lexical
ordering and become inconsistent with actual runtime aggregation. Add
subtotal/intermediate aggregate values to make this test deterministic and
aligned with production data flow. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ Hierarchy sorting test may depend on lexical null tie-breaks.
- β οΈ Runtime-like subtotal comparisons are under-tested in CI.
```
</details>
```suggestion
const data = {
Electronics: 2100,
'Electronics|Phones': 900,
'Electronics|Phones|iPhone': 500,
'Electronics|Phones|Samsung': 400,
'Electronics|Laptops': 1200,
'Electronics|Laptops|MacBook': 1200,
Clothing: 2550,
'Clothing|Shirts': 1400,
'Clothing|Shirts|T-Shirt': 1400,
'Clothing|Pants': 1150,
'Clothing|Pants|Jeans': 1150,
};
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run test `should handle product categories with subcategories` in
`.../tableRenders.test.tsx:740-775`.
2. Fixture includes only leaf values (`:754-760`), so intermediate keys like
`['Electronics','Phones']` and subtotal keys like `['Electronics']` resolve
to `null` via
`createMockAggregator` (`:598-603`).
3. `groupingValueSort` comparator (`utilities.ts:207-237`) then uses value
fallback
(`:187-189`), and equal/null cases use lexical tiebreak (`:102-104`), which
diverges from
real pivot data where subtotals have computed values.
4. In production, `processRecord` builds subtotal keys and totals
(`utilities.ts:1123-1140`), and `sortKeys` sorts using those real values
(`:1024-1069`),
so leaf-only fixture under-represents runtime hierarchy sorting behavior.
```
</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/test/react-pivottable/tableRenders.test.tsx
**Line:** 754:760
**Comment:**
*Logic Error: The category hierarchy fixture only provides leaf values,
so comparisons between subtotal keys at the same depth can fall back to lexical
ordering and become inconsistent with actual runtime aggregation. Add
subtotal/intermediate aggregate values to make this test deterministic and
aligned with production data flow.
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=27a3b8e85ccdf4ba03568a61897f1eb63249134c5b4f768d3da090a7ec0a207f&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=27a3b8e85ccdf4ba03568a61897f1eb63249134c5b4f768d3da090a7ec0a207f&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -92,6 +92,130 @@ const numberFormat = function (optsIn?:
NumberFormatOptions): Formatter {
};
};
+/**
+ * Safely converts any value to a number for aggregation purposes
+ * Handles null/undefined, strings, and non-numeric values
+ */
+function toAggregationNumber(value: unknown): number {
+ if (value == null) return 0;
+ if (typeof value === 'number') {
+ return Number.isNaN(value) ? 0 : value;
+ }
+ if (typeof value === 'string') {
+ const num = parseFloat(value.trim());
+ return Number.isNaN(num) ? 0 : num;
+ }
+ return 0;
+}
+
+type DataFunction = (key: string[], context: never[]) => unknown;
+
+interface GroupNode {
+ auto_agg_sum: number;
+ [childKey: string]: GroupNode | number;
+}
+
+/**
+ * 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;
+
+ for (const key of keys) {
+ 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 topBasis = top ? 1 : -1;
+ const orderBasis = asc ? 1 : -1;
+
+ return (a: string[], b: string[]): number => {
+ let current: GroupNode = groups;
+ const maxDepth = Math.max(a.length, b.length) - 1;
+
+ for (let depth = 0; depth < maxDepth; depth += 1) {
+ const aSeg = a[depth];
+ const bSeg = b[depth];
+
+ if (aSeg !== bSeg) {
+ const nodeA = current[aSeg] as GroupNode | undefined;
+ const nodeB = current[bSeg] as GroupNode | undefined;
+ const sumA = nodeA?.auto_agg_sum ?? 0;
+ const sumB = nodeB?.auto_agg_sum ?? 0;
+
+ if (sumA === sumB) {
+ return aSeg.localeCompare(bSeg);
+ }
+ return (sumA > sumB ? 1 : -1) * orderBasis;
+ }
+ if (depth + 1 < maxDepth && depth + 1 >= b.length) {
+ return topBasis;
+ }
+ if (depth + 1 < maxDepth && depth + 1 >= a.length) {
+ return -topBasis;
+ }
Review Comment:
**Suggestion:** The subtotal placement checks are gated by `depth + 1 <
maxDepth`, so when comparing a subtotal and a child that differ by one level,
the comparator skips subtotal-on-top/bottom logic and falls back to value
comparison. This can place subtotal rows/columns in the wrong position relative
to children. Apply the length checks unconditionally inside the loop. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Subtotal top/bottom setting ignored in parent-child comparisons.
- β οΈ Row and column hierarchy order appears inconsistent.
- β οΈ Users see unpredictable subtotal placement under value sorting.
```
</details>
```suggestion
if (depth + 1 >= b.length) {
return topBasis;
}
if (depth + 1 >= a.length) {
return -topBasis;
}
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Configure pivot with subtotals enabled (`controlPanel.tsx:218`/`242`) and
subtotal
position top or bottom (`controlPanel.tsx:367`/`384`), then pick value-based
sort
(`controlPanel.tsx:311`/`339`).
2. Renderer instantiates `PivotData` and requests sorted keys
(`TableRenderers.tsx:366`,
`372-373`), which enters `sortKeys()` and calls `groupingValueSort`
(`utilities.ts:1019`,
`1033`, `1058`).
3. Compare a parent key and direct child key (e.g., `['Region']` vs
`['Region','City']`):
comparator loop has `maxDepth=1` (`utilities.ts:160`), so guard `depth + 1 <
maxDepth` is
false and subtotal-position branch is skipped (`utilities.ts:177-182`).
4. Comparator then falls back to value comparison (`utilities.ts:187-195`)
instead of
forced subtotal placement, so parent/child order can violate selected
subtotal position.
```
</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:** 177:182
**Comment:**
*Logic Error: The subtotal placement checks are gated by `depth + 1 <
maxDepth`, so when comparing a subtotal and a child that differ by one level,
the comparator skips subtotal-on-top/bottom logic and falls back to value
comparison. This can place subtotal rows/columns in the wrong position relative
to children. Apply the length checks unconditionally inside the loop.
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=b059c7f10504e4100371280c308361206bc9f7af4d8304c4666d32caa2b2e9ae&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38080&comment_hash=b059c7f10504e4100371280c308361206bc9f7af4d8304c4666d32caa2b2e9ae&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]
