korbit-ai[bot] commented on code in PR #35322:
URL: https://github.com/apache/superset/pull/35322#discussion_r2387685819
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.js:
##########
@@ -129,6 +129,101 @@ const naturalSort = (as, bs) => {
return a.length - b.length;
};
+/**
+ * Targets: https://github.com/apache/superset/issues/20564 issue
+ * Topic: order by value (asc/desc)
+ * Fixed problems:
+ * 1. valid ordering with multiple rows/columns (groupping due to subtotal
values)
+ * 2. guarantee for normal `subtotal` row/col position both
top(left)/bottom(right) in any asc/desc case
+ * 3. fixed a-z order in equal group/row aggregate values
+ * KNOWN ISSUES: now is applyable only for monotonic sumable aggregates (sum,
count, avg), not for min/max
+ * @param keys - [[key,...],...] - array of row or col keys
+ * @param dataFunc - function to retrieve data by key (for natural sum order)
+ * @param top - bool - top/left (true) or bottom/right (false) position of
subtotal rows
+ * @param asc - ascending(true) or descending(false) sort
+ * NOTE: we REQUIRE to use `asc` parameter and not just
`(+|-)groupingValueSort(keys, dataFunc, top)`
+ * because top/bottom position should be evaluated correctly without
dependency of asc/desc
+ */
+const groupingValueSort = (keys, dataFunc, top, asc) => {
+ // precalculate subtotal for every group and subgroup
+ const groups = {};
+ for (let i = 0; i < keys.length; i += 1) {
+ const rk = keys[i];
+ let current = groups;
+ // we should not visit whole key ignore last key part (only groups)
+ for (let ki = 0; ki < rk.length - 1; ki += 1) {
+ const k = rk[ki];
+ if (!current[k]) {
+ //
+ current[k] = { auto_agg_sum: 0 };
+ }
+ current[k].auto_agg_sum += dataFunc(rk, []);
+ current = current[k];
+ }
+ }
Review Comment:
### Redundant dataFunc calls in nested loops <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The groupingValueSort function calls dataFunc(rk, []) repeatedly for the
same key within nested loops, causing O(nĀ²) time complexity where n is the
number of keys.
###### Why this matters
This creates unnecessary computational overhead as the same aggregated value
is calculated multiple times for each key across different group levels,
leading to poor performance with large datasets.
###### Suggested change ā *Feature Preview*
Cache the dataFunc result outside the inner loop:
```javascript
for (let i = 0; i < keys.length; i += 1) {
const rk = keys[i];
const cachedValue = dataFunc(rk, []);
let current = groups;
for (let ki = 0; ki < rk.length - 1; ki += 1) {
const k = rk[ki];
if (!current[k]) {
current[k] = { auto_agg_sum: 0 };
}
current[k].auto_agg_sum += cachedValue;
current = current[k];
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/59629d4e-b461-4e1b-bafd-06bff6e44e01/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/59629d4e-b461-4e1b-bafd-06bff6e44e01?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/59629d4e-b461-4e1b-bafd-06bff6e44e01?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/59629d4e-b461-4e1b-bafd-06bff6e44e01?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/59629d4e-b461-4e1b-bafd-06bff6e44e01)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6dfdcc8d-1008-4093-aa0c-45d37fc25bd8 -->
[](6dfdcc8d-1008-4093-aa0c-45d37fc25bd8)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.js:
##########
@@ -129,6 +129,101 @@ const naturalSort = (as, bs) => {
return a.length - b.length;
};
+/**
+ * Targets: https://github.com/apache/superset/issues/20564 issue
+ * Topic: order by value (asc/desc)
+ * Fixed problems:
+ * 1. valid ordering with multiple rows/columns (groupping due to subtotal
values)
+ * 2. guarantee for normal `subtotal` row/col position both
top(left)/bottom(right) in any asc/desc case
+ * 3. fixed a-z order in equal group/row aggregate values
+ * KNOWN ISSUES: now is applyable only for monotonic sumable aggregates (sum,
count, avg), not for min/max
+ * @param keys - [[key,...],...] - array of row or col keys
+ * @param dataFunc - function to retrieve data by key (for natural sum order)
+ * @param top - bool - top/left (true) or bottom/right (false) position of
subtotal rows
+ * @param asc - ascending(true) or descending(false) sort
+ * NOTE: we REQUIRE to use `asc` parameter and not just
`(+|-)groupingValueSort(keys, dataFunc, top)`
+ * because top/bottom position should be evaluated correctly without
dependency of asc/desc
+ */
+const groupingValueSort = (keys, dataFunc, top, asc) => {
+ // precalculate subtotal for every group and subgroup
+ const groups = {};
+ for (let i = 0; i < keys.length; i += 1) {
+ const rk = keys[i];
+ let current = groups;
+ // we should not visit whole key ignore last key part (only groups)
+ for (let ki = 0; ki < rk.length - 1; ki += 1) {
+ const k = rk[ki];
+ if (!current[k]) {
+ //
+ current[k] = { auto_agg_sum: 0 };
+ }
+ current[k].auto_agg_sum += dataFunc(rk, []);
+ current = current[k];
+ }
+ }
Review Comment:
### Function has too many responsibilities <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The groupingValueSort function has multiple responsibilities - it builds a
hierarchical data structure, performs sorting, and handles comparison logic all
in one large function.
###### Why this matters
Violating the Single Responsibility Principle makes the code harder to
maintain, test and reuse. Each major operation should be extracted into its own
function with a clear purpose.
###### Suggested change ā *Feature Preview*
Extract the group building and comparison logic into separate functions:
```javascript
const buildGroupHierarchy = (keys, dataFunc) => {
const groups = {};
// Group building logic here
return groups;
}
const compareGroups = (a, b, groups, top, asc) => {
// Comparison logic here
}
const groupingValueSort = (keys, dataFunc, top, asc) => {
const groups = buildGroupHierarchy(keys, dataFunc);
keys.sort((a, b) => compareGroups(a, b, groups, top, asc));
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4915b225-b57b-4589-8c97-960c123bbc32/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4915b225-b57b-4589-8c97-960c123bbc32?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4915b225-b57b-4589-8c97-960c123bbc32?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4915b225-b57b-4589-8c97-960c123bbc32?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4915b225-b57b-4589-8c97-960c123bbc32)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:04a0cf7c-050c-4158-8d63-0e227347bfc4 -->
[](04a0cf7c-050c-4158-8d63-0e227347bfc4)
--
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]
