-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 inconsistent data type #2448
Conversation
@cannium i guess this isn't covered by any tests. might be a good idea to add something to make sure this doesn't regress. i've been adding integration tests for aggregations to https://github.com/influxdb/influxdb/blob/b0865984452938e77148530c4e10b33a6f8c03c5/cmd/influxd/server_integration_test.go#L581. in general it seems the aggregations are not very well tested. |
24412f5
to
1861f41
Compare
Though I managed to make integration tests passed, I noticed that the returned time of |
i'm not sure that the current architecture supports having aggregate functions return anything more than just a value but i could be wrong. |
@cannium actually, i realized that aggregates are applied across intervals of time (determined by {
reset: true,
name: "last value",
write: `{"database" : "%DB%", "retentionPolicy" : "%RP%", "points": [
{"name": "cpu", "timestamp": "2000-01-01T00:00:00Z", "fields": {"value": 2}},
{"name": "cpu", "timestamp": "2000-01-01T00:01:00Z", "fields": {"value": 7}},
{"name": "cpu", "timestamp": "2000-01-01T00:01:10Z", "fields": {"value": 9}}
]}`,
query: `SELECT last(value) FROM cpu WHERE time >= '2000-01-01T00:00:00Z' AND time <= '2000-01-01T00:01:10Z' GROUP BY time(2m)`,
queryDb: "%DB%",
expected: `{"results":[{"series":[{"name":"cpu","columns":["time","last"],"values":[["2000-01-01T00:00:00Z",9]]}]}]}`,
}, you can see that now we've specified an interval and the time showed in the results matches the way i've described it. |
expected: `{"results":[{"series":[{"name":"cpu","columns":["time","last"],"values":[["1970-01-01T00:00:00Z",9]]}]}]}`, | ||
}, | ||
{ | ||
reset: true, |
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.
The data is the same with every write, so there is no need to set the reset
flag between each test. It will just slow testing down without reason.
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.
It's by purpose. I think it's better to make every test to stand on its own in case of any future modifications.
在 2015年5月1日,06:49,otoolep notifications@github.com 写道:
In cmd/influxd/server_integration_test.go:
},
{
reset: true,
name: "last value",
write: `{"database" : "%DB%", "retentionPolicy" : "%RP%", "points": [
{"name": "cpu", "timestamp": "2000-01-01T00:00:00Z", "fields": {"value": 2}},
{"name": "cpu", "timestamp": "2000-01-01T00:01:00Z", "fields": {"value": 7}},
{"name": "cpu", "timestamp": "2000-01-01T00:01:10Z", "fields": {"value": 9}}
]}`,
query: `SELECT last(value) FROM cpu`,
queryDb: "%DB%",
// FIXME: returned time should be "2000-01-01T00:01:10Z"
expected: `{"results":[{"series":[{"name":"cpu","columns":["time","last"],"values":[["1970-01-01T00:00:00Z",9]]}]}]}`,
},
{
The data is the same with every write, so there is no need to set the reset flag between each test. It will just slow testing down without reason.reset: true,
—
Reply to this email directly or view it on GitHub.
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 realise that, but test run times are also important to us. Happy to make changes in the future, but when data is the same, we don't use reset
.
Yes, I've read the code. But I still feel the method name confusing. From a user's perspective, one might expect the corresponding time stamp also returned, like a normal select query.
|
ping? |
@cannium would you mind rebasing this? if you can, i'll try to get this merged today. |
Rebased. |
+1 from myself and verbal +1 from @otoolep - thanks! |
Otherwise panic
would occur when using
first()
,last()
orspread()
.