bkietz commented on code in PR #35769:
URL: https://github.com/apache/arrow/pull/35769#discussion_r1374605138
##########
go/arrow/array/binarybuilder.go:
##########
@@ -370,6 +371,330 @@ func (b *BinaryBuilder) UnmarshalJSON(data []byte) error {
return b.Unmarshal(dec)
}
+const (
+ dfltBlockSize = 1 << 20 // 1 MB
Review Comment:
FWIW, the default in Velox (which I've adopted in the c++ impl) is 32KB
```suggestion
dfltBlockSize = 32 << 10 // 32 KB
```
##########
go/arrow/datatype_viewheader.go:
##########
@@ -0,0 +1,138 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package arrow
+
+import (
+ "bytes"
+ "unsafe"
+
+ "github.com/apache/arrow/go/v14/arrow/endian"
+ "github.com/apache/arrow/go/v14/arrow/internal/debug"
+ "github.com/apache/arrow/go/v14/arrow/memory"
+)
+
+const (
+ StringViewPrefixLen = 4
+ stringViewInlineSize = 12
+)
+
+func IsViewInline(length int) bool {
+ return length < stringViewInlineSize
+}
+
+// ViewHeader is a variable length string (utf8) or byte slice with
+// a 4 byte prefix and inline optimization for small values (12 bytes
+// or fewer). This is similar to Go's standard string but limited by
+// a length of Uint32Max and up to the first four bytes of the string
+// are copied into the struct. This prefix allows failing comparisons
+// early and can reduce CPU cache working set when dealing with short
+// strings.
+//
+// There are two situations:
+//
+// Entirely inlined string data
+// |----|------------|
+// ^ ^
+// | |
+// size inline string data, zero padded
+//
+// Reference into buffer
+// |----|----|----|----|
+// ^ ^ ^ ^
+// | | | |
+// size prefix buffer index and offset to
out-of-line portion
+//
+// Adapted from TU Munich's UmbraDB [1], Velox, DuckDB.
+//
+// [1]: https://db.in.tum.de/~freitag/papers/p29-neumann-cidr20.pdf
+type ViewHeader struct {
+ size uint32
+ // the first 4 bytes of this are the prefix for the string
+ // if size <= StringHeaderInlineSize, then the entire string
+ // is in the data array and is zero padded.
+ // if size > StringHeaderInlineSize, the next 8 bytes are 2 uint32
+ // values which are the buffer index and offset in that buffer
+ // containing the full string.
+ data [stringViewInlineSize]byte
+}
+
+func (sh *ViewHeader) IsInline() bool {
+ return sh.size <= uint32(stringViewInlineSize)
+}
+
+func (sh *ViewHeader) Len() int { return int(sh.size) }
+func (sh *ViewHeader) Prefix() [StringViewPrefixLen]byte {
+ return *(*[4]byte)(unsafe.Pointer(&sh.data))
+}
+
+func (sh *ViewHeader) BufferIndex() uint32 {
Review Comment:
Buffer index and offset were both changed to signed 32 bit integers in
https://lists.apache.org/thread/gl2hf6v0ct5xqpjf832vhmdgkqyxghlg
```suggestion
func (sh *ViewHeader) BufferIndex() int {
```
##########
go/arrow/datatype_viewheader.go:
##########
@@ -0,0 +1,138 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package arrow
+
+import (
+ "bytes"
+ "unsafe"
+
+ "github.com/apache/arrow/go/v14/arrow/endian"
+ "github.com/apache/arrow/go/v14/arrow/internal/debug"
+ "github.com/apache/arrow/go/v14/arrow/memory"
+)
+
+const (
+ StringViewPrefixLen = 4
+ stringViewInlineSize = 12
+)
+
+func IsViewInline(length int) bool {
+ return length < stringViewInlineSize
+}
+
+// ViewHeader is a variable length string (utf8) or byte slice with
+// a 4 byte prefix and inline optimization for small values (12 bytes
+// or fewer). This is similar to Go's standard string but limited by
+// a length of Uint32Max and up to the first four bytes of the string
+// are copied into the struct. This prefix allows failing comparisons
+// early and can reduce CPU cache working set when dealing with short
+// strings.
+//
+// There are two situations:
+//
+// Entirely inlined string data
+// |----|------------|
+// ^ ^
+// | |
+// size inline string data, zero padded
+//
+// Reference into buffer
+// |----|----|----|----|
+// ^ ^ ^ ^
+// | | | |
+// size prefix buffer index and offset to
out-of-line portion
+//
+// Adapted from TU Munich's UmbraDB [1], Velox, DuckDB.
+//
+// [1]: https://db.in.tum.de/~freitag/papers/p29-neumann-cidr20.pdf
+type ViewHeader struct {
+ size uint32
Review Comment:
`size` is signed too now
```suggestion
size int
```
##########
go/arrow/datatype_viewheader_inline_tinygo.go:
##########
@@ -0,0 +1,35 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build !go1.20 && tinygo
+
+package arrow
+
+import (
+ "reflect"
+ "unsafe"
+
+ "github.com/apache/arrow/go/v14/arrow/internal/debug"
+)
+
+func (sh *ViewHeader) InlineData() (data string) {
Review Comment:
The utility of these over `InlineBytes` isn't clear to me
##########
go/arrow/array/bufferbuilder.go:
##########
@@ -151,3 +153,112 @@ func (b *bufferBuilder) unsafeAppend(data []byte) {
copy(b.bytes[b.length:], data)
b.length += len(data)
}
+
+type multiBufferBuilder struct {
+ refCount int64
+ blockSize int
Review Comment:
This should be settable so that users (and tests) can configure their own
block size
```suggestion
BlockSize int
```
##########
go/arrow/array/binarybuilder.go:
##########
@@ -370,6 +371,330 @@ func (b *BinaryBuilder) UnmarshalJSON(data []byte) error {
return b.Unmarshal(dec)
}
+const (
+ dfltBlockSize = 1 << 20 // 1 MB
+ viewValueSizeLimit uint32 = math.MaxUint32
+)
+
+type BinaryViewBuilder struct {
+ builder
+ dtype arrow.BinaryDataType
+
+ data *memory.Buffer
+ rawData []arrow.ViewHeader
+
+ blockBuilder multiBufferBuilder
+}
+
+func NewBinaryViewBuilder(mem memory.Allocator) *BinaryViewBuilder {
+ return &BinaryViewBuilder{
+ dtype: arrow.BinaryTypes.BinaryView,
+ builder: builder{
+ refCount: 1,
+ mem: mem,
+ },
+ blockBuilder: multiBufferBuilder{
+ refCount: 1,
+ blockSize: dfltBlockSize,
+ mem: mem,
+ },
+ }
+}
+
+func (b *BinaryViewBuilder) Type() arrow.DataType { return b.dtype }
+
+func (b *BinaryViewBuilder) Release() {
+ debug.Assert(atomic.LoadInt64(&b.refCount) > 0, "too many releases")
+
+ if atomic.AddInt64(&b.refCount, -1) != 0 {
+ return
+ }
+
+ if b.nullBitmap != nil {
+ b.nullBitmap.Release()
+ b.nullBitmap = nil
+ }
+ if b.data != nil {
+ b.data.Release()
+ b.data = nil
+ b.rawData = nil
+ }
+}
+
+func (b *BinaryViewBuilder) init(capacity int) {
+ b.builder.init(capacity)
+ b.data = memory.NewResizableBuffer(b.mem)
+ bytesN := arrow.StringHeaderTraits.BytesRequired(capacity)
+ b.data.Resize(bytesN)
+ b.rawData = arrow.StringHeaderTraits.CastFromBytes(b.data.Bytes())
+}
+
+func (b *BinaryViewBuilder) Resize(n int) {
+ nbuild := n
+ if n < minBuilderCapacity {
+ n = minBuilderCapacity
+ }
+
+ if b.capacity == 0 {
+ b.init(n)
+ return
+ }
+
+ b.builder.resize(nbuild, b.init)
+ b.data.Resize(arrow.StringHeaderTraits.BytesRequired(n))
+ b.rawData = arrow.StringHeaderTraits.CastFromBytes(b.data.Bytes())
+}
+
+func (b *BinaryViewBuilder) ReserveData(length int) {
+ if uint32(length) > viewValueSizeLimit {
+ panic(fmt.Errorf("%w: BinaryView or StringView elements cannot
reference strings larger than 4GB",
+ arrow.ErrInvalid))
+ }
+ b.blockBuilder.Reserve(int(length))
+}
+
+func (b *BinaryViewBuilder) Reserve(n int) {
+ b.builder.reserve(n, b.Resize)
+}
+
+func (b *BinaryViewBuilder) Append(v []byte) {
+ if uint32(len(v)) > viewValueSizeLimit {
+ panic(fmt.Errorf("%w: BinaryView or StringView elements cannot
reference strings larger than 4GB", arrow.ErrInvalid))
+ }
+
+ if !arrow.IsViewInline(len(v)) {
+ b.ReserveData(len(v))
+ }
+
+ b.Reserve(1)
+ b.UnsafeAppend(v)
+}
+
+// AppendString is identical to Append, only accepting a string instead
+// of a byte slice, avoiding the extra copy that would occur if you simply
+// did []byte(v).
+//
+// This is different than AppendValueFromString which exists for the
+// Builder interface, in that this expects raw binary data which is
+// appended as such. AppendValueFromString expects base64 encoded binary
Review Comment:
```suggestion
// appended unmodified. AppendValueFromString expects base64 encoded binary
```
##########
go/arrow/internal/testing/gen/random_array_gen.go:
##########
@@ -351,6 +351,40 @@ func (r *RandomArrayGenerator) LargeString(size int64,
minLength, maxLength int6
return bldr.NewArray()
}
+func (r *RandomArrayGenerator) StringView(size int64, minLength, maxLength
int64, nullProb float64) arrow.Array {
Review Comment:
specifying max buffer size might also be useful here
##########
go/arrow/datatype_viewheader.go:
##########
@@ -0,0 +1,138 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package arrow
+
+import (
+ "bytes"
+ "unsafe"
+
+ "github.com/apache/arrow/go/v14/arrow/endian"
+ "github.com/apache/arrow/go/v14/arrow/internal/debug"
+ "github.com/apache/arrow/go/v14/arrow/memory"
+)
+
+const (
+ StringViewPrefixLen = 4
+ stringViewInlineSize = 12
+)
+
+func IsViewInline(length int) bool {
+ return length < stringViewInlineSize
+}
+
+// ViewHeader is a variable length string (utf8) or byte slice with
+// a 4 byte prefix and inline optimization for small values (12 bytes
+// or fewer). This is similar to Go's standard string but limited by
+// a length of Uint32Max and up to the first four bytes of the string
+// are copied into the struct. This prefix allows failing comparisons
+// early and can reduce CPU cache working set when dealing with short
+// strings.
+//
+// There are two situations:
+//
+// Entirely inlined string data
+// |----|------------|
+// ^ ^
+// | |
+// size inline string data, zero padded
+//
+// Reference into buffer
+// |----|----|----|----|
+// ^ ^ ^ ^
+// | | | |
+// size prefix buffer index and offset to
out-of-line portion
+//
+// Adapted from TU Munich's UmbraDB [1], Velox, DuckDB.
+//
+// [1]: https://db.in.tum.de/~freitag/papers/p29-neumann-cidr20.pdf
+type ViewHeader struct {
+ size uint32
+ // the first 4 bytes of this are the prefix for the string
+ // if size <= StringHeaderInlineSize, then the entire string
+ // is in the data array and is zero padded.
+ // if size > StringHeaderInlineSize, the next 8 bytes are 2 uint32
+ // values which are the buffer index and offset in that buffer
+ // containing the full string.
+ data [stringViewInlineSize]byte
+}
+
+func (sh *ViewHeader) IsInline() bool {
+ return sh.size <= uint32(stringViewInlineSize)
+}
+
+func (sh *ViewHeader) Len() int { return int(sh.size) }
+func (sh *ViewHeader) Prefix() [StringViewPrefixLen]byte {
+ return *(*[4]byte)(unsafe.Pointer(&sh.data))
+}
+
+func (sh *ViewHeader) BufferIndex() uint32 {
+ return endian.Native.Uint32(sh.data[StringViewPrefixLen:])
+}
+
+func (sh *ViewHeader) BufferOffset() uint32 {
+ return endian.Native.Uint32(sh.data[StringViewPrefixLen+4:])
+}
+
+func (sh *ViewHeader) InlineBytes() (data []byte) {
+ debug.Assert(sh.IsInline(), "calling InlineBytes on non-inline
StringHeader")
Review Comment:
At the risk of offering an opinion well out of my expertise, would it be
preferable to return nil here instead?
```suggestion
if !sh.IsInline() {
return nil
}
```
##########
go/arrow/array/binarybuilder.go:
##########
@@ -370,6 +371,330 @@ func (b *BinaryBuilder) UnmarshalJSON(data []byte) error {
return b.Unmarshal(dec)
}
+const (
+ dfltBlockSize = 1 << 20 // 1 MB
+ viewValueSizeLimit uint32 = math.MaxUint32
+)
+
+type BinaryViewBuilder struct {
+ builder
+ dtype arrow.BinaryDataType
+
+ data *memory.Buffer
+ rawData []arrow.ViewHeader
+
+ blockBuilder multiBufferBuilder
+}
+
+func NewBinaryViewBuilder(mem memory.Allocator) *BinaryViewBuilder {
+ return &BinaryViewBuilder{
+ dtype: arrow.BinaryTypes.BinaryView,
+ builder: builder{
+ refCount: 1,
+ mem: mem,
+ },
+ blockBuilder: multiBufferBuilder{
+ refCount: 1,
+ blockSize: dfltBlockSize,
+ mem: mem,
+ },
+ }
+}
+
+func (b *BinaryViewBuilder) Type() arrow.DataType { return b.dtype }
+
+func (b *BinaryViewBuilder) Release() {
+ debug.Assert(atomic.LoadInt64(&b.refCount) > 0, "too many releases")
+
+ if atomic.AddInt64(&b.refCount, -1) != 0 {
+ return
+ }
+
+ if b.nullBitmap != nil {
+ b.nullBitmap.Release()
+ b.nullBitmap = nil
+ }
+ if b.data != nil {
+ b.data.Release()
+ b.data = nil
+ b.rawData = nil
+ }
+}
+
+func (b *BinaryViewBuilder) init(capacity int) {
+ b.builder.init(capacity)
+ b.data = memory.NewResizableBuffer(b.mem)
+ bytesN := arrow.StringHeaderTraits.BytesRequired(capacity)
+ b.data.Resize(bytesN)
+ b.rawData = arrow.StringHeaderTraits.CastFromBytes(b.data.Bytes())
+}
+
+func (b *BinaryViewBuilder) Resize(n int) {
+ nbuild := n
+ if n < minBuilderCapacity {
+ n = minBuilderCapacity
+ }
+
+ if b.capacity == 0 {
+ b.init(n)
+ return
+ }
+
+ b.builder.resize(nbuild, b.init)
+ b.data.Resize(arrow.StringHeaderTraits.BytesRequired(n))
+ b.rawData = arrow.StringHeaderTraits.CastFromBytes(b.data.Bytes())
+}
+
+func (b *BinaryViewBuilder) ReserveData(length int) {
+ if uint32(length) > viewValueSizeLimit {
+ panic(fmt.Errorf("%w: BinaryView or StringView elements cannot
reference strings larger than 4GB",
Review Comment:
After adopting signed sizes, this limit is slightly reduced
```suggestion
panic(fmt.Errorf("%w: BinaryView or StringView elements cannot
reference strings larger than 2GB",
```
##########
go/arrow/internal/arrdata/arrdata.go:
##########
@@ -1155,6 +1156,65 @@ func makeRunEndEncodedRecords() []arrow.Record {
return recs
}
+func makeStringViewRecords() []arrow.Record {
Review Comment:
It might be worthwhile to ensure at least one chunk has multiple data buffers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
