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

Support int64 data types #2261

Merged
merged 1 commit into from
Apr 13, 2015
Merged

Support int64 data types #2261

merged 1 commit into from
Apr 13, 2015

Conversation

otoolep
Copy link
Contributor

@otoolep otoolep commented Apr 13, 2015

No description provided.

@otoolep
Copy link
Contributor Author

otoolep commented Apr 13, 2015

@benbjohnson

case int:
return Number
return Float
case int64, int32, int:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is really necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might just need int64.

@dgnorton
Copy link
Contributor

Can you add an integration test that writes & reads ints?

I tested this change on an ARM 32 bit system and it seems to work. The output is interesting once we it gets above math.MaxInt32 though. See the values in the middle... https://gist.github.com/dgnorton/e734dc02cafc80241ae6. It's not a CLI formatting thing. That's also the way the values look in the JSON if I use curl. Haven't tried this test on 64 bit yet.

@otoolep
Copy link
Contributor Author

otoolep commented Apr 13, 2015

Thanks @dgnorton -- I'll add some more tests. The formatting issue you pointed out, yeah, I've seen that before. I am assuming it's due to the use of float64. It's nothing to do with this change.

@@ -976,6 +976,39 @@ func TestServer_StartRetentionPolicyEnforcement_ErrZeroInterval(t *testing.T) {
}
}

// Ensure the server respects limit and offset in show series queries
func TestServer_WriteAllDataTypes(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must add test to server.go since it's the only way to control the types.

@otoolep
Copy link
Contributor Author

otoolep commented Apr 13, 2015

Updated with unit test. +1 @dgnorton ?

@dgnorton
Copy link
Contributor

Why can it only be tested through server.go? Users read & write int64 values through the usual API.

@otoolep
Copy link
Contributor Author

otoolep commented Apr 13, 2015

If by "usual API" you mean they write through the JSON API, Go's JSON unmarshalling casts all numbers to float64.

http://golang.org/pkg/encoding/json/#Unmarshal

There is no way to get integer values into the system through the JSON API. It'll need an API change, or perhaps some code on our side.

@dgnorton
Copy link
Contributor

I think we used to do it using UseNumber() maybe? Not sure, have to go back and look at v0.8 code.

@otoolep
Copy link
Contributor Author

otoolep commented Apr 13, 2015

You could be right @dgnorton -- but I think that is orthogonal to this change. Regardless of how integers get into the system, we need to have support for encoding them. This is what this change does.

@dgnorton
Copy link
Contributor

lgtm :shipit:

otoolep added a commit that referenced this pull request Apr 13, 2015
@otoolep otoolep merged commit 4e28c3a into master Apr 13, 2015
@otoolep otoolep deleted the support_64_int branch April 13, 2015 18:49
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

2 participants