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-17455: [Go] Function and Kernel execution architecture #13964

Merged
merged 6 commits into from
Aug 25, 2022

Conversation

zeroshade
Copy link
Member

In addition to implementing the function execution architecture, this also shifts some files around for better package naming and scoping. A new package exec is created inside of compute/internal/ to make it easier to scope other internal-only methods / functionality such as the kernel implementations themselves.

This is a fairly large change because so much is interconnected, but it also includes extensive tests covering ~85% of the internal/exec package and nearly 80% of the entire compute package. More tests will be added as functionality is added.

This implements the interface for Kernels along with a `KernelSignature` struct and the type matching for `InputType` and `OutputType` via a `TypeMatcher` interface. Along with tests for all of the above.

Authored-by: Matt Topol <[email protected]>
Signed-off-by: Matt Topol <[email protected]>
@github-actions
Copy link

Copy link
Contributor

@cyb70289 cyb70289 left a comment

Choose a reason for hiding this comment

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

LGTM.

Will be great if there's a simple kernel to illustrate all pieces working together.

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

ch := make(chan Datum, 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

10 looks without strong support, but I don't have better suggestion.

Is channel strictly necessary here? I think channel make the code easier to read than C++ which collects all results with a datum accumulator.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're correct that a channel is not strictly necessary here, I did it for the reason you suggested: it makes it more readable than the C++ using a datum accumulator.

I'll make the 10 a configurable setting via the ExecCtx and just leave 10 as the default for now. Ultimately i just wanted to make sure that neither the Exec nor the WrapResults got blocked in most cases on how quickly the execution works.

@zeroshade
Copy link
Member Author

@cyb70289 you can see some simple kernels in the tests which are used to test the infrastructure 😄

@zeroshade
Copy link
Member Author

@cyb70289 also following this I'm already starting to implement kernels for various functions, so aside from the tests there'll also be lots of implemented kernels to use for examples.

@zeroshade zeroshade merged commit 7e7b8e1 into apache:master Aug 25, 2022
@zeroshade zeroshade deleted the arrow-17455-exec-architecture branch August 25, 2022 16:05
@ursabot
Copy link

ursabot commented Aug 25, 2022

Benchmark runs are scheduled for baseline = 48e5e16 and contender = 7e7b8e1. 7e7b8e1 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.44% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.27% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 7e7b8e1f ec2-t3-xlarge-us-east-2
[Finished] 7e7b8e1f test-mac-arm
[Failed] 7e7b8e1f ursa-i9-9960x
[Finished] 7e7b8e1f ursa-thinkcentre-m75q
[Finished] 48e5e161 ec2-t3-xlarge-us-east-2
[Failed] 48e5e161 test-mac-arm
[Failed] 48e5e161 ursa-i9-9960x
[Finished] 48e5e161 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

anjakefala pushed a commit to anjakefala/arrow that referenced this pull request Aug 31, 2022
…13964)

In addition to implementing the function execution architecture, this also shifts some files around for better package naming and scoping. A new package `exec` is created inside of `compute/internal/` to make it easier to scope other internal-only methods / functionality such as the kernel implementations themselves.

This is a fairly large change because so much is interconnected, but it also includes extensive tests covering ~85% of the `internal/exec` package and nearly 80% of the entire `compute` package. More tests will be added as functionality is added.

Authored-by: Matt Topol <[email protected]>
Signed-off-by: Matt Topol <[email protected]>
zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
…13964)

In addition to implementing the function execution architecture, this also shifts some files around for better package naming and scoping. A new package `exec` is created inside of `compute/internal/` to make it easier to scope other internal-only methods / functionality such as the kernel implementations themselves.

This is a fairly large change because so much is interconnected, but it also includes extensive tests covering ~85% of the `internal/exec` package and nearly 80% of the entire `compute` package. More tests will be added as functionality is added.

Authored-by: Matt Topol <[email protected]>
Signed-off-by: Matt Topol <[email protected]>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
…13964)

In addition to implementing the function execution architecture, this also shifts some files around for better package naming and scoping. A new package `exec` is created inside of `compute/internal/` to make it easier to scope other internal-only methods / functionality such as the kernel implementations themselves.

This is a fairly large change because so much is interconnected, but it also includes extensive tests covering ~85% of the `internal/exec` package and nearly 80% of the entire `compute` package. More tests will be added as functionality is added.

Authored-by: Matt Topol <[email protected]>
Signed-off-by: Matt Topol <[email protected]>
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.

3 participants