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-40318: [C++][Docs] Add documentation of array factories #40373

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Mar 5, 2024

Rationale for this change

These factory functions are generally useful and available, so documenting them helps external users find them without having to search the source code.

What changes are included in this PR?

This PR adds the array factories in arrow/array/util.h into a doxygen group for array factories and adds that group to the Sphinx C++ API documentation.

Are these changes tested?

I built the docs locally to verify.

Are there any user-facing changes?

Nothing to the API, only docs.

Copy link

github-actions bot commented Mar 5, 2024

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

@vyasr
Copy link
Contributor Author

vyasr commented Mar 5, 2024

Question, are there bots set up to close the associated issue when this PR merges? Normally I would comment with one of Github's recognized keywords (Closes #X) but I'm guessing that's not necessary here.

@vyasr
Copy link
Contributor Author

vyasr commented Mar 5, 2024

The CI test failures look odd and unrelated to the changeset.

@kou kou changed the title GH-40318:[C++] Add documentation of array factories GH-40318: [C++][Docs] Add documentation of array factories Mar 6, 2024
@kou
Copy link
Member

kou commented Mar 6, 2024

@github-actions crossbow submit preview-docs

@kou
Copy link
Member

kou commented Mar 6, 2024

Question, are there bots set up to close the associated issue when this PR merges? Normally I would comment with one of Github's recognized keywords (Closes #X) but I'm guessing that's not necessary here.

We'll do it by our merge script: https://github.com/apache/arrow/blob/main/dev/merge_arrow_pr.py

So you don't need to do anything.

Copy link

github-actions bot commented Mar 6, 2024

Revision: 098503b

Submitted crossbow builds: ursacomputing/crossbow @ actions-d7141ab764

Task Status
preview-docs GitHub Actions

@vyasr
Copy link
Contributor Author

vyasr commented Mar 6, 2024

Looks like the Cython 3.0.9 release yesterday broke the Python build. I can open a separate issue.

@vyasr
Copy link
Contributor Author

vyasr commented Mar 6, 2024

Following up on #40386 and #40387

@pitrou
Copy link
Member

pitrou commented Mar 7, 2024

@github-actions crossbow submit preview-docs

Copy link

github-actions bot commented Mar 7, 2024

Revision: d04e32b

Submitted crossbow builds: ursacomputing/crossbow @ actions-f96c10210b

Task Status
preview-docs GitHub Actions

@kou
Copy link
Member

kou commented Mar 7, 2024

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Good addition to the docs, thank you!

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Mar 7, 2024
@pitrou pitrou merged commit 1c9a312 into apache:main Mar 7, 2024
34 of 36 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Mar 7, 2024
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 1c9a312.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@vyasr vyasr deleted the doc/column_factories branch March 8, 2024 04:28
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Mar 8, 2024
It looks like soon after I started investigating scalar conversions for #14121 (but well before I made the PR) a major underlying hole was plugged in pyarrow via apache/arrow#36162. Most of #14121 was created to give us a way to handle scalars from pyarrow generically in libcudf. Now that pyarrow scalars can be easily tossed into arrays, we no longer really need separate scalar functions in libcudf; we can simply create an array from the scalar, put it into a table, and then call the table function.

Additionally, arrow also has a function for creating an array from a scalar. This function is not new but [was previously undocumented](apache/arrow#40373). The builder code added to libcudf in #14121 can be removed and replaced with that factory. The scalar conversion is as simple as calling that arrow function and then using our preexisting `from_arrow` function on the resulting array.

For now this PR is just a simplification of internals. Future PRs will remove the scalar API once we have a more standard path for the conversion of arrays via the C Data Interface.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Yunsong Wang (https://github.com/PointKernel)
  - David Wendt (https://github.com/davidwendt)
  - Bradley Dice (https://github.com/bdice)

URL: #15213
thisisnic pushed a commit to thisisnic/arrow that referenced this pull request Mar 8, 2024
…che#40373)

### Rationale for this change

These factory functions are generally useful and available, so documenting them helps external users find them without having to search the source code.

### What changes are included in this PR?

This PR adds the array factories in arrow/array/util.h into a doxygen group for array factories and adds that group to the Sphinx C++ API documentation.

### Are these changes tested?

I built the docs locally to verify.

### Are there any user-facing changes?

Nothing to the API, only docs.

* GitHub Issue: apache#40318

Authored-by: Vyas Ramasubramani <vyasr@nvidia.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
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