Conversation
ed0f3b2 to
04719c0
Compare
Internally we use uint16 to store 16-bit value of BFloat16 but use float32 in the external APIs for the easy of use. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
90be89f to
6aea98f
Compare
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Dynamic random source based on timestamp keep changing the "golden" test file causing it to fail. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
|
|
||
| func (c ColBFloat16) Row(i int) float32 { | ||
| // BFloat16 is upper 16 bits of float32 | ||
| bits := uint32(c[i]) << 16 |
There was a problem hiding this comment.
Do ClickHouse BFloat16 has a sign bit? So may be only 15 bits shift?
btw, here is a library that may be useful https://github.com/chenxingqiang/go-floatx/blob/master/float16.go
There was a problem hiding this comment.
Yes. BFloat16 has the signed bit. Hence we use only the "upper" 16 bits (that always include signed bit)
I verified with inserting some negative values for BFloat16 CH
INSERT INTO bfloat_test VALUES (1, 1.3);
INSERT INTO bfloat_test VALUES (1, -1.8);
SELECT *
FROM bfloat_test
Query id: 79f16ef4-f53b-498f-a619-46b791f153db
┌─id─┬───────val─┐
1. │ 1 │ -1.796875 │
2. │ 1 │ 1.296875 │
└────┴───────────┘
| expected float32 | ||
| delta float64 | ||
| }{ | ||
| {"zero", 0.0, 0.0, 0.0}, |
There was a problem hiding this comment.
this library https://github.com/chenxingqiang/go-floatx claims to test all possible values. May be take same approach? =)
There was a problem hiding this comment.
good point. We should definitely do that.
|
Awesome PR! Quick question: does the algorithm you mention prevent any differences that may come up in memory due to CPU architecture/instructions? I want to make sure we don't see any differences in the data when receiving/sending it, or when converting from float to int. Expanded tests, perhaps even on different archs could be needed. I see our CI has amd64 and 386, but does this cover ARM? We did have an issue in the past with overflows on some CPUs for clickhouse-go |
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Good point @SpencerTorres. It make sense to extend CI testing to ARM. Added it in our workflows |
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
|
Ok. I see the reason why ARM64 was not added before. Basically ARM runners are not cheap and not readily available in Github. I enabled it and it just waiting for more than 20 mins just to get the runner. even then it got timed out. I'm going to remove the ARM64 for now. We can tackle this in separate PR (need different enterprise plan to get ARM64 runners) |
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
|
@SpencerTorres created new issue to add ARM64 based github runners for the CI. |
Summary
Internally we use
uint16to store 16-bit value of BFloat16but we use
float32in the external APIs for the easy of use.The complex part was how to round of Float32 -> Float16 without any systematic bias. Using banker's bias algorithm (closes-to-even bias)
Checklist
Delete items not relevant to your PR: