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

Align binary math expression streams by time #7473

Merged
merged 1 commit into from
Oct 18, 2016

Conversation

jsternberg
Copy link
Contributor

@jsternberg jsternberg commented Oct 17, 2016

Also fills in missing values using the fill expression for any binary
aggregation.

Fixes #7427 and #7469.

@jsternberg
Copy link
Contributor Author

@mark-rushakoff this can act as an alternative to #7464 that also implements fill() for missing values. I'll also cherry pick the test case that you have in that PR so we can have it here too.

@jsternberg jsternberg force-pushed the js-7427-align-binary-math-with-fill branch from ed2e75a to b9e9459 Compare October 17, 2016 20:31
left *buf{{$k.Name}}Iterator
right *buf{{$k.Name}}Iterator
fn {{$k.name}}{{if ne $k.Name $v.Name}}{{$v.Name}}{{end}}ExprFunc
points []{{$k.Name}}Point // must be size 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to have this be an array rather than a slice, so that the compiler can enforce length of 2?

Copy link
Contributor Author

@jsternberg jsternberg Oct 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that at first, but then I can't set it to nil when fill(none) is specified. Oh, that reminds me. I forgot to implement fill(previous) which is the only reason this is here to begin with.

@jsternberg
Copy link
Contributor Author

Should this be the correct output of fill(previous)?

> select a, b, a + b from cpu fill(previous)
name: cpu
time                    a       b       a_b
----                    -       -       ---
1476729651246087386     1
1476729656794710992             2       3
1476729661978670263     3       4       7

@desa
Copy link
Contributor

desa commented Oct 17, 2016

@jsternberg wouldn't we get something like

> select a, b, a + b from cpu fill(previous)
name: cpu
time                    a       b       a_b
----                    -       -       ---
1476729651246087386     1
1476729656794710992     1       2       3
1476729661978670263     3       4       7

or am I just misunderstanding?

@jsternberg
Copy link
Contributor Author

That's a good point, but I'm not sure I can do that as part of this PR since it's a separate part of the code. If we think that's a proper thing to do in the future, we might want to add the binary functionality now (like this PR) and then make a note in the docs that we're going to do it for the other types of queries too so it does the output that you said.

@jsternberg jsternberg force-pushed the js-7427-align-binary-math-with-fill branch from b9e9459 to 2cd7c90 Compare October 17, 2016 22:51
Also fills in missing values using the fill expression for any binary
aggregation.
@jsternberg jsternberg force-pushed the js-7427-align-binary-math-with-fill branch from 2cd7c90 to 19a61db Compare October 18, 2016 18:31
@jwilder jwilder added this to the 1.1.0 milestone Oct 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants