Merge pull request #1380 from cockroachdb/pmattis/integer-value

Remove proto.Value.Integer.

client/batch.go

@@ -115,9 +115,10 @@ func (b *Batch) fillResults() error {
 				row := &result.Rows[k]
 				row.Key = []byte(call.Args.(*proto.IncrementRequest).Key)
 				if result.Err == nil {
-					// TODO(pmattis): Should IncrementResponse contain a
-					// proto.Value so that the timestamp can be returned?
+					// TODO(pmattis): This is odd. KeyValue.Value is otherwise always a
+					// []byte except for setting it to a *int64 here.
 					row.Value = &t.NewValue
+					row.setTimestamp(t.Timestamp)
 				}
 			case *proto.ScanResponse:
 				result.Rows = make([]KeyValue, len(t.Rows))

client/call.go

@@ -70,9 +70,6 @@ func GetProto(key proto.Key, msg gogoproto.Message) Call {
 		if reply.Value == nil {
 			return util.Errorf("%s: no value present", key)
 		}
-		if reply.Value.Integer != nil {
-			return util.Errorf("%s: unexpected integer value: %+v", key, reply.Value)
-		}
 		return gogoproto.Unmarshal(reply.Value.Bytes, msg)
 	}
 	return c

client/db.go

@@ -31,6 +31,7 @@ import (
 	"github.com/cockroachdb/cockroach/base"
 	"github.com/cockroachdb/cockroach/proto"
 	"github.com/cockroachdb/cockroach/util"
+	roachencoding "github.com/cockroachdb/cockroach/util/encoding"
 	"github.com/cockroachdb/cockroach/util/log"
 	"github.com/cockroachdb/cockroach/util/retry"
 	gogoproto "github.com/gogo/protobuf/proto"
@@ -66,8 +67,6 @@ func (kv *KeyValue) setValue(v *proto.Value) {
 	}
 	if v.Bytes != nil {
 		kv.Value = v.Bytes
-	} else if v.Integer != nil {
-		kv.Value = v.Integer
 	}
 	if v.Timestamp != nil {
 		kv.Timestamp = v.Timestamp.GoTime()
@@ -90,7 +89,14 @@ func (kv *KeyValue) ValueBytes() []byte {
 // ValueInt returns the value as an int64. This method will panic if the
 // value's type is not an int64.
 func (kv *KeyValue) ValueInt() int64 {
-	return *kv.Value.(*int64)
+	if kv.Value == nil {
+		return 0
+	}
+	if i, ok := kv.Value.(*int64); ok {
+		return *i
+	}
+	_, uint64val := roachencoding.DecodeUint64(kv.ValueBytes())
+	return int64(uint64val)
 }
 
 // ValueProto parses the byte slice value as a proto message.

client/table.go

@@ -659,7 +659,10 @@ func (b *Batch) IncStruct(obj interface{}, value int64, column string) {
 	c := Increment(proto.Key(key), value)
 	c.Post = func() error {
 		reply := c.Reply.(*proto.IncrementResponse)
-		pv := &proto.Value{Integer: gogoproto.Int64(reply.NewValue)}
+		// TODO(pmattis): This isn't very efficient. Should be able to pass the
+		// integer value directly instead of encoding it into a []byte.
+		pv := &proto.Value{}
+		pv.SetInteger(reply.NewValue)
 		return unmarshalTableValue(pv, v.FieldByIndex(col.field.Index))
 	}
 
@@ -879,20 +882,19 @@ func marshalTableValue(v reflect.Value) (proto.Value, error) {
 		if v.Bool() {
 			i = 1
 		}
-		r.Integer = &i
+		r.SetInteger(i)
 		return r, nil
 
 	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
-		r.Integer = gogoproto.Int64(v.Int())
+		r.SetInteger(v.Int())
 		return r, nil
 
 	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
-		r.Integer = gogoproto.Int64(int64(v.Uint()))
+		r.SetInteger(int64(v.Uint()))
 		return r, nil
 
 	case reflect.Float32, reflect.Float64:
-		// TODO(pmattis): Should we have a separate float field?
-		r.Integer = gogoproto.Int64(int64(math.Float64bits(v.Float())))
+		r.SetInteger(int64(math.Float64bits(v.Float())))
 		return r, nil
 
 	case reflect.String:
@@ -913,9 +915,6 @@ func unmarshalTableValue(src *proto.Value, dest reflect.Value) error {
 
 	switch d := dest.Addr().Interface().(type) {
 	case *string:
-		if src.Integer != nil {
-			return fmt.Errorf("unable to unmarshal integer value: %s", dest)
-		}
 		if src.Bytes != nil {
 			*d = string(src.Bytes)
 		} else {
@@ -924,9 +923,6 @@ func unmarshalTableValue(src *proto.Value, dest reflect.Value) error {
 		return nil
 
 	case *[]byte:
-		if src.Integer != nil {
-			return fmt.Errorf("unable to unmarshal integer value: %s", dest)
-		}
 		if src.Bytes != nil {
 			*d = src.Bytes
 		} else {
@@ -943,41 +939,26 @@ func unmarshalTableValue(src *proto.Value, dest reflect.Value) error {
 
 	switch dest.Kind() {
 	case reflect.Bool:
-		if src.Bytes != nil {
-			return fmt.Errorf("unable to unmarshal byts value: %s", dest)
-		}
 		dest.SetBool(src.GetInteger() != 0)
 		return nil
 
 	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
-		if src.Bytes != nil {
-			return fmt.Errorf("unable to unmarshal byts value: %s", dest)
-		}
 		dest.SetInt(src.GetInteger())
 		return nil
 
 	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
-		if src.Bytes != nil {
-			return fmt.Errorf("unable to unmarshal byts value: %s", dest)
-		}
 		dest.SetUint(uint64(src.GetInteger()))
 		return nil
 
 	case reflect.Float32, reflect.Float64:
-		if src.Bytes != nil {
-			return fmt.Errorf("unable to unmarshal byts value: %s", dest)
-		}
 		dest.SetFloat(math.Float64frombits(uint64(src.GetInteger())))
 		return nil
 
 	case reflect.String:
-		if src == nil {
+		if src == nil || src.Bytes == nil {
 			dest.SetString("")
 			return nil
 		}
-		if src.Integer != nil {
-			return fmt.Errorf("unable to unmarshal integer value: %s", dest)
-		}
 		dest.SetString(string(src.Bytes))
 		return nil
 	}

proto/api.proto

@@ -148,8 +148,8 @@ 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
-// Bytes and Integer are set to nil.
+// an empty value, the value parameter is still specified, but Bytes
+// is set to nil.
 message PutRequest {
   optional RequestHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
   optional Value value = 2 [(gogoproto.nullable) = false];

proto/data.go

@@ -321,13 +321,24 @@ func (v *Value) Verify(key []byte) error {
 				cksum, Key(key), v)
 		}
 	}
-	if v.Bytes != nil && v.Integer != nil {
-		return util.Errorf("both the value byte slice and integer fields are set for key %s: [% x]",
-			Key(key), v)
-	}
 	return nil
 }
 
+// SetInteger encodes the specified int64 value into the bytes field of the
+// receiver.
+func (v *Value) SetInteger(i int64) {
+	v.Bytes = encoding.EncodeUint64(nil, uint64(i))
+}
+
+// GetInteger decodes an int64 value from the bytes field of the receiver.
+func (v *Value) GetInteger() int64 {
+	if v == nil || v.Bytes == nil {
+		return 0
+	}
+	_, u := encoding.DecodeUint64(v.Bytes)
+	return int64(u)
+}
+
 // computeChecksum computes a checksum based on the provided key and
 // the contents of the value. If the value contains a byte slice, the
 // checksum includes it directly; if the value contains an integer,
@@ -336,8 +347,6 @@ func (v *Value) computeChecksum(key []byte) uint32 {
 	c := encoding.NewCRC32Checksum(key)
 	if v.Bytes != nil {
 		c.Write(v.Bytes)
-	} else if v.Integer != nil {
-		c.Write(encoding.EncodeUint64(nil, uint64(v.GetInteger())))
 	}
 	sum := c.Sum32()
 	encoding.ReleaseCRC32Checksum(c)

proto/data.proto

@@ -46,13 +46,8 @@ message Timestamp {
 // basic types: a "bag o' bytes" generic byte slice and an incrementable
 // int64, for use with the Increment API call.
 message Value {
-  oneof value {
-    // Bytes is the byte slice value.
-    bytes bytes = 1;
-    // Integer is an integer value type. Only Integer values may exist at a key
-    // when making the Increment API call.
-    int64 integer = 2;
-  }
+  // Bytes is the byte slice value.
+  optional bytes bytes = 1;
   // Checksum is a CRC-32-IEEE checksum of the key + value, in that order.
   // If this is an integer value, then the value is interpreted as an 8
   // byte, big-endian encoded value. This value is set by the client on

proto/data_test.go

@@ -24,7 +24,6 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/util"
-	gogoproto "github.com/gogo/protobuf/proto"
 )
 
 // TestKeyNext tests that the method for creating lexicographic
@@ -235,14 +234,6 @@ func TestTimestampPrev(t *testing.T) {
 	}
 }
 
-func TestValueBothBytesAndIntegerSet(t *testing.T) {
-	k := []byte("key")
-	v := Value{Bytes: []byte("a"), Integer: gogoproto.Int64(0)}
-	if err := v.Verify(k); err == nil {
-		t.Error("expected error with both byte slice and integer fields set")
-	}
-}
-
 // TestUnmarshal expects key unmarshaling to never return nil.
 func TestUnmarshal(t *testing.T) {
 	testCases := []struct {
@@ -264,36 +255,6 @@ func TestUnmarshal(t *testing.T) {
 	}
 }
 
-// TestValueZeroIntegerSerialization verifies that a value with
-// integer=0 set can be marshalled and unmarshalled successfully.
-// This tests exists because gob serialization treats integers
-// and pointers to integers as the same and so loses a proto.Value
-// which encodes integer=0.
-//
-// TODO(spencer): change Value type to switch between integer and
-//   []byte value types using a mechanism other than nil pointers.
-func TestValueZeroIntegerSerialization(t *testing.T) {
-	k := Key("key 00")
-	v := Value{Integer: gogoproto.Int64(0)}
-	v.InitChecksum(k)
-
-	data, err := gogoproto.Marshal(&v)
-	if err != nil {
-		t.Fatal(err)
-	}
-	v2 := &Value{}
-	if err = gogoproto.Unmarshal(data, v2); err != nil {
-		t.Fatal(err)
-	}
-	if v2.Integer == nil {
-		t.Errorf("expected non-nil integer value; got %s", v2)
-	} else if v2.GetInteger() != 0 {
-		t.Errorf("expected zero integer value; got %d", v2.GetInteger())
-	} else if err = v2.Verify(k); err != nil {
-		t.Errorf("failed value verification: %s", err)
-	}
-}
-
 func TestValueChecksumEmpty(t *testing.T) {
 	k := []byte("key")
 	v := Value{}
@@ -328,27 +289,6 @@ func TestValueChecksumWithBytes(t *testing.T) {
 	}
 }
 
-func TestValueChecksumWithInteger(t *testing.T) {
-	k := []byte("key")
-	testValues := []int64{0, 1, -1, math.MinInt64, math.MaxInt64}
-	for _, i := range testValues {
-		v := Value{Integer: gogoproto.Int64(i)}
-		v.InitChecksum(k)
-		if err := v.Verify(k); err != nil {
-			t.Error(err)
-		}
-		// Try a different key; should fail.
-		if err := v.Verify([]byte("key2")); err == nil {
-			t.Error("expected checksum verification failure on different key")
-		}
-		// Mess with value.
-		v.Integer = gogoproto.Int64(i + 1)
-		if err := v.Verify(k); err == nil {
-			t.Error("expected checksum verification failure on different value")
-		}
-	}
-}
-
 func TestTxnEqual(t *testing.T) {
 	tc := []struct {
 		txn1, txn2 *Transaction