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

Option to show rollup grouping values only on the 'rolled-up' row #1555

Closed
rbasralian opened this issue Sep 28, 2023 · 8 comments · Fixed by #1636
Closed

Option to show rollup grouping values only on the 'rolled-up' row #1555

rbasralian opened this issue Sep 28, 2023 · 8 comments · Fixed by #1636
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@rbasralian
Copy link

A user has requested that rollups only display the aggregation key values in the "rolled-up" row, and not in the expanded rows. For example, instead of a rollup looking like this:

image

they'd prefer it like this:

image

Below are examples from other UIs.

ag-grid:
image

Excel:
image

Relates to #1483.

@rbasralian rbasralian added enhancement New feature or request triage Issue requires triage labels Sep 28, 2023
@vbabich vbabich added this to the Backlog milestone Oct 3, 2023
@vbabich vbabich removed the triage Issue requires triage label Oct 3, 2023
@vbabich
Copy link
Collaborator

vbabich commented Oct 3, 2023

We'd need a sticky row to display the aggregation key. This is a bit more complicated than just not showing the key.

@rbasralian
Copy link
Author

rbasralian commented Oct 4, 2023

Excel does not appear to sticky the row, FWIW

Example expanded pivot table:
image

Same thing after scrolling down a few rows — note the grouping values are no longer visible:

image

It is still probably more of a concern for us (since the data will usually be dynamic, so added/removed rows or rollup keys can make it harder to track the context of what you're looking at).

@rbasralian
Copy link
Author

From further discussion, what they really want is for the keys to be in their own column, as in the ag-grid example:

image

They are not concerned about keeping the aggregation keys 'sticky'.

@dsmmcken
Copy link
Contributor

As in dumping all the keys into one visual column?

That would make it hard to sort or filter the columns.

@rbasralian
Copy link
Author

The key columns could still exist independently as well (e.g. "Name" here: https://github.com/ag-grid/ag-grid/blob/latest/github-grid-demo.jpg)

@dsmmcken
Copy link
Contributor

If duplicating is desirable I guess that would work, to show it.

The other concern I have is that column is no longer an actual column of data, like it would be purely a visual column that is pretending to be one column of data, when it is actually several. So that could also be mis-leading. Do you have thoughts on that.

Like say you group on sym + exchange. Do you present a new column with a made-up name like "Roll-up" or "Group" as in that ag-grid containing the mixed ones, but then also duplicate in a sym/exchange columns as well still? That "Roll-up" or "Group" column wouldn't actually exist on the table though, and we've also added table width by duplicating.

Just trying to talk through all the problems if we want to arrive at a better solution.

@rbasralian
Copy link
Author

I agree that it's a bit unnatural for us to have a "fake column" that's really just a UI construct.

Keeping the original real columns (e.g. "Sym" and "Exchange") in addition to the synthetic "Group" column is actually what makes it tolerable to me; you can still use the real columns for filtering/sorting/seeing clearly which row you're looking at even if you've scrolled, and the "Group" column is just for expanding/collapsing rollup levels.

We'd have to add the "Group" column for CSV exports too. It's also a bit awkward to add it there, but I assume not too different to implement than doing it in the UI.

@mofojed mofojed self-assigned this Nov 6, 2023
@dsmmcken
Copy link
Contributor

dsmmcken commented Nov 7, 2023

To clarify the requested change for @mofojed

  1. When a rollup is created a new virtual column should be added called "Group", It should contain the just the top node value at each depth.
  2. The original columns should still exist.
  3. The context menu should be maintained for cells that have a value, and be proxied to operate on the actual columns (such a selecting a row and filtering by value, should filter the column that node would be from), and the expand depth menu should still be present.
  4. The virtual column should not be moveable, and always the first column. The virtual column would not have a quick filter box, and not be sortable.
  5. The column should still be hide able.

mofojed added a commit that referenced this issue Dec 15, 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 #1555
- Tested with rollup from docs:
```py
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 added a commit to deephaven/deephaven-core that referenced this issue Dec 22, 2023
Release notes https://github.com/deephaven/web-client-ui/releases/tag/v0.58.0

### Bug Fixes

* `figure_title` and `chart_title` were not mapped up correctly ([#1676](deephaven/web-client-ui#1676)) ([73e0b65](deephaven/web-client-ui@73e0b65)), closes [#1674](deephaven/web-client-ui#1674) [#1675](deephaven/web-client-ui#1675)


### Features

* "Group" column for rollup/tree tables ([#1636](deephaven/web-client-ui#1636)) ([ba1d51b](deephaven/web-client-ui@ba1d51b)), closes [#1555](deephaven/web-client-ui#1555)
* Add alt+click shortcut to copy cell and column headers ([#1694](deephaven/web-client-ui#1694)) ([4a8a81a](deephaven/web-client-ui@4a8a81a)), closes [deephaven/web-client-ui#1585](deephaven/web-client-ui#1585)
* Theming - Spectrum variable mapping and light theme ([#1680](deephaven/web-client-ui#1680)) ([2278697](deephaven/web-client-ui@2278697)), closes [#1669](deephaven/web-client-ui#1669) [#1539](deephaven/web-client-ui#1539)




---------

Co-authored-by: deephaven-internal <[email protected]>
Co-authored-by: mikebender <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants