superset icon indicating copy to clipboard operation
superset copied to clipboard

fix(pivot_table_v2): Pivot table cannot validly group rows and keep ordering

Open comdiv opened this issue 2 years ago • 8 comments

SUMMARY

Fix and improve value_a_z, value_z_a sort order for pivot table

  1. work on any size of cols and rows in pivot table options, groups them
  2. keep subtotals with their groups
  3. keep subtotals on required top/bottom position
  4. switches to a-z order in case of same numerical values

TESTING INSTRUCTIONS

Create any pivot v2 table with

  1. at least two rows (or/and to cols) and summable metric
  2. switch on subtotals
  3. try switch different orderings (all row/col groups should be consistent)
  4. try switch subtotal position (all row/col groups should be consistent and subtotals should be at expected place)

ADDITIONAL INFORMATION

Known issue - not tested for non monotonic summable aggregates with natural order (sum, count, avg). It's because it collect raw subtotals sumarrizing internal values. Some aggregate function (min, max for) could have unexpected behavior in sorting.

comdiv avatar Jul 01 '22 14:07 comdiv

/testenv up

zhaoyongjie avatar Jul 04 '22 02:07 zhaoyongjie

@zhaoyongjie Container image not yet published for this PR. Please try again when build is complete.

github-actions[bot] avatar Jul 04 '22 02:07 github-actions[bot]

@zhaoyongjie Ephemeral environment creation failed. Please check the Actions logs for details.

github-actions[bot] avatar Jul 04 '22 02:07 github-actions[bot]

@comdiv Thanks for the fixing. It can not work when I populate a column and select value asc by rows.

image

image

zhaoyongjie avatar Jul 04 '22 02:07 zhaoyongjie

As in previous version you should sum whole row, not just 'Abigail' because 'value order' means 'row value order'. Because you cannot express that Abigail column should be sorted while Alicia not. Sum of values used

comdiv avatar Jul 04 '22 04:07 comdiv

closing/reopening to see if this'll pass CI. It also still needs a review, of course.

rusackas avatar Feb 01 '24 23:02 rusackas

Codecov Report

Attention: Patch coverage is 0% with 47 lines in your changes missing coverage. Please review.

Project coverage is 68.74%. Comparing base (8d4994a) to head (e9d84df). Report is 3548 commits behind head on master.

Files Patch % Lines
...hart-pivot-table/src/react-pivottable/utilities.js 0.00% 47 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20588      +/-   ##
==========================================
+ Coverage   66.85%   68.74%   +1.88%     
==========================================
  Files        1753     1918     +165     
  Lines       65825    84047   +18222     
  Branches     7006    12771    +5765     
==========================================
+ Hits        44010    57781   +13771     
- Misses      20030    23468    +3438     
- Partials     1785     2798    +1013     
Flag Coverage Δ
javascript 59.83% <0.00%> (+7.87%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 02 '24 00:02 codecov[bot]

/testenv up

rusackas avatar Feb 08 '24 19:02 rusackas

/testenv up

rusackas avatar Jun 03 '24 18:06 rusackas

/testenv up

kgabryje avatar Jun 08 '24 18:06 kgabryje

@comdiv Would you mind rebasing your PR on the latest master? Maybe that will unblock the CI

kgabryje avatar Jun 09 '24 11:06 kgabryje

Also, for the sake of completeness, could you add before/after screenshots or recordings to the description?

kgabryje avatar Jun 09 '24 11:06 kgabryje

There are a bunch of CI tasks not running. Let's kick-start it by closing/reopening and see if that gets us through the full list. Then hopefully the ephemeral will work ¯_(ツ)_/¯

rusackas avatar Jun 10 '24 19:06 rusackas