Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: "Group" column for rollup/tree tables #1636

Merged
merged 15 commits into from
Dec 15, 2023

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Nov 10, 2023

  • Have a virtual column that just shows the rolled up value
  • Displays the value from the "source" cell (ie. the value from the grouping column at that depth)
  • The context menu shows the filter options for the source cell, so you can filter by options
  • Virtual column you cannot use quick filters or sort on
  • Closes Option to show rollup grouping values only on the 'rolled-up' row #1555
  • Tested with rollup from docs:
from deephaven import read_csv, agg

insurance = read_csv("https://media.githubusercontent.com/media/deephaven/examples/main/Insurance/csv/insurance.csv")

agg_list = [agg.avg(cols=["bmi", "expenses"])]
by_list = ["region", "age"]

test_rollup = insurance.rollup(aggs=[], by=by_list, include_constituents=True)
insurance_rollup = insurance.rollup(aggs=agg_list, by=by_list, include_constituents=True)

@mofojed mofojed self-assigned this Nov 10, 2023
Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Attention: 144 lines in your changes are missing coverage. Please review.

Comparison is base (9d519e1) 46.74% compared to head (8187250) 46.65%.

Files Patch % Lines
packages/iris-grid/src/IrisGridTreeTableModel.ts 1.88% 52 Missing ⚠️
...d/src/mousehandlers/IrisGridContextMenuHandler.tsx 0.00% 38 Missing ⚠️
...ckages/iris-grid/src/IrisGridTableModelTemplate.ts 11.11% 16 Missing ⚠️
packages/iris-grid/src/IrisGrid.tsx 0.00% 12 Missing ⚠️
packages/iris-grid/src/IrisGridModel.ts 25.00% 12 Missing ⚠️
packages/iris-grid/src/CrossColumnSearch.tsx 0.00% 7 Missing ⚠️
packages/iris-grid/src/ColumnStatistics.tsx 0.00% 3 Missing ⚠️
packages/iris-grid/src/IrisGridProxyModel.ts 50.00% 2 Missing ⚠️
.../mousehandlers/IrisGridColumnSelectMouseHandler.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1636      +/-   ##
==========================================
- Coverage   46.74%   46.65%   -0.10%     
==========================================
  Files         606      606              
  Lines       36762    36852      +90     
  Branches     9227     9255      +28     
==========================================
+ Hits        17184    17192       +8     
- Misses      19526    19608      +82     
  Partials       52       52              
Flag Coverage Δ
unit 46.65% <12.19%> (-0.10%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The organize columns menu shows the group column name (although that may need to be unique right now and can be fixed separately)

image

I tried to add an aggregate column and it crashed my table

Linker can connect to/from the group column. Doesn't crash the table, but does create an invalid textbox if it's the receiving side of a link

packages/iris-grid/src/IrisGridTreeTableModel.ts Outdated Show resolved Hide resolved
@mattrunyon
Copy link
Collaborator

Also shows up in the find column menu

image

@mattrunyon
Copy link
Collaborator

Still an issue that you can link into/out of the virtual column. At least for the link into, it creates an invalid filter on the column which is unclearable unless you use the clear all filters keyboard shortcut (ctrl+e).

Linking out works, but feels odd since it's the key column and can have multiple types. I'd say just don't make it a linkable column at all

Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to fix the linker issue

mattrunyon
mattrunyon previously approved these changes Dec 11, 2023
… cell

- Proxies to getting the menu items from the actual cell
- It was duplicating the constituent data, which was not what we wanted
- Can clear column filter from line up
- Should actually clear _all_ group filters and have a different name...
- Returns the range of columns that should be cleared when clearing the filter on this column
- "Key column" is more descriptive
- Add support for `DisplayColumn` with a `displayName` prop
- Clean up some comments
- Just some minor cleanup
- Now it doesn't crash when you add aggregations
- Don't show the proxy column in the Organize Columns, Search Columns, or aggregations sections
- Fix an issue with rehydrating tree tables with an aggregation set (table.size can be negative on initial load)
@mofojed mofojed merged commit ba1d51b into deephaven:main Dec 15, 2023
4 of 5 checks passed
@mofojed mofojed deleted the 1555-rollup-groupings branch December 15, 2023 17:15
@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to show rollup grouping values only on the 'rolled-up' row
2 participants