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

SELECT solely on time should return error #3778

Merged
merged 5 commits into from
Aug 21, 2015
Merged

SELECT solely on time should return error #3778

merged 5 commits into from
Aug 21, 2015

Conversation

otoolep
Copy link
Contributor

@otoolep otoolep commented Aug 20, 2015

Fixes #3010.

Simply check for time in the SELECT clause, and kick it back if the query contains it. Should this be a case-insensitive check?

@otoolep
Copy link
Contributor Author

otoolep commented Aug 20, 2015

@dgnorton

@otoolep
Copy link
Contributor Author

otoolep commented Aug 20, 2015

@corylanou

@pauldix
Copy link
Member

pauldix commented Aug 20, 2015

It should actually be valid to put time in the select. It's better to ignore it.

@otoolep
Copy link
Contributor Author

otoolep commented Aug 20, 2015

@pauldix -- what if it's the only SELECT field? No results back?

@pauldix
Copy link
Member

pauldix commented Aug 20, 2015

then it should throw an error. They need to actually select some real data.

@otoolep
Copy link
Contributor Author

otoolep commented Aug 20, 2015

Right, makes sense. So if it's the only field throw back an error, if it's accompanied by other fields, ignore it.

@otoolep otoolep changed the title SELECT on time should return error SELECT solely on time should return error Aug 20, 2015
@otoolep
Copy link
Contributor Author

otoolep commented Aug 20, 2015

@pauldix -- policy updated, plus test added to show that time, value already works fine.

@@ -1394,6 +1394,11 @@ func TestServer_Query_Common(t *testing.T) {
exp: fmt.Sprintf(`{"results":[{"series":[{"name":"cpu","columns":["time","value"],"values":[["%s",1]]}]}]}`, now.Format(time.RFC3339Nano)),
},
&Query{
name: "explicitly selecting time and a valid measurement and field should succeed",
command: `SELECT time,value FROM db0.rp0.cpu`,
Copy link
Contributor

Choose a reason for hiding this comment

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

A test with select value, time would be nice as it would show that time will always be the first column, but all other columns still maintain their sort order (or should)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, well, that test case fails. The following data comes back:

actual: {"results":[{"series":[{"name":"cpu","columns":["value","time"],"values":[["2015-08-20T23:57:35.973310065Z",1]]}]}]}

I'd like to address this separately as it will require a somewhat wider set of changes. OK? #3779

I'd like to get this panic fixed before the 0.9.3 window closes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

@corylanou
Copy link
Contributor

+1

@dgnorton
Copy link
Contributor

@pauldix are there event based use cases where the value isn't important...only the time it happened matters?

@pauldix
Copy link
Member

pauldix commented Aug 21, 2015

@dgnorton maybe, but they still need to store at least one value. For those use cases I would recommend using a boolean. Guess it's a little wasteful to return that, but this is definitely the least of our concerns right now.

@pauldix
Copy link
Member

pauldix commented Aug 21, 2015

+1

@otoolep
Copy link
Contributor Author

otoolep commented Aug 21, 2015

Thanks for the reviews @pauldix and @corylanou -- merging on green and will cherry-pick to 0.9.3.

otoolep added a commit that referenced this pull request Aug 21, 2015
SELECT solely on time should return error
@otoolep otoolep merged commit 1edb13e into master Aug 21, 2015
@otoolep otoolep deleted the select_on_time branch August 21, 2015 19:22
@nornagon
Copy link

FWIW, I'm building an alerting system on top of influxdb and it would be really useful to be able to get the time of the last reported value for a particular series, without particular reference to the value itself. The goal is to write an alert that checks for "is still receiving data", in case the sensors in question die.

@gunnaraasen
Copy link
Member

@nornagon that sounds like a valid use case that hasn't been fully discussed. Can you open a new issue with a feature request?

@nornagon
Copy link

Done: #4862

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.

None yet

6 participants