GH-38718: [Go][Format][Integration] Add StringView/BinaryView to Go implementation #35769

zeroshade · 2023-05-25T17:58:45Z

Rationale for this change

See #35628 for the rationale and description of the StringView/BinaryView array types.

This change is adding Go as a second implementation of it.

What changes are included in this PR?

Add Array Types for StringView and BinaryView along with StringViewType and BinaryViewType and necessary enums and builders. These arrays can be round tripped through JSON and IPC.

Are these changes tested?

Yes, unit tests have been added and integration tests run

Closes: #38718
Closes: [Go][Integration] Add support for Utf8View to the go implementation #38718

github-actions · 2023-05-25T17:59:06Z

Closes: [Format] Add string view type to the arrow format #35627

go/arrow/datatype_binary.go

lidavidm · 2023-05-26T15:02:31Z

go/arrow/datatype_binary.go

+func (*BinaryViewType) binary()               {}
+func (*BinaryViewType) view()                 {}


What are these for?

convenience interfaces. in datatype.go there's BinaryDataType which defines an interface adding IsUtf8() bool which requires a binary() method so that you can easily write code that works for all of these binary interfaces via type switches or interfaces.

Then I added this view() method for a BinaryViewDataType so you can write code targeting both BinaryView and StringView without needing to duplicate code by just using the interface in the type switch.

zeroshade · 2023-06-15T21:12:38Z

CC @felipecrv @benibus

lquerel

Interesting optimization. I'm not entirely sure why these new types of builders/arrays are named XXXView, considering that, from what I understand, they store small strings inline in a fixed width data array and encode the index/offset for longer strings, with the remainder of the string being stored externally. However, I don't have a better name to suggest at the moment :)

In general, as a user of this library, I would find it useful to add more comments to methods whose usage isn't immediately clear from the name.

lquerel · 2023-06-20T20:55:15Z

go/arrow/array/binary.go

