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

[C++] Adding recipe for custom compute functions #227

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

drin
Copy link

@drin drin commented Jul 8, 2022

This recipe shows the major portions of a custom, or new, compute
function:

  • defining a compute kernel
  • creating a function instance
  • associating the kernel with the function
  • registering the function in a registry
  • calling the function

The aliases are to keep the code as readable as possible, and also to frontload the various dependencies to make it obvious to the reader.

The kernel implementation here is essentially the same as the new FastHash32 function in progress of being added. The reason for this is that there are examples of much simpler functions (AbsoluteValue), and this recipe also helps the reader understand how to call other functions and how to structure non-trivial returns.

@drin drin changed the title 174: Adding recipe for custom compute functions Adding recipe for custom compute functions Jul 8, 2022
@drin drin marked this pull request as ready for review July 8, 2022 02:04
@wjones127
Copy link
Member

In order to get these recipes to appear on https://arrow.apache.org/cookbook/cpp/index.html, we should add a compute.rst file in cpp/source that provides some narrative around these examples.

@lidavidm
Copy link
Member

Thanks for adding this, I'll take a look.

Side note: we should probably just set up clang-format for C++ examples…

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks, this is a great example. I left some comments, but as noted we also need an reST file to get this to appear in the Cookbook itself. Of course, this probably needs to wait on 9.0.0 too

cpp/code/compute_fn.cc Outdated Show resolved Hide resolved
cpp/code/compute_fn.cc Outdated Show resolved Hide resolved
cpp/code/compute_fn.cc Outdated Show resolved Hide resolved
cpp/code/compute_fn.cc Outdated Show resolved Hide resolved
cpp/code/compute_fn.cc Outdated Show resolved Hide resolved
@drin
Copy link
Author

drin commented Jul 26, 2022

thanks for the comments! I will get around to the reST file today. I was putting it off a bit but today feels like a good day for it.

drin added 2 commits July 27, 2022 15:28
This recipe shows the major portions of a custom, or new, compute
function:
- defining a compute kernel
- creating a function instance
- associating the kernel with the function
- registering the function in a registry
- calling the function
Mostly minor changes. Only major change is replacing the use of
hashing32 from key_hash.c with ScalarHelper from hashing.h
@drin
Copy link
Author

drin commented Jul 27, 2022

rebased to try and build the cookbooks, but I am not sure how to build the documentation for the C++ code

compute.rst is documentation to describe compute functions. For now it
describes how to define and register a compute function. Still a work in
progress. Updates to compute_fn.cc are to reflect the description
provided in compute.rst
@drin
Copy link
Author

drin commented Jul 28, 2022

I am not sure how to add whole function definitions to the recipe instead of portions of the body.

For example:

Exec(KernelContext *ctx, const ExecSpan &input_arg, ExecResult *out) {
StartRecipe("DefineAComputeKernel");

cpp/source/compute.rst Outdated Show resolved Hide resolved
cpp/source/compute.rst Outdated Show resolved Hide resolved
cpp/source/compute.rst Outdated Show resolved Hide resolved
cpp/source/compute.rst Outdated Show resolved Hide resolved
drin added 3 commits July 28, 2022 14:15
Also added namespace and :class: and :func: annotations. I think it
doesn't matter in the cookbook, but it links nicely when in the arrow
docs themselves. I'm kind of curious how it renders in the cookbook
Trying to see if moving these recipe labels captures C++ code blocks
outside of function bodies
I am trying the literalinclude directive first before trying to use
recipe directives in comments
@drin drin requested a review from lidavidm July 28, 2022 21:53
@drin drin changed the title Adding recipe for custom compute functions [C++] Adding recipe for custom compute functions Aug 20, 2022
@drin
Copy link
Author

drin commented Aug 24, 2022

@wjones127 or @lidavidm if either of you have time, could you check this out?

I wasn't able to locally confirm that code excerpt directives worked correctly, but I am hoping to reference this cookbook from documentation for authoring compute kernels

@lidavidm
Copy link
Member

I kicked off CI, but not sure if I'll have time to look in more detail right now

@drin
Copy link
Author

drin commented Aug 29, 2022

I am having trouble setting up a build to figure out why it couldn't find the recipe, so I'm just going to shelve it for a bit.

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

Successfully merging this pull request may close these issues.

4 participants