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

GH-38594: [Docs][C++][Gandiva] Document how to register Gandiva external functions #38763

Merged
merged 6 commits into from
Dec 1, 2023

Conversation

niyue
Copy link
Contributor

@niyue niyue commented Nov 17, 2023

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

Copy link

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

@niyue
Copy link
Contributor Author

niyue commented Nov 17, 2023

@kou @js8544
I draft a simple markdown doc to describe how Gandiva external function could be integrated. Could you please help to review? Thanks.

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.

@niyue niyue force-pushed the feature/gdv-external-func-doc branch from 5d7ac32 to 44841d7 Compare November 17, 2023 14:53
@llama90
Copy link
Contributor

llama90 commented Nov 17, 2023

Hello.

While this is not about Gandiva, I see areas that seem to need changes based on the .md files written in Arrow. reference: README.md

Copy link
Contributor

@llama90 llama90 left a 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.

cpp/src/gandiva/doc/external_func.md Outdated Show resolved Hide resolved
cpp/src/gandiva/doc/external_func.md Outdated Show resolved Hide resolved
cpp/src/gandiva/doc/external_func.md Outdated Show resolved Hide resolved
cpp/src/gandiva/doc/external_func.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 17, 2023
@js8544
Copy link
Collaborator

js8544 commented Nov 17, 2023

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 rst format. See the various docs in docs/source/cpp/ directory for examples.

@kou
Copy link
Member

kou commented Nov 17, 2023

As js8544 said, could you write this as a Sphinx documentation?
We have a page for Gandiva: https://github.com/apache/arrow/blob/main/docs/source/cpp/gandiva.rst
We can extend it or create a new page and refer the new page from the existing page.

If we want to use Markdown in our Sphinx documentation, we can discuss it as a separated issue.
(https://github.com/apache/arrow-flight-sql-postgresql uses Sphinx https://arrow.apache.org/flight-sql-postgresql/current/ but it uses Markdown not reStructuredText. So it's not hard to use Markdown in Sphinx.)

@niyue
Copy link
Contributor Author

niyue commented Nov 18, 2023

Sure. Let me port this to rst today.

@niyue
Copy link
Contributor Author

niyue commented Nov 18, 2023

I re-format the document to be a rst document, and put it under the docs/source/cpp directory now. Please check it out. Thanks.

Copy link
Contributor

@llama90 llama90 left a 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.

docs/source/cpp/gandiva_external_func.rst Outdated Show resolved Hide resolved
docs/source/cpp/gandiva_external_func.rst Outdated Show resolved Hide resolved
@niyue
Copy link
Contributor Author

niyue commented Nov 28, 2023

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.

@js8544
Copy link
Collaborator

js8544 commented Nov 28, 2023

Sorry I totally forgot about this PR 🤦. I'll have a look tomorrow!

Copy link
Member

@kou kou left a 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?

docs/source/cpp/gandiva_external_func.rst Outdated Show resolved Hide resolved
docs/source/cpp/gandiva_external_func.rst Outdated Show resolved Hide resolved
@kou
Copy link
Member

kou commented Nov 29, 2023

@github-actions crossbow submit preview-docs

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Nov 29, 2023
Copy link

Revision: 892ae01

Submitted crossbow builds: ursacomputing/crossbow @ actions-8420909df5

Task Status
preview-docs Github Actions

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 29, 2023
Copy link
Collaborator

@js8544 js8544 left a 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.

docs/source/cpp/gandiva_external_func.rst Outdated Show resolved Hide resolved
docs/source/cpp/gandiva_external_func.rst Outdated Show resolved Hide resolved
docs/source/cpp/gandiva_external_func.rst Outdated Show resolved Hide resolved
docs/source/cpp/gandiva_external_func.rst Outdated Show resolved Hide resolved
docs/source/cpp/gandiva_external_func.rst Outdated Show resolved Hide resolved
docs/source/cpp/gandiva_external_func.rst Outdated Show resolved Hide resolved
@AlenkaF
Copy link
Member

AlenkaF commented Nov 29, 2023

The structure already looks pretty good.
I would suggest, though, to organise it in a similar way to acero docs:

@niyue niyue force-pushed the feature/gdv-external-func-doc branch from d3bca9d to 90968bd Compare November 29, 2023 12:43
@niyue
Copy link
Contributor Author

niyue commented Nov 29, 2023

@AlenkaF

on the index page list all the subpages with toctree directive (.. toctree::)
It would be good to maybe move more content from docs/source/cpp/gandiva.rst to subpages, save the subpages in the gandiva folder and add them to the index page toctree

  • I move the three sections in the index page to a new sub page
  • I added the toctree to the index page, but currently there is not too much content for gandiva so I still keep the brief summary sections above, and added the toctree below. Let me know if this is not desirable.

The overall flow chart from the PR

I updated the overall flow chart, and remove some internal classes from the chart and add external C functions to the chart

@AlenkaF
Copy link
Member

AlenkaF commented Nov 29, 2023

@github-actions crossbow submit preview-docs

Copy link

Revision: 90968bd

Submitted crossbow builds: ursacomputing/crossbow @ actions-11a52e7806

Task Status
preview-docs Github Actions

@niyue
Copy link
Contributor Author

niyue commented Nov 29, 2023

@AlenkaF I am new to authoring docs in arrow, could you please help how I could see the results of crossbow submit preview-docs? Thanks.

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 rst file, but I am not able to verify some special rst constructs like page links with that.

@AlenkaF
Copy link
Member

AlenkaF commented Nov 29, 2023

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 ;)

@AlenkaF
Copy link
Member

AlenkaF commented Nov 29, 2023

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

docs/source/cpp/gandiva/external_func.png Outdated Show resolved Hide resolved
docs/source/cpp/gandiva/external_func.rst Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting merge Awaiting merge awaiting changes Awaiting changes and removed awaiting change review Awaiting change review awaiting merge Awaiting merge labels Nov 29, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 30, 2023
@kou
Copy link
Member

kou commented Dec 1, 2023

I'll merge this. Thanks!

@kou kou merged commit f3cdd81 into apache:main Dec 1, 2023
9 checks passed
@kou kou removed the awaiting change review Awaiting change review label Dec 1, 2023
Copy link

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.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
… 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]>
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.

[Docs][C++][Gandiva] Document how to register external functions
5 participants