[query] Change Tag ID generation to avoid possible collisions

src/query/models/config.go

@@ -31,7 +31,8 @@ var validIDSchemes = []IDSchemeType{
 	TypePrependMeta,
 }
 
-func (t IDSchemeType) validateIDSchemeType() error {
+// Validate validates that the scheme type is valid.
+func (t IDSchemeType) Validate() error {
 	if t == TypeDefault {
 		return errors.New("id scheme type not set")
 	}
@@ -53,7 +54,7 @@ func (t IDSchemeType) String() string {
 	case TypeQuoted:
 		return "quoted"
 	case TypePrependMeta:
-		return "prependMeta"
+		return "prepend_meta"
 	default:
 		// Should never get here.
 		return "unknown"

src/query/models/config_test.go

@@ -30,17 +30,17 @@ import (
 )
 
 func TestIDSchemeValidation(t *testing.T) {
-	err := TypeDefault.validateIDSchemeType()
+	err := TypeDefault.Validate()
 	assert.EqualError(t, err, "id scheme type not set")
-	err = TypeLegacy.validateIDSchemeType()
+	err = TypeLegacy.Validate()
 	assert.NoError(t, err)
-	err = TypePrependMeta.validateIDSchemeType()
+	err = TypePrependMeta.Validate()
 	assert.NoError(t, err)
-	err = TypeQuoted.validateIDSchemeType()
+	err = TypeQuoted.Validate()
 	assert.NoError(t, err)
-	err = IDSchemeType(4).validateIDSchemeType()
+	err = IDSchemeType(4).Validate()
 	assert.EqualError(t, err, "invalid config id schema type 'unknown':"+
-		" should be one of [legacy quoted prependMeta]")
+		" should be one of [legacy quoted prepend_meta]")
 }
 
 func TestMetricsTypeUnmarshalYAML(t *testing.T) {

src/query/models/options.go

@@ -50,7 +50,7 @@ func (o *tagOptions) Validate() error {
 		return errNoName
 	}
 
-	return o.idScheme.validateIDSchemeType()
+	return o.idScheme.Validate()
 }
 
 func (o *tagOptions) SetVersion(version int) TagOptions {

src/query/models/options_test.go

@@ -59,7 +59,7 @@ func TestBadNameTagOptions(t *testing.T) {
 
 func TestBadSchemeTagOptions(t *testing.T) {
 	msg := "invalid config id schema type 'unknown': should be one of" +
-		" [legacy quoted prependMeta]"
+		" [legacy quoted prepend_meta]"
 	opts := NewTagOptions().
 		SetIDSchemeType(IDSchemeType(4))
 	assert.EqualError(t, opts.Validate(), msg)

src/query/models/tags.go

@@ -193,29 +193,24 @@ func (t Tags) prependMetaID() []byte {
 }
 
 func writeTagLengthMeta(dst []byte, lengths []int) int {
-	idx := 0
-	for _, l := range lengths {
-		idx = writer.WriteInteger(dst, l, idx)
-	}
-
-	return idx
+	idx := writer.WriteIntegers(dst, lengths, sep, 0)
+	dst[idx] = finish
+	return idx + 1
 }
 
 func (t Tags) prependMetaLen() (int, []int) {
-	idLen := 0
-	tagLengths := make([]int, len(t.Tags))
+	idLen := 1 // account for separator
+	tagLengths := make([]int, len(t.Tags)*2)
 	for i, tag := range t.Tags {
 		tagLen := len(tag.Name)
-		tagLen += len(tag.Value)
-		tagLengths[i] = tagLen
+		tagLengths[2*i] = tagLen
+		idLen += tagLen
+		tagLen = len(tag.Value)
+		tagLengths[2*i+1] = tagLen
 		idLen += tagLen
 	}
 
-	prefixLen := 0
-	for _, l := range tagLengths {
-		prefixLen += writer.IntLength(l)
-	}
-
+	prefixLen := writer.IntsLength(tagLengths)
 	return idLen + prefixLen, tagLengths
 }
 

src/query/models/tags_test.go

@@ -28,6 +28,8 @@ import (
 	"testing"
 	"unsafe"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/m3db/m3/src/query/util/writer"
 	xtest "github.com/m3db/m3/src/x/test"
 
@@ -96,11 +98,12 @@ func TestQuotedCollisions(t *testing.T) {
 }
 
 func TestLongTagNewIDOutOfOrderPrefixed(t *testing.T) {
-	tags := testLongTagIDOutOfOrder(t, TypePrependMeta)
+	tags := testLongTagIDOutOfOrder(t, TypePrependMeta).
+		AddTag(Tag{Name: []byte("t9"), Value: []byte(`"v1"t2"v2"`)})
 	actual := tags.ID()
 	expectedLength, _ := tags.prependMetaLen()
-	assert.Equal(t, expectedLength, len(actual))
-	assert.Equal(t, []byte("4444t1v1t2v2t3v3t4v4"), actual)
+	require.Equal(t, expectedLength, len(actual))
+	assert.Equal(t, []byte(`2,2,2,2,2,2,2,2,2,10!t1v1t2v2t3v3t4v4t9"v1"t2"v2"`), actual)
 }
 
 func createTags(withName bool) Tags {
@@ -265,17 +268,13 @@ func TestTagAppend(t *testing.T) {
 }
 
 func TestWriteTagLengthMeta(t *testing.T) {
-	lengths := []int{0, 1, 2, 8, 10, 8, 100, 8, 101, 8, 110}
-	l := 0
-	for _, length := range lengths {
-		l += writer.IntLength(length)
-	}
-
-	assert.Equal(t, 18, l)
+	lengths := []int{0, 1, 2, 8, 10, 8, 100, 8, 101, 8, 110, 123456, 12345}
+	l := writer.IntsLength(lengths) + 1 // account for final character
+	require.Equal(t, 41, l)
 	buf := make([]byte, l)
 	count := writeTagLengthMeta(buf, lengths)
-	assert.Equal(t, 18, count)
-	assert.Equal(t, []byte("012810810081018110"), buf)
+	require.Equal(t, 41, count)
+	assert.Equal(t, []byte("0,1,2,8,10,8,100,8,101,8,110,12345,12345!"), buf)
 }
 
 func buildTags(b *testing.B, count, length int, opts TagOptions, escape bool) Tags {

src/query/models/types.go

@@ -26,8 +26,9 @@ import (
 
 // Separators for tags.
 const (
-	sep = byte(',')
-	eq  = byte('=')
+	sep    = byte(',')
+	finish = byte('!')
+	eq     = byte('=')
 )
 
 // IDSchemeType determines the scheme for generating

src/query/util/writer/int_writer.go

@@ -55,3 +55,34 @@ func WriteInteger(dst []byte, value, idx int) int {
 	dst[idx-1] = byte(48 + value)
 	return finalIndex
 }
+
+// IntsLength determines the number of digits in a list of base 10 integers,
+// accounting for separators between each integer.
+func IntsLength(is []int) int {
+	// initialize length accounting for separators.
+	l := len(is) - 1
+	for _, i := range is {
+		l += IntLength(i)
+	}
+
+	return l
+}
+
+// WriteIntegers writes a slice of base 10 integer to a buffer at a given index,
+// separating each value with the given separator, returning the index at which
+// the write ends.
+//
+// NB: Ensure that there is sufficient space in the buffer to hold values and
+// separators.
+func WriteIntegers(dst []byte, values []int, sep byte, idx int) int {
+	l := len(values) - 1
+	for _, v := range values[:l] {
+		idx = WriteInteger(dst, v, idx)
+		dst[idx] = sep
+		idx++
+	}
+
+	idx = WriteInteger(dst, values[l], idx)
+	// Write the last integer.
+	return idx
+}

src/query/util/writer/int_writer_test.go

@@ -25,6 +25,8 @@ import (
 	"math"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/stretchr/testify/assert"
 )
 
@@ -59,3 +61,80 @@ func TestWriteIntegersAtIndex(t *testing.T) {
 	assert.Equal(t, 5, idx)
 	assert.Equal(t, []byte("12345"), buf)
 }
+
+func TestWriteIntegersSingle(t *testing.T) {
+	sep := byte('!')
+	ints := []int{1}
+	l := IntsLength(ints)
+	assert.Equal(t, 1, l)
+
+	buf := make([]byte, l)
+	idx := WriteIntegers(buf, ints, sep, 0)
+	assert.Equal(t, l, idx)
+	expected := []byte("1")
+	assert.Equal(t, expected, buf)
+
+	ints = []int{10}
+	l = IntsLength(ints)
+	assert.Equal(t, 2, l)
+	buf = make([]byte, l)
+	idx = WriteIntegers(buf, ints, sep, 0)
+	assert.Equal(t, l, idx)
+	expected = []byte("10")
+	assert.Equal(t, expected, buf)
+}
+
+func TestWriteIntegersSingleAtIndex(t *testing.T) {
+	sep := byte('!')
+	ints := []int{1}
+	buf := make([]byte, 2)
+	buf[0] = byte('?')
+	idx := WriteIntegers(buf, ints, sep, 1)
+	assert.Equal(t, 3, idx)
+	expected := []byte("?1")
+	assert.Equal(t, expected, buf)
+
+	idx = 0
+	idx = WriteIntegers(buf, ints, sep, idx)
+	idx = WriteIntegers(buf, ints, sep, idx)
+	assert.Equal(t, 3, idx)
+	expected = []byte("11")
+	assert.Equal(t, expected, buf)
+}
+
+func TestWriteIntegersMultiple(t *testing.T) {
+	sep := byte('!')
+	ints := []int{1, 2}
+	l := IntsLength(ints)
+	assert.Equal(t, 3, l)
+
+	buf := make([]byte, l)
+	idx := WriteIntegers(buf, ints, sep, 0)
+	assert.Equal(t, l, idx)
+	expected := []byte("1!2")
+	require.Equal(t, expected, buf)
+
+	ints = []int{10, 20}
+	l = IntsLength(ints)
+	assert.Equal(t, 5, l)
+	buf = make([]byte, l)
+	idx = WriteIntegers(buf, ints, sep, 0)
+	assert.Equal(t, l, idx)
+	expected = []byte("10!20")
+	assert.Equal(t, expected, buf)
+}
+
+func TestWriteIntegersMultipleAtIndex(t *testing.T) {
+	sep := byte('!')
+	ints := []int{1, 20, 300, 4000, 50000}
+	l := IntsLength(ints)
+	assert.Equal(t, 19, l)
+
+	buf := make([]byte, l+2)
+	buf[0] = '?'
+	buf[l+1] = '?'
+	idx := WriteIntegers(buf, ints, sep, 1)
+	assert.Equal(t, l+1, idx)
+	expected := []byte("?1!20!300!4000!50000?")
+	require.Equal(t, expected, buf)
+}