-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-38594: [Docs][C++][Gandiva] Document how to register Gandiva external functions #38763
Conversation
|
6eac33e
to
5d7ac32
Compare
@kou @js8544 It may be easier to read in a markdown rendered format, like the GitHub markdown viewer: https://github.com/niyue/arrow/blob/feature/gdv-external-func-doc/cpp/src/gandiva/doc/external_func.md I am not a native English speaker, and far from a decent doc writer, please feel free to let me know what should be revised. Thanks. |
5d7ac32
to
44841d7
Compare
Hello. While this is not about Gandiva, I see areas that seem to need changes based on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose changes for the readability of Markdown documents in terms of code. As I enjoy writing Markdown documents, I have looked into the recommended aspects for readability.
Thank you for the PR! I noticed that the doc is in Markdown format. It's fine for internal documentation. However, if it's targeted to external users (i.e. displayed on Arrow website), it has to be in |
As js8544 said, could you write this as a Sphinx documentation? If we want to use Markdown in our Sphinx documentation, we can discuss it as a separated issue. |
Sure. Let me port this to |
44841d7
to
3b21465
Compare
I re-format the document to be a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the document available at https://github.com/apache/arrow/blob/main/docs/source/cpp/arrays.rst?plain=1, I would like to make a suggestion.
Hi there, is there anything that we need to revise for this doc? Please feel free to suggest and I will try my best to polish it. Thanks. |
Sorry I totally forgot about this PR 🤦. I'll have a look tomorrow! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a link to this page to docs/source/cpp/gandiva.rst
?
@AlenkaF Do you have any suggestion how to organize Gandiva documentations?
@github-actions crossbow submit preview-docs |
Revision: 892ae01 Submitted crossbow builds: ursacomputing/crossbow @ actions-8420909df5
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The doc looks good. Just a couple of nits.
The structure already looks pretty good.
|
…rnal function integration.
d3bca9d
to
90968bd
Compare
I updated the overall flow chart, and remove some internal classes from the chart and add external C functions to the chart |
@github-actions crossbow submit preview-docs |
Revision: 90968bd Submitted crossbow builds: ursacomputing/crossbow @ actions-11a52e7806
|
@AlenkaF I am new to authoring docs in arrow, could you please help how I could see the results of I expect this command to generate some docs for previewing, but I cannot find the link I could use to preview the doc in CI results. Currently I use github's file preview function to preview the basic skeleton of the |
Sure! When the build is finished you navigate to the Summary part of the Crossbow build and on the bottom you will find the preview of the docs. See link for better info: https://arrow.apache.org/docs/dev/developers/documentation.html#building-a-docs-preview-in-a-pull-request ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
I'll merge this. Thanks! |
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit f3cdd81. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
… external functions (apache#38763) ### Rationale for this change Provide a basic documentation on how to register and develop Gandiva external functions, including IR functions and C functions. ### What changes are included in this PR? A markdown doc providing an overview and detailed steps for integrating external functions into Gandiva. ### Are these changes tested? It is a doc change ### Are there any user-facing changes? No * Closes: apache#38594 Lead-authored-by: Yue Ni <[email protected]> Co-authored-by: Yue <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Rationale for this change
Provide a basic documentation on how to register and develop Gandiva external functions, including IR functions and C functions.
What changes are included in this PR?
A markdown doc providing an overview and detailed steps for integrating external functions into Gandiva.
Are these changes tested?
It is a doc change
Are there any user-facing changes?
No