bito-code-review[bot] commented on code in PR #38080:
URL: https://github.com/apache/superset/pull/38080#discussion_r2824784546
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx:
##########
@@ -589,3 +590,184 @@ 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': 200,
+ 'Clothing|Pants|Jeans': 150,
+ };
+
+ groupingValueSort(keys, createMockAggregator(data), true, true);
+
+ expect(keys[0]).toEqual(['Clothing']);
+ expect(keys[1]).toEqual(['Clothing', 'Pants']);
+ expect(keys[2]).toEqual(['Clothing', 'Pants', 'Jeans']);
+ expect(keys[3]).toEqual(['Clothing', 'Shirts']);
+ expect(keys[4]).toEqual(['Clothing', 'Shirts', 'T-Shirt']);
+ expect(keys[5]).toEqual(['Electronics']);
+ expect(keys[6]).toEqual(['Electronics', 'Phones']);
+ expect(keys[7]).toEqual(['Electronics', 'Phones', 'Samsung']);
+ expect(keys[8]).toEqual(['Electronics', 'Phones', 'iPhone']);
+ expect(keys[9]).toEqual(['Electronics', 'Laptops']);
+ expect(keys[10]).toEqual(['Electronics', 'Laptops', 'MacBook']);
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Tests fail due to value vs alphabetical sorting
mismatch</b></div>
<div id="fix">
The added tests for groupingValueSort expect value-based sorting when group
aggregates are equal (e.g., for leaf nodes in hierarchical data), but the
current implementation falls back to alphabetical sorting of key segments. This
causes all the new hierarchical sorting tests to fail. For example, in the
'should place subtotal at top' test, it expects ['Region', 'City2'] before
['Region', 'City1'] based on values 50 < 100, but the code uses 'City2' >
'City1' alphabetically. The comparator needs to be fixed to prioritize value
comparison over alphabetical when sums are equal, matching the function's
'value sort' intent.
</div>
</div>
<small><i>Code Review Run #bcf18b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -92,6 +92,128 @@ 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 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++) {
+ 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,
+ 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++) {
+ 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:
<div>
<div id="suggestion">
<div id="issue"><b>Logic Error in Hierarchical Comparator</b></div>
<div id="fix">
The comparator logic for hierarchical sorting uses '<' instead of '<=' in
the conditions checking when a key ends, which fails to properly order
subtotals relative to their children at the maximum depth. This can result in
incorrect display of grouped data in pivot tables.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
if (depth + 1 <= maxDepth && depth + 1 >= b.length) {
return topBasis;
}
if (depth + 1 <= maxDepth && depth + 1 >= a.length) {
return -topBasis;
}
````
</div>
</details>
</div>
<small><i>Code Review Run #bcf18b</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
