fix: Default to Skip operation instead of Sum operation#1648
fix: Default to Skip operation instead of Sum operation#1648mofojed merged 5 commits intodeephaven:mainfrom
Skip operation instead of Sum operation#1648Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1648 +/- ##
==========================================
- Coverage 46.65% 46.58% -0.08%
==========================================
Files 593 597 +4
Lines 36467 36525 +58
Branches 9135 9144 +9
==========================================
+ Hits 17014 17015 +1
- Misses 19401 19458 +57
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mofojed
changed the title
Skip operation instead of Sum operationSkip operation instead of Sum operation
mofojed
force-pushed
the
4182-skip-aggs
branch
from
ee679a1 to
5ffdee4
Compare
- It's already defined in IrisGridTableModelTemplate which this class extends, and was the exact same code
- Unless the user specifies it, we should just skip it
mofojed
force-pushed
the
4182-skip-aggs
branch
from
5ffdee4 to
bececf6
Compare
mattrunyon
left a comment
mattrunyon
left a comment
There was a problem hiding this comment.
If I create aggregations (created a sum using the table in the linked ticket) and select none of the columns, a server error is logged to the console (doesn't happen before this change). Here's the top of the stack
io.grpc.StatusRuntimeException: INVALID_ARGUMENT: io.deephaven.proto.backplane.grpc.AggregateRequest must have at least one aggregations (5)
There's a 2nd issue where if you have no columns selected in the aggregations menu, it also still shows the aggregation after you leave the edit menu. Not sure if that's intentional, but it is that way in main so probably just file as a separate ticket
- Now if you deselect all of the columns for an aggregation, it will filter out that column from the config that it applies - Will also remove from the aggregations list entirely
mattrunyon
left a comment
mattrunyon
left a comment
There was a problem hiding this comment.
Looks like it's functioning correctly. Just a few minor things
|
|
||
| // Filter out aggregations without any columns actually selected | ||
| const aggregations = aggregationSettings.aggregations.filter( | ||
| agg => agg.selected.length > 0 || agg.invert |
There was a problem hiding this comment.
If you somehow got into a state with all "selected", but invert = true then it's actually none selected. Not sure if that's possible though
I think invert makes logic around this more complex than it needs to be, but not worth changing in this PR. Like why track selected + invert instead of just selected. The invert seems to only get set on toggle all and reset. Which for reset it sets selection to none + invert. Should just set selection to all in that case
I'm guessing it's because of a "select all" case if columns or custom columns are added. Just feels like there should be selectAll instead or something
There was a problem hiding this comment.
We do invert so it scales up to large lists. If you have hundreds of columns and only deselect 1, rather than storing the hundreds of columns it just sends the one and the invert bit.
Not so important here, but in places like the Advanced Filter Creator it is important, as you could be deselecting values from a list of potentially millions.
In this case invert = true with all "selected", we flip the inversion bit (AggregationEdit#handleSelect). Could wrap this logic in some sort of utility method or something though to make it clearer, or resolve it to the list of aggs and just look at that. Both which I'm not inclined to do in this PR.
There was a problem hiding this comment.
Makes sense. Although the actual logic looks like invert is only false if toggleAll is used and thus you can still end up with n-1 columns in your aggregations list instead of what should be the max of n/2 if the inversion is flipped any time it would be cheaper to track the smaller portion.
Either way not important to this change
| if (aggregationIndex === -1) { | ||
| aggregationIndex = newAggregations.length; | ||
| } |
There was a problem hiding this comment.
Were we just not adding aggregations in this case before?
There was a problem hiding this comment.
This case never happened before, because the aggregation always existed and we just replaced it. Now we're removing it if the aggregation is empty.
There was a problem hiding this comment.
There is a minor difference in behaviour, where if you add a few aggregations, then edit the top one in the list, deselect everything, and then start selecting stuff again, it will now be at the end of the list.
I'm just going to change it to remove it only after the user exits the AggregationEdit screen.
|
Also just found out BigInt and BigDecimal show up as possible (on sum at least), but don't actually aggregate. They can cause the same error as the error with none selected before if only a BigInt or BigDecimal is selected |
|
@mattrunyon The BigInteger issue looks to be a JS API issue, I've logged a ticket for it: deephaven/deephaven-core#4877 |
- So the order stays stable
2 participants
Skipoperation unless the user has actually applied one or more operations for a column