alexandrusoare commented on code in PR #38080:
URL: https://github.com/apache/superset/pull/38080#discussion_r2981157261
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -167,6 +190,115 @@ 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(
+ other =>
+ other.length > key.length &&
+ key.every((segment, idx) => other[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 {
Review Comment:
The code in this function seems a bit dense and hard to follow, is there
something that we can do to simplify the logic?
##########
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:
Is this relevant?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -167,6 +190,115 @@ 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(
+ other =>
Review Comment:
I think `other` might be a bit too generic here — maybe we could use a more
descriptive name like `candidate` or `extension` to better reflect its role in
the comparison?
--
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]
