Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-17276: [Go][Integration] Implement IPC handling for union type #13806

Merged
merged 22 commits into from
Aug 8, 2022

Conversation

zeroshade
Copy link
Member

With this, the Go implementation finally fully supports IPC handling for All the Arrow DataTypes!

@github-actions
Copy link

github-actions bot commented Aug 5, 2022

@zeroshade
Copy link
Member Author

Pinging @wolfeidau for the last in this series of changes to get Go support for data types :)

Copy link
Contributor

@wolfeidau wolfeidau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few small things and suggestions.

Again this is pretty amazing work.

go/arrow/ipc/file_reader.go Show resolved Hide resolved
go/arrow/ipc/file_reader.go Outdated Show resolved Hide resolved
go/arrow/ipc/file_reader.go Outdated Show resolved Hide resolved
go/arrow/ipc/writer.go Outdated Show resolved Hide resolved
@zeroshade
Copy link
Member Author

Thanks for the lookover @wolfeidau !

@zeroshade zeroshade changed the title ARROW-17276: [Go][Integratoin] Implement IPC handling for union type ARROW-17276: [Go][Integration] Implement IPC handling for union type Aug 6, 2022
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Some comments below, but looks good generally.

go/arrow/internal/arrdata/arrdata.go Show resolved Hide resolved
Comment on lines +1192 to +1193
if arr.Offset == nil {
offsets = []byte{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that actually happen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It happens in the case of an empty array, which i found when it crashed during archery integration testing lol.

go/arrow/internal/arrjson/arrjson_test.go Show resolved Hide resolved

switch field.Length() {
case 0:
buffers = append(buffers, memory.NewBufferBytes([]byte{}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the type codes buffer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. In the case of an empty array the type codes buffer is empty.

go/arrow/ipc/writer.go Outdated Show resolved Hide resolved
go/arrow/ipc/writer.go Outdated Show resolved Hide resolved
Comment on lines +811 to +822
for i, c := range codes {
if offsets[c] == -1 {
// offsets are guaranteed to be increasing according to the spec
// so the first offset we find for a child is the initial offset
// and will become the "0" for this child.
offsets[c] = unshiftedOffsets[i]
shiftedOffsets[i] = 0
} else {
shiftedOffsets[i] = unshiftedOffsets[i] - offsets[c]
}
lengths[c] = maxI32(lengths[c], shiftedOffsets[i]+1)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! This may serve as an inspiration for Arrow C++, in turn :-)

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zeroshade zeroshade merged commit d171b6c into apache:master Aug 8, 2022
@zeroshade zeroshade deleted the arrow-17276-ipc-unions branch August 8, 2022 17:20
@ursabot
Copy link

ursabot commented Aug 8, 2022

Benchmark runs are scheduled for baseline = b1d36c0 and contender = d171b6c. d171b6c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.17% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.27% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d171b6c3 ec2-t3-xlarge-us-east-2
[Finished] d171b6c3 test-mac-arm
[Finished] d171b6c3 ursa-i9-9960x
[Finished] d171b6c3 ursa-thinkcentre-m75q
[Finished] b1d36c02 ec2-t3-xlarge-us-east-2
[Failed] b1d36c02 test-mac-arm
[Finished] b1d36c02 ursa-i9-9960x
[Finished] b1d36c02 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants