Add keepLastValue function #995
Conversation
|
Darn, why did you close this? This is probably my favorite function |
|
No worries @tehlers320, it's coming :) |
expr/func_keeplastvalue.go
Outdated
| ArgInt{val: &s.limit}, | ||
| // Allow user to specify 'INF' as value. if so, will fall back to maxInt | ||
| // Ignores any other strings (just like Graphite) | ||
| ArgString{val: &stub}, |
There was a problem hiding this comment.
Don't you mean 'inf' not 'INF' ?
this comment is confusing because it implies the behavior for INF vs other strings is different,
but actually in all cases (inf, INF, or any other strings) it will always just use the default of MaxInt64 because ANY string value is ignored. (not just "any other string" as it states)
There was a problem hiding this comment.
Graphite actually says that "INF" is infinity, but accepts any string since it doesn't check for types.
From graphite:
Param('limit', ParamTypes.integer, default='INF')
I think the way this function is implemented in graphite is not what they intended it to be, but I'm including the option to put in a string just to mimic the behavior.
| {Val: 1234567890, Ts: 60}, | ||
| } | ||
|
|
||
| testKeepLastValue( |
There was a problem hiding this comment.
similar to feedback in previous PR, using utility functions is good, but if they call t.Fatal internally, it's harder to trace back to the calling function so if a test fails it's hard to tell where it failed.
better for the util function to return an error, and here, check for error and if so call t.Fatal.
not a huge deal though, if you have more important stuff to do, then don't worry about this. but something to keep in mind for the future at least. i don't want you to refactor this if you have other functions or whatever to work on.
There was a problem hiding this comment.
Yeah I didn't have the utility functions when I wrote this (I just rebased), so didn't do that. Will use them in the future though.
| if 0 < consecutiveNaNs && consecutiveNaNs <= limit && !math.IsNaN(lastVal) { | ||
| for i := len(out) - consecutiveNaNs; i < len(out); i++ { | ||
| out[i].Val = lastVal | ||
| } |
There was a problem hiding this comment.
interesting. i always thought graphite would just replace nones upto the limit, instead of only replacing nones if an entire batch falls under the limit.
but i checked functions.py and it's the same.
TIL ...
|
@stivenbb can you rebase on top of latest master ? |
stivenbb
force-pushed
the
keepLastValue1
branch
from
7f066ef
to
3ce3c8f
Compare
|
here you go |
|
@stivenbb thanks so much 🥇 |
|
You got it! |
Native implementation of keepLastValue() Graphite function.