+	return &a.values[a.array.data.offset+i]
+}
+
+func (a *BinaryView) Value(i int) []byte {


Since there is no compiler guarantee in Go that the slice returned by this method will remain unchanged (whether intentionally or unintentionally) by users of this API, I would suggest adding a comment to this method. This comment should clearly specify that it's unsafe to make any kind of changes to the returned slice.

This is a concern present everywhere in Arrow though, so a comment here could be understood as implying that places without a comment like this allow buffers to be mutated.

This is probably a reflex from my Rust experience where this slice will be immutable. In the context of Go, I agree with you that sporadically adding a comment might be counterproductive if we do not apply this globally.

At the top level, there do already exist comments that state that it is intended that all Arrow Arrays be immutable. I agree with the concern that adding a comment here specifically could be counterproductive. If you can think of a good place to put such a comment that would be more universal, I'd be more than happy to do so.

go/arrow/array/binarybuilder.go

go/arrow/array/binary.go

go/arrow/datatype_stringheader.go

candiduslynx

a first round

dev/archery/archery/integration/datagen.py

format/Schema.fbs

go/arrow/datatype.go

go/arrow/datatype_binary.go

candiduslynx · 2023-06-21T07:26:30Z

go/arrow/datatype_stringheader_inline_tinygo.go

+func (sh *StringHeader) InlineData() (data string) {
+	debug.Assert(sh.IsInline(), "calling InlineData on non-inline StringHeader")
+
+	h := (*reflect.StringHeader)(unsafe.Pointer(&data))


any diff with go1.19 implementation?

yea. The Len in the StringHeader for tinygo is a uintptr instead of an int which is what go1.19 has. Compare line 33 of each file.

how sad, but now I see why it's warranted.
However, a comment to note the diff for those implementations would be nice to have (not required, but nice to have nonetheless).

go/arrow/internal/flatbuf/BinaryView.go

go/arrow/type_traits_string_view.go

go/arrow/array/binary.go

Co-authored-by: Alex Shcherbakov <candiduslynx@users.noreply.github.com>

go/arrow/datatype_viewheader.go

Co-authored-by: Alex Shcherbakov <candiduslynx@users.noreply.github.com>

go/arrow/array/binary.go

go/arrow/array/bufferbuilder.go

go/arrow/datatype_viewheader.go

conbench-apache-arrow · 2023-11-14T22:43:02Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 26149d9.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…o Go implementation (apache#35769) ### Rationale for this change See apache#35628 for the rationale and description of the StringView/BinaryView array types. This change is adding Go as a second implementation of it. ### What changes are included in this PR? Add Array Types for `StringView` and `BinaryView` along with `StringViewType` and `BinaryViewType` and necessary enums and builders. These arrays can be round tripped through JSON and IPC. ### Are these changes tested? Yes, unit tests have been added and integration tests run * Closes: [apache#38718](apache#38718) * Closes: apache#38718 Lead-authored-by: Matt Topol <zotthewizard@gmail.com> Co-authored-by: Alex Shcherbakov <candiduslynx@users.noreply.github.com> Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>

zeroshade requested review from lidavidm and bkietz May 25, 2023 17:58

github-actions bot added Component: Documentation Component: Go awaiting committer review Awaiting committer review labels May 25, 2023

bkietz reviewed May 25, 2023

View reviewed changes

go/arrow/datatype_binary.go Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes labels May 25, 2023

lidavidm reviewed May 26, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 26, 2023

zeroshade force-pushed the string-view branch from f09bcc8 to e3146bb Compare June 15, 2023 16:45

zeroshade marked this pull request as ready for review June 15, 2023 16:45

zeroshade requested review from assignUser, kou and raulcd as code owners June 15, 2023 16:45

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 15, 2023

zeroshade requested a review from westonpace June 15, 2023 21:12

lquerel approved these changes Jun 20, 2023

View reviewed changes

felipecrv reviewed Jun 21, 2023

View reviewed changes

go/arrow/array/binary.go Show resolved Hide resolved

felipecrv reviewed Jun 21, 2023

View reviewed changes

go/arrow/datatype_stringheader.go Outdated Show resolved Hide resolved

candiduslynx reviewed Jun 21, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Jun 21, 2023

Update go/arrow/datatype.go

a84ee2e

Co-authored-by: Alex Shcherbakov <candiduslynx@users.noreply.github.com>

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 30, 2023

updates from feedback

be3b3a5

candiduslynx approved these changes Oct 31, 2023

View reviewed changes

go/arrow/datatype_viewheader.go Outdated Show resolved Hide resolved

zeroshade and others added 2 commits November 13, 2023 12:01

Update go/arrow/datatype_viewheader.go

e2bbe6f

Co-authored-by: Alex Shcherbakov <candiduslynx@users.noreply.github.com>

Merge branch 'main' into string-view

cab4899

zeroshade requested a review from bkietz November 13, 2023 17:03

zeroshade added 2 commits November 13, 2023 12:06

update with rebase

9db6557

fix version imports

460c06e

candiduslynx suggested changes Nov 13, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 13, 2023

fix formatting, updates from feedback, add tests

8e41840

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Nov 13, 2023

candiduslynx approved these changes Nov 14, 2023

View reviewed changes

bkietz approved these changes Nov 14, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Nov 14, 2023

bkietz changed the title ~~GH-35627: [Go][Format][Integration] Add StringView/BinaryView to Go implementation~~ GH-38718: [Go][Format][Integration] Add StringView/BinaryView to Go implementation Nov 14, 2023

bkietz merged commit 26149d9 into apache:main Nov 14, 2023
28 of 29 checks passed

bkietz removed the awaiting merge Awaiting merge label Nov 14, 2023

zeroshade deleted the string-view branch November 14, 2023 20:55

urvishdesai mentioned this pull request Nov 28, 2023

Support for Utf8View in the Rust library jorgecarleitao/arrow2#1596

Open

bkietz mentioned this pull request Nov 30, 2023

[Go][Integration] Add support for C abi Utf8View to the go implementation #39013

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38718: [Go][Format][Integration] Add StringView/BinaryView to Go implementation #35769

GH-38718: [Go][Format][Integration] Add StringView/BinaryView to Go implementation #35769

zeroshade commented May 25, 2023 •

edited by github-actions bot

Loading

github-actions bot commented May 25, 2023

lidavidm May 26, 2023

zeroshade May 26, 2023

zeroshade commented Jun 15, 2023

lquerel left a comment

lquerel Jun 20, 2023

felipecrv Jun 20, 2023

lquerel Jun 21, 2023

zeroshade Jun 21, 2023

candiduslynx left a comment

candiduslynx Jun 21, 2023

zeroshade Jun 21, 2023

candiduslynx Jun 22, 2023

conbench-apache-arrow bot commented Nov 14, 2023

		func (*BinaryViewType) binary() {}
		func (*BinaryViewType) view() {}

GH-38718: [Go][Format][Integration] Add StringView/BinaryView to Go implementation #35769

GH-38718: [Go][Format][Integration] Add StringView/BinaryView to Go implementation #35769

Conversation

zeroshade commented May 25, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

github-actions bot commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeroshade commented Jun 15, 2023

lquerel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candiduslynx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Nov 14, 2023

zeroshade commented May 25, 2023 •

edited by github-actions bot

Loading