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

Aggregate Columns ignores columns selection #1355

Closed
dsmmcken opened this issue Jun 7, 2023 · 5 comments · Fixed by #1648 or deephaven/deephaven-core#4930
Closed

Aggregate Columns ignores columns selection #1355

dsmmcken opened this issue Jun 7, 2023 · 5 comments · Fixed by #1648 or deephaven/deephaven-core#4930
Assignees
Labels
bug Something isn't working

Comments

@dsmmcken
Copy link
Contributor

dsmmcken commented Jun 7, 2023

Description

image

Steps to reproduce

Using the following table:

from deephaven import empty_table, time_table

simple_ticking = time_table("PT00:00:01").update([
        "MyString=new String(`a`+i)",
        "MyInt=new Integer(i)",
        "MyLong=new Long(i)",
        "MyDouble=new Double(i+i/10)",
        "MyFloat=new Float(i+i/10)",
        "MyBoolean=new Boolean(i%2==0)",
        "MyChar= new Character((char) ((i%26)+97))",
        "MyShort=new Short(Integer.toString(i%32767))",
        "MyByte= new java.lang.Byte(Integer.toString(i%127))"])

complex_ticking = simple_ticking.update([
        "MyBigDecimal= new java.math.BigDecimal(i+i/10)",
        "MyBigInteger= new java.math.BigInteger(Integer.toString(i))"
])                          
  1. select from table options sidebar "Aggregate Columns"
  2. Select sum from drop down and click add aggregation
  3. From the sum button click the pencil to edit aggregations
  4. De-select a number of columns

Expected results

Sum is only computed for the selected columns

Actual results

Sum is selected for all columns

Versions
Vermillion and

Engine Version: 0.26.0
Web UI Version: 0.40.4
Java Version: 17.0.7
Barrage Version: 0.5.0

@dsmmcken dsmmcken added bug Something isn't working triage Issue requires triage labels Jun 7, 2023
@mofojed
Copy link
Member

mofojed commented Jun 7, 2023

Seems to be broken in Silverheels as well. The issue isn't evident if you pass in a different type of operation (e.g. Max) instead of Sum, but the aggregation is actually still happening on the server. The defaultOperation being set is SUM: https://github.com/deephaven/deephaven-core/blob/ede9c3977a86f4558c8495a036d3dc487e68e475/web/client-api/src/main/java/io/deephaven/web/client/api/JsTotalsTableConfig.java#L80

UI can't set that to SKIP because that operation is deprecated (it throws an error if we try to use it): https://github.com/deephaven/deephaven-core/blob/ede9c3977a86f4558c8495a036d3dc487e68e475/web/client-api/src/main/java/io/deephaven/web/client/api/tree/enums/JsAggregationOperation.java#L34

UI could set FIRST or some other inexpensive operation instead, but we'll have the same issue if you choose the FIRST aggregation when you only want FIRST on one.

@niloc132 how are we supposed to use this? Seems like having the defaultOperation = SUM isn't really what we want in general?

@vbabich
Copy link
Collaborator

vbabich commented Jun 14, 2023

@niloc132 will add the SKIP operation and the UI will need to set that as a default.

@vbabich vbabich added this to the June 2023 milestone Jun 14, 2023
@vbabich vbabich removed the triage Issue requires triage label Jun 14, 2023
@niloc132
Copy link
Member

Is there a dhc issue for this? I'm not finding one...

@mofojed
Copy link
Member

mofojed commented Jun 16, 2023

@niloc132 Would need to split off one for core

@mofojed
Copy link
Member

mofojed commented Jul 13, 2023

@niloc132 finally created one for core: deephaven/deephaven-core#4182

