-
Notifications
You must be signed in to change notification settings - Fork 32
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
fix: Default to Skip
operation instead of Sum
operation
#1648
Conversation
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. |
Skip
operation instead of Sum
operationSkip
operation instead of Sum
operation
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
5ffdee4
to
bececf6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
packages/iris-grid/src/IrisGrid.tsx
Outdated
if (aggregationIndex === -1) { | ||
aggregationIndex = newAggregations.length; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were we just not adding aggregations in this case before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Skip
operation unless the user has actually applied one or more operations for a column