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.

Sign up for GitHub

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

Jump to bottom

GH-20415: [Go] Kernel Input Type for RLE #14146

Merged
merged 10 commits into from
Jan 30, 2023
Merged

GH-20415: [Go] Kernel Input Type for RLE #14146

merged 10 commits into from
Jan 30, 2023

Conversation

zeroshade
Copy link
Member

@zeroshade zeroshade commented Sep 15, 2022 •
edited by github-actions bot
Loading

@zeroshade
Copy link
Member Author

CC @zagto

@github-actions
Copy link

https://issues.apache.org/jira/browse/ARROW-17750

westonpace
westonpace reviewed Sep 22, 2022
View reviewed changes
Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

Looks fairly straightforward, though I don't truly understand go

go/arrow/array/concat.go
@@ -42,7 +43,12 @@ func Concatenate(arrs []arrow.Array, mem memory.Allocator) (result arrow.Array,

defer func() {
if pErr := recover(); pErr != nil {
err = fmt.Errorf("arrow/concat: unknown error: %v", pErr)
switch e := pErr.(type) {
case error:
Copy link
Member

Choose a reason for hiding this comment

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

I don't really know go but why is it important to distinguish between these two error types? Adding unknown error doesn't seem to contribute too much to a user's understanding of the error.

Copy link
Member Author

Choose a reason for hiding this comment

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

using %w wraps the error that was panic'd so that someone receiving the error can use errors.Is or errors.As if they desire to react differently based on the type of the error (such as arrow.ErrInvalid vs arrow.ErrIndex vs arrow.ErrNotImplemented etc...) basically it creates an error chain

In the case of the "unknown error" it means that we had a panic with a string rather than a proper error instance. Though I agree that the text "unknown error" doesn't contribute much to a user's understanding here, so i'll just get rid of that.

go/arrow/array/concat.go Outdated Show resolved Hide resolved
go/arrow/array/concat_test.go Outdated Show resolved Hide resolved
go/arrow/array/encoded.go Outdated Show resolved Hide resolved
go/arrow/array/array.go Outdated Show resolved Hide resolved
go/arrow/rle/rle_utils_test.go Outdated Show resolved Hide resolved
@zeroshade zeroshade force-pushed the rle-kernel-input branch 2 times, most recently from 7bbac16 to f26c1aa Compare October 21, 2022 16:07
@zeroshade zeroshade marked this pull request as ready for review January 26, 2023 19:00
lidavidm
lidavidm approved these changes Jan 26, 2023
View reviewed changes
go/arrow/compute/internal/exec/kernel.go Outdated Show resolved Hide resolved
@zeroshade
Copy link
Member Author

I'll merge this at EOD if no one has any objections/comments

@zeroshade zeroshade changed the title ARROW-17750: [Go] Kernel Input Type for RLE GH-20415: [Go] Kernel Input Type for RLE Jan 30, 2023
@github-actions
Copy link

@asfimport asfimport mentioned this pull request Jan 30, 2023
Closed
@github-actions
Copy link

⚠️ GitHub issue #20415 has been automatically assigned in GitHub to PR creator.

@zeroshade zeroshade merged commit 27cea43 into apache:master Jan 30, 2023
@zeroshade zeroshade deleted the rle-kernel-input branch January 30, 2023 16:37
@ursabot
Copy link

ursabot commented Jan 30, 2023

Benchmark runs are scheduled for baseline = d422137 and contender = 27cea43. 27cea43 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] test-mac-arm
[Failed] ursa-i9-9960x
[Failed] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 27cea436 ec2-t3-xlarge-us-east-2
[Failed] 27cea436 test-mac-arm
[Failed] 27cea436 ursa-i9-9960x
[Failed] 27cea436 ursa-thinkcentre-m75q
[Finished] d422137d ec2-t3-xlarge-us-east-2
[Failed] d422137d test-mac-arm
[Failed] d422137d ursa-i9-9960x
[Failed] d422137d 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

sjperkins pushed a commit to sjperkins/arrow that referenced this pull request Feb 10, 2023
@zeroshade @sjperkins
apacheGH-20415: [Go] Kernel Input Type for RLE (apache#14146)
d29fd92 
Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
gringasalpastor pushed a commit to gringasalpastor/arrow that referenced this pull request Feb 17, 2023
@zeroshade
apacheGH-20415: [Go] Kernel Input Type for RLE (apache#14146)
7a5d4e7 
Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonpace westonpace westonpace left review comments

@lidavidm lidavidm lidavidm approved these changes

@cyb70289 cyb70289 Awaiting requested review from cyb70289

Assignees
No one assigned
Labels
Component: Go
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Go] Kernel Input Type Matcher for RLE
4 participants
@zeroshade @ursabot @lidavidm @westonpace