@mofojed mofojed modified the milestones: June 2023, July 2023 Jul 13, 2023
@mofojed mofojed modified the milestones: July 2023, August 2023 Aug 28, 2023
@mofojed mofojed modified the milestones: August 2023, September 2023, October 2023 Sep 11, 2023
mofojed added a commit that referenced this issue Nov 23, 2023
- Previously we were unable to remove columns from an aggregate
selection
- Default to `Skip` operation unless the user has actually applied one
or more operations for a column
- Requires deephaven/deephaven-core#4780
- Tested using the steps in the description of #1355
- Fixes #1355
mofojed pushed a commit to deephaven/deephaven-core that referenced this issue Dec 11, 2023
Release notes https://github.com/deephaven/web-client-ui/releases/tag/v0.56.0

# [0.56.0](deephaven/web-client-ui@v0.55.0...v0.56.0) (2023-12-11)


### Bug Fixes

* add right margin to <Button kind='inline'/> using icons ([#1664](deephaven/web-client-ui#1664)) ([fd8a6c6](deephaven/web-client-ui@fd8a6c6))
* adjust filter bar colour ([#1666](deephaven/web-client-ui#1666)) ([4c0200e](deephaven/web-client-ui@4c0200e))
* convert organize columns component to purecomponent ([#1653](deephaven/web-client-ui#1653)) ([8ddc114](deephaven/web-client-ui@8ddc114)), closes [#1650](deephaven/web-client-ui#1650)
* Default to `Skip` operation instead of `Sum` operation ([#1648](deephaven/web-client-ui#1648)) ([6083173](deephaven/web-client-ui@6083173)), closes [#1355](deephaven/web-client-ui#1355) [#1355](deephaven/web-client-ui#1355)
* Fix button snapshots ([#1655](deephaven/web-client-ui#1655)) ([c0cc966](deephaven/web-client-ui@c0cc966))
* popper blur in styleguide ([#1672](deephaven/web-client-ui#1672)) ([6fa2204](deephaven/web-client-ui@6fa2204))
* Unable to delete selected rows in some input tables ([#1678](deephaven/web-client-ui#1678)) ([1e71550](deephaven/web-client-ui@1e71550)), closes [#1677](deephaven/web-client-ui#1677)


### Features

* Add embed-widget ([#1668](deephaven/web-client-ui#1668)) ([1b06675](deephaven/web-client-ui@1b06675)), closes [#1629](deephaven/web-client-ui#1629)
* forward and back button for organize column search ([#1641](deephaven/web-client-ui#1641)) ([89f2be5](deephaven/web-client-ui@89f2be5)), closes [#1529](deephaven/web-client-ui#1529)
* Tables that have names starting with underscore do not auto-launch from console ([#1656](deephaven/web-client-ui#1656)) ([21131fe](deephaven/web-client-ui@21131fe)), closes [#1549](deephaven/web-client-ui#1549) [#1410](deephaven/web-client-ui#1410)
* theme fontawesome icon size wrapped in spectrum icons ([#1658](deephaven/web-client-ui#1658)) ([2aa8cef](deephaven/web-client-ui@2aa8cef))
* Theme Selector ([#1661](deephaven/web-client-ui#1661)) ([5e2be64](deephaven/web-client-ui@5e2be64)), closes [#1660](deephaven/web-client-ui#1660)
* Theming - Bootstrap ([#1603](deephaven/web-client-ui#1603)) ([88bcae0](deephaven/web-client-ui@88bcae0))
* Theming - Inline svgs ([#1651](deephaven/web-client-ui#1651)) ([1e40d3e](deephaven/web-client-ui@1e40d3e))
* View cell contents in context menu ([#1657](deephaven/web-client-ui#1657)) ([90b7517](deephaven/web-client-ui@90b7517)), closes [#1605](deephaven/web-client-ui#1605)


### BREAKING CHANGES

* Bootstrap color variables are now predominantly hsl based. SCSS will need to be updated accordingly. Theme providers are needed to load themes.
* Tables assigned to variable beginning with "_" will not open automatically even if "Auto Launch Panels" is checked.

Co-authored-by: deephaven-internal <deephaven-internal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants