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

Remove proto.Value.Integer. #1380

Merged
merged 1 commit into from
Jun 16, 2015
Merged

Remove proto.Value.Integer. #1380

merged 1 commit into from
Jun 16, 2015

Conversation

petermattis
Copy link
Collaborator

Integer values are now encoded into proto.Value.Bytes and any 0 or 8
byte value can be intepreted as an int64. Some of the printing niceness
disappeared from the cockroach cli, but that will hopefully return once
we move to the table-based API where we know the types of columns.

row.Value = &t.NewValue
row.setTimestamp(t.Timestamp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a bug that we don't set the timestamp originally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably. I don't think anything is using KeyValue.Timestamp right now.

Copy link
Member

Choose a reason for hiding this comment

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

Let's track this somewhere then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. See #1383.

Copy link
Member

Choose a reason for hiding this comment

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

thanks Pete.

@vivekmenezes
Copy link
Contributor

LGTM

@@ -149,7 +149,7 @@ message GetResponse {

// A PutRequest is arguments to the Put() method. Note that to write
// an empty value, the value parameter is still specified, but both
Copy link
Contributor

Choose a reason for hiding this comment

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

"both"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@tamird
Copy link
Contributor

tamird commented Jun 16, 2015

\o/ LGTM

@petermattis petermattis force-pushed the pmattis/integer-value branch from b1933d4 to db479ed Compare June 16, 2015 14:19
Integer values are now encoded into proto.Value.Bytes and any 0 or 8
byte value can be intepreted as an int64. Some of the printing niceness
disappeared from the cockroach cli, but that will hopefully return once
we move to the table-based API where we know the types of columns.
@petermattis petermattis force-pushed the pmattis/integer-value branch from db479ed to 3d52588 Compare June 16, 2015 14:50
petermattis added a commit that referenced this pull request Jun 16, 2015
@petermattis petermattis merged commit ae6564b into master Jun 16, 2015
@petermattis petermattis deleted the pmattis/integer-value branch June 16, 2015 15:10
@petermattis petermattis removed the PTAL label Jun 16, 2015
@@ -90,7 +89,14 @@ func (kv *KeyValue) ValueBytes() []byte {
// ValueInt returns the value as an int64. This method will panic if the
Copy link
Contributor

Choose a reason for hiding this comment

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

This method no longer panics.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can still panic if the byte slice is less than 8 bytes long. See https://github.com/cockroachdb/cockroach/blob/master/util/encoding/encoding.go#L193.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK, so the interpretation of "value's type" has changed. This is a little unclear since Value is still exposed as an interface{}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll clean up the comment in a subsequent PR.

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