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.
- 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
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