[GitHub] [arrow] zeroshade commented on a diff in pull request #35769: GH-35627: [Go][Format][Integration] Add StringView/BinaryView to Go implementation

["zeroshade (via GitHub)" <gi...@apache.org>]

zeroshade commented on code in PR #35769:
URL: https://github.com/apache/arrow/pull/35769#discussion_r1205999970


##########
go/arrow/datatype_binary.go:
##########
@@ -83,16 +93,168 @@ func (t *LargeStringType) Layout() DataTypeLayout {
 func (t *LargeStringType) OffsetTypeTraits() OffsetTraits { return Int64Traits }
 func (LargeStringType) IsUtf8() bool                      { return true }
 
+const (
+	StringHeaderPrefixLen  = 4
+	stringHeaderInlineSize = 12
+)
+
+func IsStringHeaderInline(length int) bool {
+	return length < stringHeaderInlineSize
+}
+
+// StringHeader 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:
+//
+//	Short string   |----|----|--------|
+//	                ^    ^    ^
+//	                |    |    |
+//	              size prefix remaining in-line portion, zero padded
+//
+//	IO Long String |----|----|----|----|
+//	                ^    ^     ^     ^
+//	                |    |     |     |
+//	              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 StringHeader struct {

Review Comment:
   The only way to modify the alignment requirements of a struct in go is to modify the types of its fields. It is going to pick the alignment for the struct based on the largest alignment needed for a given member. In this case using `unsafe.Alignof(arrow.StringHeader{})` reports an alignment of 4 bytes. That said, as long as the size of 16 bytes for the struct is maintained, I don't think there'll be an issue? In addition I believe we explicitly handle the alignment for IPC input/output.
   
   I guess there could be an issue with the C-Data interface if the buffer ends up not 8-byte aligned? but then again, I don't think you added a proposal for the new format string to indicate this across the C-Data interface yet :smile:



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org