Conversation
mlafeldt
left a comment
There was a problem hiding this comment.
Thanks! Found some minor inconsistencies and missing tests. Otherwise looks great.
types.go
Outdated
| } | ||
|
|
||
| // NewBitFromString creates a Bit from a string of '0' and '1' characters. | ||
| func NewBitFromString(s string) (*Bit, error) { |
There was a problem hiding this comment.
This returns *Bit while all other type constructors return values - should be Bit, error) for consistency
value.go
Outdated
| case TYPE_BIT: | ||
| bit := mapping.GetBit(v) | ||
| defer mapping.DestroyBit(&bit) | ||
| return Bit{data: mapping.BitMembers(&bit)}, nil |
There was a problem hiding this comment.
In this file, TYPE_BIT is still marked as "Invalid or unsupported" in isPrimitiveType() and missing from inferPrimitiveType()
types.go
Outdated
| // The slice is consumed directly without copying, so the caller must not modify it afterwards. | ||
| func NewBitFromRaw(data []byte) (*Bit, error) { | ||
| if len(data) <= 1 { | ||
| return &Bit{}, nil |
There was a problem hiding this comment.
Nitpicky inconsistency:
- NewBitFromString("") → error ("empty bit string")
- NewBitFromRaw(nil) → success, returns &Bit{}
| require.Equal(t, expected, r) | ||
| } | ||
|
|
||
| func TestAppenderBit(t *testing.T) { |
There was a problem hiding this comment.
Can we also test scanning non-null into var r *Bit?
This adds support for reading/writing the BIT type. Since there is no native type for representing bitstrings in Go, this adds a custom Go type that represents one. It uses the same internal storage as DuckDB so that conversions are cheap and require minimal copying. P.S. Strictly speaking there is the BitString type in the encoding/asn1, but that's not meant for general purpose use. It's really only there so that X509 certificates can be decoded. It's also laid out very differently than DuckDB its BIT type (e.g. it uses 0's for padding instead of 1's).
|
I think I addressed all of your comments. I also removed |
mlafeldt
left a comment
There was a problem hiding this comment.
Thanks for the changes. I had another look and left a few more inline comments to address before merging.
| // Validate checks that Data is a valid DuckDB bit encoding: the padding count | ||
| // (first byte) must be 0-7, and the padding bits in the first data byte must | ||
| // all be set to 1. | ||
| func (b Bit) Validate() error { |
There was a problem hiding this comment.
This is never called on any write path. Both setBit and bindBit pass Data (which is public) straight to DuckDB without validation.
| // all be set to 1. | ||
| func (b Bit) Validate() error { | ||
| if len(b.Data) <= 1 { | ||
| return nil |
There was a problem hiding this comment.
We also need to reject single-byte with nonzero padding
if len(b.Data) == 1 && b.Data[0] != 0 {
return fmt.Errorf("invalid padding count %d with no data bytes", b.Data[0])
}| // NewBitFromString creates a Bit from a string of '0' and '1' characters. | ||
| func NewBitFromString(s string) (Bit, error) { | ||
| if len(s) == 0 { | ||
| return Bit{}, nil |
There was a problem hiding this comment.
This will return Bit{Data: nil}. Passing it to setBit or bindBit sends a nil slice into C code.
I suggest to either return an error for empty strings or return Bit{Data: []byte{0}} as the canonical empty encoding.
| func (conn *Conn) CheckNamedValue(nv *driver.NamedValue) error { | ||
| switch nv.Value.(type) { | ||
| case *big.Int, Interval, []any, []bool, []int8, []int16, []int32, []int64, []int, []uint8, []uint16, | ||
| case *big.Int, Interval, Bit, []any, []bool, []int8, []int16, []int32, []int64, []int, []uint8, []uint16, |
| } | ||
| return mapping.BindInterval(*s.preparedStmt, mapping.IdxT(n+1), i), nil | ||
| case Bit: | ||
| return s.bindBit(&v, n) |
| // Complex type. | ||
| return false | ||
| case TYPE_INVALID, TYPE_BIT, TYPE_ANY: | ||
| case TYPE_INVALID, TYPE_ANY: |
There was a problem hiding this comment.
In value.go, Bit is still not handled in inferPrimitiveType and createPrimitiveValue
This adds support for reading/writing the BIT type. Since there is no native type for representing bitstrings in Go, this adds a custom Go type that represents one. It uses the same internal storage as DuckDB so that conversions are cheap and require minimal copying.
P.S. Strictly speaking there is the BitString type in the encoding/asn1, but that's not meant for general purpose use. It's really only there so that X509 certificates can be decoded. It's also laid out very differently than DuckDB its BIT type (e.g. it uses 0's for padding instead of 1's).