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

Handle negative timestamps #4500

Merged
merged 3 commits into from
Oct 19, 2015
Merged

Handle negative timestamps #4500

merged 3 commits into from
Oct 19, 2015

Conversation

gunnaraasen
Copy link
Member

Fixes #3367.

@otoolep
Copy link
Contributor

otoolep commented Oct 19, 2015

What does a negative timestamp actually mean now?

@gunnaraasen
Copy link
Member Author

A negative timestamp is the number of nanoseconds earlier than the epoch. Right now, the line protocol does not support writing timestamps earlier than 0 (i.e. 1970-01-01T00:00:00Z). With this change the earliest timestamp will be -9223372036854775807 (i.e. 1677-09-21 00:12:43.145224193Z).

Also added a couple tests added to confirm the upper and lower bounds that the line protocol will parse (e.g. the max and min int64 values).

@otoolep
Copy link
Contributor

otoolep commented Oct 19, 2015

I guess I am wondering why we would support this, but I guess if we can code for it without much work, it's OK.

cc @jwilder

@otoolep
Copy link
Contributor

otoolep commented Oct 19, 2015

As opposed to just rejecting the point.

@otoolep
Copy link
Contributor

otoolep commented Oct 19, 2015

+1 on the code, not so sure about whether we shouldn't instead kick back an error.

@gunnaraasen
Copy link
Member Author

It does double the supported time range with minimal cost.

@beckettsean
Copy link
Contributor

@otoolep 0.9 has always supported negative timestamps for writes and reads. This fixes a bug in the line protocol where it threw an error when parsing a negative epoch timestamp. The JSON write path still happily allows negative timestamps and the entire query engine is fine with it.

@beckettsean
Copy link
Contributor

When this is merged we need to update the docs

@otoolep
Copy link
Contributor

otoolep commented Oct 19, 2015

OK, thanks @beckettsean -- sounds like this should be merged.

if err == nil {
t.Fatalf("ParsePoints failed: %v", err)
}
_, err = models.ParsePointsString("cpu value=1 -1?")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a test of 1- as well. Looks like that should pass but good for coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea

@jwilder
Copy link
Contributor

jwilder commented Oct 19, 2015

Small test suggestion but 👍 otherwise.

@gunnaraasen
Copy link
Member Author

Added the suggested test. Going to merge on green.

gunnaraasen added a commit that referenced this pull request Oct 19, 2015
@gunnaraasen gunnaraasen merged commit f101e98 into master Oct 19, 2015
@otoolep otoolep deleted the ga-time-bounds branch October 28, 2015 20:03
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.

4 participants