Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add cumulative sum expression function #80129
Add cumulative sum expression function #80129
Changes from 2 commits
17d1e0b
93dc455
5d29f58
950e642
e67af0b
f48880d
583d42c
61f6d47
9dca454
6e22969
f9c58b1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
thinking about this again, it does seem a bit weird that the comulative sum would overwrite the current value, or at least that being the only option.
could we add another argument,
name
which would be the name of the new column ? (what map does)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.
and i guess (what do you think @lukeelmers ) we can still replace the column if no name is provided ?
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.
I thought about it but didn't implement eventually because you can put that together quite easily yourself using
mapColumn
like in the example in the description (trying to keep the function as single-purpose as possible). I'm fine with adding it if you think it's the right thing though, in that case what aboutinput
andoutput
arguments (name
/column
isn't really descriptive anymore)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.
Actually, thinking about it again, what about the following API?
name
or unnamed arg are the target column and a required argument.from
is an optional argument and references the column to calculate the cumulative sum for - if it's not set, it defaults toname
Then you can do these:
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 can't replace the column in Lens because the same inner column could be used for multiple metrics. Imagine that the user has configured
Count
andCumulative sum of count
- we need to use the original column, and create a copy.There are no existing expression functions which are able to create a column with the fieldFormatter params, or that can "copy" an existing column
For both these reasons, my preference is to use the API we last discussed here: #61775 (comment)
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.
++ to not overwriting the column (which it sounds like we can't do anyway)
As an aside: I didn't realize
mapColumn
doesn't copy the full meta, but perhaps it should be provided as an option there as well.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.
why would we ever want to change formatter (looking at the issue linked above)? i think that's not a good idea. I think the resulting column should keep the same format information as original column had (why would doing a sum/derivative/ some other calculation affect the format ?)
also thinking about it again, seems overriding existing column:
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.
I'm not sure about the use case for changing the formatter either, but it does feel like if we are making any expression functions that copy columns (currently just
mapColumn
) we should at least be preserving the full meta, or as much of it as makes sense.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.
I definitely see your point @lukeelmers, but
mapColumn
is not necessarily mapping just a single column, it's acting on the whole row. Maybe a separate function would be justified. I will create a separate issue for discussing this (IMHO it doesn't block moving forward with 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.
++ Yeah it shouldn't block moving forward with this, but point taken on it acting on a whole row.