You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by wu...@apache.org on 2025/08/11 04:16:19 UTC
(skywalking-banyandb) branch main updated: fix: correct decode bug in tag.go and column.go (#720)
This is an automated email from the ASF dual-hosted git repository.
wusheng pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/skywalking-banyandb.git
The following commit(s) were added to refs/heads/main by this push:
new 7e5b2d04 fix: correct decode bug in tag.go and column.go (#720)
7e5b2d04 is described below
commit 7e5b2d0404e8ad6d5835eee6fe589a2544d0decb
Author: Gao Hongtao <ha...@gmail.com>
AuthorDate: Mon Aug 11 12:16:14 2025 +0800
fix: correct decode bug in tag.go and column.go (#720)
- Fix decodeDefault function to use bb.Buf[1:] instead of bb.Buf
- The first byte contains encoding type information and should be skipped during decoding
- Add comprehensive tests for high cardinality string encoding to verify the fix
- Ensures proper decoding of dictionary and plain encoded values
This fixes a bug where the encoding type byte was being included in the data
decoding, causing incorrect value parsing.
---
banyand/measure/column.go | 2 +-
banyand/measure/column_test.go | 121 ++++++++++++++++++++++++++++++++++++++++
banyand/stream/tag.go | 2 +-
banyand/stream/tag_test.go | 122 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 245 insertions(+), 2 deletions(-)
diff --git a/banyand/measure/column.go b/banyand/measure/column.go
index 1c17929b..c5bf2978 100644
--- a/banyand/measure/column.go
+++ b/banyand/measure/column.go
@@ -371,7 +371,7 @@ func (c *column) decodeDefault(decoder *encoding.BytesBlockDecoder, bb *bytes.Bu
defer releaseDictionary(dict)
c.values, err = dict.Decode(c.values[:0], bb.Buf[1:], count)
} else {
- c.values, err = decoder.Decode(c.values[:0], bb.Buf, count)
+ c.values, err = decoder.Decode(c.values[:0], bb.Buf[1:], count)
}
if err != nil {
logger.Panicf("%s: cannot decode values: %v", path, err)
diff --git a/banyand/measure/column_test.go b/banyand/measure/column_test.go
index 2a928b38..9f2e39ec 100644
--- a/banyand/measure/column_test.go
+++ b/banyand/measure/column_test.go
@@ -18,6 +18,7 @@
package measure
import (
+ "fmt"
"testing"
"github.com/stretchr/testify/assert"
@@ -150,3 +151,123 @@ func TestColumnFamily_resizeColumns(t *testing.T) {
assert.Equal(t, 6, len(columns))
assert.True(t, cap(columns) >= 6) // The capacity is at least 6, but could be more
}
+
+func TestColumn_HighCardinalityStringEncoding(t *testing.T) {
+ tests := []struct {
+ name string
+ description string
+ expectedEncType encoding.EncodeType
+ uniqueCount int
+ totalCount int
+ }{
+ {
+ name: "exactly 256 unique values - should use dictionary",
+ description: "Dictionary encoding should be used when exactly at the threshold",
+ expectedEncType: encoding.EncodeTypeDictionary,
+ uniqueCount: 256,
+ totalCount: 256,
+ },
+ {
+ name: "257 unique values - should use plain encoding",
+ description: "Plain encoding should be used when exceeding dictionary threshold",
+ expectedEncType: encoding.EncodeTypePlain,
+ uniqueCount: 257,
+ totalCount: 257,
+ },
+ {
+ name: "300 unique values - should use plain encoding",
+ description: "Plain encoding should be used for high cardinality strings",
+ expectedEncType: encoding.EncodeTypePlain,
+ uniqueCount: 300,
+ totalCount: 300,
+ },
+ {
+ name: "1000 unique values - should use plain encoding",
+ description: "Plain encoding should be used for very high cardinality",
+ expectedEncType: encoding.EncodeTypePlain,
+ uniqueCount: 1000,
+ totalCount: 1000,
+ },
+ {
+ name: "500 total with 200 unique - should use dictionary",
+ description: "Dictionary should be used when unique count is below threshold despite high total count",
+ expectedEncType: encoding.EncodeTypeDictionary,
+ uniqueCount: 200,
+ totalCount: 500,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ // Generate unique string values
+ values := make([][]byte, tt.totalCount)
+
+ // Create unique values up to uniqueCount
+ for i := 0; i < tt.uniqueCount; i++ {
+ values[i] = []byte(fmt.Sprintf("unique_value_%06d", i))
+ }
+
+ // If totalCount > uniqueCount, repeat some values to reach totalCount
+ for i := tt.uniqueCount; i < tt.totalCount; i++ {
+ // Repeat values cyclically
+ repeatIndex := i % tt.uniqueCount
+ values[i] = []byte(fmt.Sprintf("unique_value_%06d", repeatIndex))
+ }
+
+ testColumn := &column{
+ name: "high_cardinality_column",
+ valueType: pbv1.ValueTypeStr,
+ values: values,
+ }
+
+ // Encode the column
+ cm := &columnMetadata{}
+ buf := &bytes.Buffer{}
+ w := &writer{}
+ w.init(buf)
+
+ testColumn.mustWriteTo(cm, w)
+
+ // Verify basic metadata
+ assert.Equal(t, w.bytesWritten, cm.size)
+ assert.Equal(t, uint64(len(buf.Buf)), cm.size)
+ assert.Equal(t, uint64(0), cm.offset)
+ assert.Equal(t, testColumn.name, cm.name)
+ assert.Equal(t, testColumn.valueType, cm.valueType)
+
+ // Check encoding type by examining the first byte of the encoded data
+ assert.True(t, len(buf.Buf) > 0, "Encoded buffer should not be empty")
+ actualEncType := encoding.EncodeType(buf.Buf[0])
+ assert.Equal(t, tt.expectedEncType, actualEncType,
+ "Expected %s encoding (%d), got %d. %s",
+ getColumnEncodeTypeName(tt.expectedEncType), tt.expectedEncType, actualEncType, tt.description)
+
+ // Test roundtrip: decode and verify all values are preserved
+ decoder := &encoding.BytesBlockDecoder{}
+ unmarshaled := &column{}
+ unmarshaled.mustReadValues(decoder, buf, *cm, uint64(len(testColumn.values)))
+
+ assert.Equal(t, testColumn.name, unmarshaled.name)
+ assert.Equal(t, testColumn.valueType, unmarshaled.valueType)
+ assert.Equal(t, len(testColumn.values), len(unmarshaled.values), "Number of values should match")
+
+ // Verify all values are correctly decoded
+ for i, originalValue := range testColumn.values {
+ assert.Equal(t, originalValue, unmarshaled.values[i],
+ "Value at index %d should match original", i)
+ }
+ })
+ }
+}
+
+// Helper function to get encode type name for better test output.
+func getColumnEncodeTypeName(encType encoding.EncodeType) string {
+ switch encType {
+ case encoding.EncodeTypePlain:
+ return "Plain"
+ case encoding.EncodeTypeDictionary:
+ return "Dictionary"
+ default:
+ return fmt.Sprintf("Unknown(%d)", encType)
+ }
+}
diff --git a/banyand/stream/tag.go b/banyand/stream/tag.go
index aeefef1f..4ad42dd4 100644
--- a/banyand/stream/tag.go
+++ b/banyand/stream/tag.go
@@ -386,7 +386,7 @@ func (t *tag) decodeDefault(decoder *encoding.BytesBlockDecoder, bb *bytes.Buffe
defer releaseDictionary(dict)
t.values, err = dict.Decode(t.values[:0], bb.Buf[1:], count)
} else {
- t.values, err = decoder.Decode(t.values[:0], bb.Buf, count)
+ t.values, err = decoder.Decode(t.values[:0], bb.Buf[1:], count)
}
if err != nil {
logger.Panicf("%s: cannot decode values: %v", path, err)
diff --git a/banyand/stream/tag_test.go b/banyand/stream/tag_test.go
index a1d293ae..1416bbd5 100644
--- a/banyand/stream/tag_test.go
+++ b/banyand/stream/tag_test.go
@@ -18,6 +18,7 @@
package stream
import (
+ "fmt"
"testing"
"github.com/stretchr/testify/assert"
@@ -154,3 +155,124 @@ func TestTagFamily_resizeTags(t *testing.T) {
assert.Equal(t, 6, len(tags))
assert.True(t, cap(tags) >= 6) // The capacity is at least 6, but could be more
}
+
+func TestTag_HighCardinalityStringEncoding(t *testing.T) {
+ tests := []struct {
+ name string
+ description string
+ expectedEncType encoding.EncodeType
+ uniqueCount int
+ totalCount int
+ }{
+ {
+ name: "exactly 256 unique values - should use dictionary",
+ uniqueCount: 256,
+ totalCount: 256,
+ expectedEncType: encoding.EncodeTypeDictionary,
+ description: "Dictionary encoding should be used when exactly at the threshold",
+ },
+ {
+ name: "257 unique values - should use plain encoding",
+ uniqueCount: 257,
+ totalCount: 257,
+ expectedEncType: encoding.EncodeTypePlain,
+ description: "Plain encoding should be used when exceeding dictionary threshold",
+ },
+ {
+ name: "300 unique values - should use plain encoding",
+ uniqueCount: 300,
+ totalCount: 300,
+ expectedEncType: encoding.EncodeTypePlain,
+ description: "Plain encoding should be used for high cardinality strings",
+ },
+ {
+ name: "1000 unique values - should use plain encoding",
+ uniqueCount: 1000,
+ totalCount: 1000,
+ expectedEncType: encoding.EncodeTypePlain,
+ description: "Plain encoding should be used for very high cardinality",
+ },
+ {
+ name: "500 total with 200 unique - should use dictionary",
+ uniqueCount: 200,
+ totalCount: 500,
+ expectedEncType: encoding.EncodeTypeDictionary,
+ description: "Dictionary should be used when unique count is below threshold despite high total count",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ // Generate unique string values
+ values := make([][]byte, tt.totalCount)
+
+ // Create unique values up to uniqueCount
+ for i := 0; i < tt.uniqueCount; i++ {
+ values[i] = []byte(fmt.Sprintf("unique_value_%06d", i))
+ }
+
+ // If totalCount > uniqueCount, repeat some values to reach totalCount
+ for i := tt.uniqueCount; i < tt.totalCount; i++ {
+ // Repeat values cyclically
+ repeatIndex := i % tt.uniqueCount
+ values[i] = []byte(fmt.Sprintf("unique_value_%06d", repeatIndex))
+ }
+
+ testTag := &tag{
+ name: "high_cardinality_tag",
+ valueType: pbv1.ValueTypeStr,
+ values: values,
+ }
+
+ // Encode the tag
+ tm := &tagMetadata{}
+ buf, filterBuf := &bytes.Buffer{}, &bytes.Buffer{}
+ w, fw := &writer{}, &writer{}
+ w.init(buf)
+ fw.init(filterBuf)
+
+ testTag.mustWriteTo(tm, w, fw)
+
+ // Verify basic metadata
+ assert.Equal(t, w.bytesWritten, tm.size)
+ assert.Equal(t, uint64(len(buf.Buf)), tm.size)
+ assert.Equal(t, uint64(0), tm.offset)
+ assert.Equal(t, testTag.name, tm.name)
+ assert.Equal(t, testTag.valueType, tm.valueType)
+
+ // Check encoding type by examining the first byte of the encoded data
+ assert.True(t, len(buf.Buf) > 0, "Encoded buffer should not be empty")
+ actualEncType := encoding.EncodeType(buf.Buf[0])
+ assert.Equal(t, tt.expectedEncType, actualEncType,
+ "Expected %s encoding (%d), got %d. %s",
+ getEncodeTypeName(tt.expectedEncType), tt.expectedEncType, actualEncType, tt.description)
+
+ // Test roundtrip: decode and verify all values are preserved
+ decoder := &encoding.BytesBlockDecoder{}
+ unmarshaled := &tag{}
+ unmarshaled.mustReadValues(decoder, buf, *tm, uint64(len(testTag.values)))
+
+ assert.Equal(t, testTag.name, unmarshaled.name)
+ assert.Equal(t, testTag.valueType, unmarshaled.valueType)
+ assert.Equal(t, len(testTag.values), len(unmarshaled.values), "Number of values should match")
+
+ // Verify all values are correctly decoded
+ for i, originalValue := range testTag.values {
+ assert.Equal(t, originalValue, unmarshaled.values[i],
+ "Value at index %d should match original", i)
+ }
+ })
+ }
+}
+
+// Helper function to get encode type name for better test output.
+func getEncodeTypeName(encType encoding.EncodeType) string {
+ switch encType {
+ case encoding.EncodeTypePlain:
+ return "Plain"
+ case encoding.EncodeTypeDictionary:
+ return "Dictionary"
+ default:
+ return fmt.Sprintf("Unknown(%d)", encType)
+ }
+}