fix(pivot_table_v2): refreshed pr for fixing sorting in pivot table #35322
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Redundant dataFunc calls in nested loops ▹ view | 🧠 Incorrect |
 |
Function has too many responsibilities ▹ view | 🧠 Not in standard |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| 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]; | ||
| } | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
It looks good. But I'd break the groupingValueSort function into smaller functions for better code readability.
| 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]; | ||
| } | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@rusackas @justinpark @kgabryje is anything happening with this? |
mateusz-osko
force-pushed
the
pr-20588-refresh-pivot-table-sorting-fix2
branch
from
1ec7e36 to
9dfbe7f
Compare
mateusz-osko
force-pushed
the
pr-20588-refresh-pivot-table-sorting-fix2
branch
from
9dfbe7f to
2cd07b1
Compare
|
Sorry it's been slow... we're all trying to catch up :D Running CI to see how that fares, for the moment, and adding our newest Committer (🎉) for a review. |
SBIN2010
added
the
🎪 ⚡ showtime-trigger-start
github-actions
bot
added
🎪 2cd07b1 🚦 building
github-actions
bot
added
🎪 2cd07b1 🚦 deploying
|
🎪 Showtime deployed environment on GHA for 2cd07b1 • Environment: http://54.213.20.62:8080 (admin/admin) |
|
Works great. |
|
@Kamkoz93 let us know if you can help address the above comnents, and we'll get this thing merged 🚀 |
github-actions
bot
removed
🎪 2cd07b1 📅 2025-11-05T04-17
5 participants
SUMMARY
Cherry-pick of #20588 (follow-up to #20564) to restore correct value-based ordering in Pivot Table v2 when sorting across multiple row/column groups.
Commits included:
5598f02 — normalize value-based ordering for multiple rows/cols
faba485 — lint fix for utilities.js
Created new PR based on old not merged due to some CI issues.
Fixed messed up ordering when sort descending on mertics aplied.
###BEFORE SCREENSHOTS###
###AFTER SCREENSHOTS###
