influxdata / influxdata/influxdb

pack(120,0) sences will never be execute in simple8b.encode. so simple8b.EncodeAll and simple8b.Encoder will return different bytes value.

Open
#20,302 0 comments 0 reactions 0 assignees View on GitHub
1.x kind/bug
Dominant language
Rust
Stars
31.7k
Forks
3.7k
Avg merge
13h 37m
Merged PRs (30d)
8

Description

__Steps to reproduce:__
List the minimal actions needed to reproduce the behavior.

1. add a new test for simple8b.EncodeAll and simple8b.Encoder in "pkg/encoding/simple8b/encoding_test.go"
in the test case, use simple8b.Encode and simple8b.EncodeAll to encode an int array with 121 values.
The first 120 values ​​are all 1, but the last one is 2.

```
func Test_Encode_And_EncodeAll(t *testing.T) {
enc := simple8b.NewEncoder()
n := 121
in := make([]uint64, n)
for i := 0; i < n; i++ {
in[i] = uint64(1)
}

// len(in) > 120 and the 121th value is not 1.
in[120] = 2

in_copy := make([]uint64, len(in))
copy(in_copy, in)

for _, v := range in {
if err := enc.Write(v); err != nil {
t.Fatalf("Unexpected error: %v", err)
}
}

encoded, err := enc.Bytes()
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}

all, err := simple8b.EncodeAll(in_copy)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}

sz := len(all)*8
encodedAll := make([]byte, sz)

// Write the encoded values
for i, v := range all {
binary.BigEndian.PutUint64(encodedAll[i*8:i*8+8], v)
}

if !bytes.Equal(encoded, encodedAll) {
t.Fatalf("encode is not equal encodeAll.")
}
}

```

2. run test
```
go test -v -test.run Test_Encode_And_EncodeAll
```
3. check test result
```
=== RUN Test_Encode_And_EncodeAll
encoding_test.go:55: encode is not equal encodeAll.
--- FAIL: Test_Encode_And_EncodeAll (0.00s)
FAIL
exit status 1
FAIL github.com/influxdata/influxdb/pkg/encoding/simple8b 0.441s
```
encodeAll result: [16 0 0 0 0 0 0 0 240 0 0 0 0 0 0 2]
encode result: [47 255 255 255 255 255 255 255 47 255 255 255 255 255 255 255 240 0 0 0 0 0 0 2]

4. i had found the bug in simple8b/encoding/canPack, in the function, `canPack(src, 120, 0)` will never be return true.
```
func canPack(src []uint64, n, bits int) bool {
if len(src) < n {
return false
}

// Selector 0,1 are special and use 0 bits to encode runs of 1's
if bits == 0 {
for _, v := range src { // check all values, if the 121th value is not equal 1, it is also return false.
if v != 1 {
return false
}
}
return true
}

max := uint64((1 << uint64(bits)) - 1)

for _, s := range src[:n] {
if s > max {
return false
}
}

return true
}
```

5. but in simple8b.EncodeAll , it has been rewrite and only check `remaining[:120]` which only contains 120 values instead of `src` which may contains more than 120 value.

6. modify the canPack func :
```
func canPack(src []uint64, n, bits int) bool {
if len(src) < n {
return false
}

// Selector 0,1 are special and use 0 bits to encode runs of 1's
if bits == 0 {
for _, v := range src[:n] { // only check the first n values.
if v != 1 {
return false
}
}
return true
}

max := uint64((1 << uint64(bits)) - 1)

for _, s := range src[:n] {
if s > max {
return false
}
}

return true
}
```

7. test again:
```
go test -v -test.run Test_Encode_And_EncodeAll
=== RUN Test_Encode_And_EncodeAll
--- PASS: Test_Encode_And_EncodeAll (0.00s)
PASS
ok github.com/influxdata/influxdb/pkg/encoding/simple8b 0.310s
```

results of two methods:
encodeAll: [16 0 0 0 0 0 0 0 240 0 0 0 0 0 0 2]
encode: [16 0 0 0 0 0 0 0 240 0 0 0 0 0 0 2]

8.in the influxdb, there are two scenes for using `encode` and `encodeAll`:
encode: tsdb/engine/tsm1/cache/compact, when compact the cache data to level 1 tsm file, it will use `encode` method.
encodeAll: tsdb/engine/tsm1/tsm/compact, when compact the mult tsm files, it will use `encodeAll` method.

if you have two influxdb instance with same data, the bytes of some blocks may be not equal.
In the cluster/enterprise version, it may be useful. May be it have been fixed in enterprise version?

9. in all influxdb repo, there are three files using `pkg/encoding/simple8b`, and two files using `jwiler/encoding/simple8b'.
```
zoro@zhaoxinyu ~/D/u/i/influxdb (umon-v1.8.2)> grep 'encoding/simple8b' * -r
pkg/encoding/simple8b/encoding_test.go: "github.com/influxdata/influxdb/pkg/encoding/simple8b"
tsdb/engine/tsm1/batch_integer.go: "github.com/influxdata/influxdb/pkg/encoding/simple8b"
tsdb/engine/tsm1/batch_timestamp.go: "github.com/influxdata/influxdb/pkg/encoding/simple8b"
tsdb/engine/tsm1/int.go: "github.com/jwilder/encoding/simple8b"
tsdb/engine/tsm1/timestamp.go: "github.com/jwilder/encoding/simple8b"
```

i think that influxdb want to use `pkg/encoding/simple8b` to replace `jwilder/encoding/simple8b`,
so i suggest deleteing the `jwilder/encoding/simple8b`.

__Expected behavior:__
simple8b.Encode's result should be equal to the bytes of using simple8b.EncodeAll and binary.BigEndian.PutUint64.

__Actual behavior:__
The results of these two methods are not the same.

__Environment info:__
i had checked those versions, all those versions had the problem.
```
master
v1.8.2
v1.8.3
```

Contributor guide

Open the contributing guide

Research direction

Start by reading canPack and the Encode and EncodeAll entry points in pkg/encoding/simple8b, then reproduce the mismatch with the 121-value case in pkg/encoding/simple8b/encoding_test.go. The work is done when both paths produce identical bytes and the focused test, along with the package tests, passes.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
50/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.