GH-37751: [C++][Gandiva] Avoid registering exported functions multiple times in gandiva by niyue · Pull Request #37752 · apache/arrow · GitHub

niyue · 2023-09-16T09:38:15Z

Rationale for this change

Try to fix #37751

What changes are included in this PR?

The exported functions defined in gandiva are registered multiple times unnecessarily. This PR tries to address this issue.

Are these changes tested?

Yes.

Are there any user-facing changes?

No

Closes: [C++][Gandiva] Exported functions are registered multiple times in gandiva #37751

github-actions · 2023-09-16T09:38:43Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

niyue · 2023-09-16T09:41:13Z

All the REGISTER_EXPORTED_FUNCS are moved into a exported_funcs.cc file, which makes the registration API to be called from 7 * 6 times down to 6 times.

js8544

Some minor nits. Can you also add a unit test to make sure each function is only registered once?

js8544 · 2023-09-17T06:48:12Z

Can you also fix the lint issue in CI?

niyue · 2023-09-17T13:19:23Z

Can you also fix the lint issue in CI?

Sure. I fixed the lint issue in CI in the new commit.

niyue · 2023-09-17T13:22:57Z

Can you also add a unit test to make sure each function is only registered once?

@js8544 I tried to do that, but currently the registered() method of ExportedFuncsRegistry is a private method, and it is not possible to retrieve the list of currently registered functions from the registry externally, so it is not possible to add such a unit test unless we make the registered() method public. Do you have any recommendation for this? Thanks.

UPDATE:
I used the FRIEND_TEST macro to test this private method now

niyue · 2023-09-22T00:30:55Z

@js8544 this PR is ready for review. There is still one check failing, but it seemed irrelevant with my change (may be caused by CI network issue as far as I can tell). Any comment is appreciated. Thanks.

…ly happens once.

niyue · 2023-10-05T07:13:02Z

@js8544 I've resolved the remaining issues and rebased to the latest commit, could you please take a look again? Thanks.

pitrou · 2023-10-11T08:29:25Z

@github-actions crossbow submit -g cpp

js8544

+1, just a simple nit.

github-actions · 2023-10-11T08:32:21Z

Revision: 3925cad1c3d84ebab04819f87447754da66cff79

Submitted crossbow builds: ursacomputing/crossbow @ actions-0aa03adfc8

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-35-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

pitrou · 2023-10-11T09:07:29Z

@github-actions crossbow submit -g python

github-actions · 2023-10-11T09:10:53Z

Revision: 9e7ebb5bbb955b8cec1ca0355f7c1fe5411cda09

Submitted crossbow builds: ursacomputing/crossbow @ actions-d691ca469b

Task	Status
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.4.1
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.4.1
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-python
test-debian-11-python-3
test-fedora-35-python-3
test-ubuntu-20.04-python-3
test-ubuntu-22.04-python-3

… of testing and use pointers as returned value for consistent with coding convention.

js8544 · 2023-10-16T02:48:41Z

Thanks! LGTM now. Will need @pitrou's approval to merge.

conbench-apache-arrow · 2023-10-17T19:07:04Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 47314e8.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…ultiple times in gandiva (apache#37752) ### Rationale for this change Try to fix apache#37751 ### What changes are included in this PR? The exported functions defined in gandiva are registered multiple times unnecessarily. This PR tries to address this issue. ### Are these changes tested? Yes. ### Are there any user-facing changes? No Authored-by: Yue Ni <niyue.com@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

github-actions Bot added Component: C++ Component: Gandiva awaiting review Awaiting review labels Sep 16, 2023

niyue commented Sep 16, 2023

View reviewed changes

Comment thread cpp/src/gandiva/engine.cc Outdated

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 16, 2023

niyue commented Sep 16, 2023

View reviewed changes

niyue changed the title ~~Avoid registering exported functions multiple times in gandiva.~~ [GH-37751] [C++] [Gandiva] Avoid registering exported functions multiple times in gandiva Sep 16, 2023

niyue force-pushed the bugfix/avoid-dup-registration branch 5 times, most recently from bc2c673 to 5eaf0f8 Compare September 17, 2023 04:21

js8544 reviewed Sep 17, 2023

View reviewed changes

Comment thread cpp/src/gandiva/exported_funcs_registry.h Outdated

Comment thread cpp/src/gandiva/engine.cc Outdated

niyue force-pushed the bugfix/avoid-dup-registration branch 2 times, most recently from 9ebb6b4 to 92fbf55 Compare September 17, 2023 13:14

niyue commented Sep 20, 2023

View reviewed changes

Comment thread cpp/src/gandiva/exported_funcs_registry_test.cc Outdated

niyue force-pushed the bugfix/avoid-dup-registration branch 3 times, most recently from 5482a47 to af90fa2 Compare September 21, 2023 13:41

kou changed the title ~~[GH-37751] [C++] [Gandiva] Avoid registering exported functions multiple times in gandiva~~ [GH-37751] [C++][Gandiva] Avoid registering exported functions multiple times in gandiva Sep 23, 2023

niyue added 2 commits October 5, 2023 13:14

Avoid registering exported functions multiple times in gandiva.

470f86d

Add minimum test case for verifying exported function registration on…

3cf1766

…ly happens once.

niyue force-pushed the bugfix/avoid-dup-registration branch from af90fa2 to 3cf1766 Compare October 5, 2023 05:19

pitrou reviewed Oct 9, 2023

View reviewed changes

Comment thread cpp/src/gandiva/exported_funcs_registry.h Outdated

apache deleted a comment from github-actions Bot Oct 11, 2023

js8544 approved these changes Oct 11, 2023

View reviewed changes

Comment thread cpp/src/gandiva/exported_funcs_registry_test.cc Outdated

js8544 suggested changes Oct 12, 2023

View reviewed changes

Comment thread cpp/src/gandiva/exported_funcs_registry.h Outdated

Make the registered function of ExportedFuncsRegistry public for ease…

49ea793

… of testing and use pointers as returned value for consistent with coding convention.

niyue force-pushed the bugfix/avoid-dup-registration branch from 9e7ebb5 to 49ea793 Compare October 16, 2023 01:21

pitrou approved these changes Oct 16, 2023

View reviewed changes

pitrou changed the title ~~[GH-37751] [C++][Gandiva] Avoid registering exported functions multiple times in gandiva~~ GH-37751: [C++][Gandiva] Avoid registering exported functions multiple times in gandiva Oct 16, 2023

pitrou merged commit 47314e8 into apache:main Oct 16, 2023

pitrou removed the awaiting committer review Awaiting committer review label Oct 16, 2023

pitrou mentioned this pull request Oct 16, 2023

[C++][Gandiva] Exported functions are registered multiple times in gandiva #37751

Closed

github-actions Bot added the awaiting committer review Awaiting committer review label Oct 16, 2023

Conversation

niyue commented Sep 16, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Sep 16, 2023

Uh oh!

Uh oh!

niyue Sep 16, 2023

Choose a reason for hiding this comment

Uh oh!

js8544 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

js8544 commented Sep 17, 2023

Uh oh!

niyue commented Sep 17, 2023

Uh oh!

niyue commented Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

niyue commented Sep 22, 2023

Uh oh!

niyue commented Oct 5, 2023

Uh oh!

Uh oh!

pitrou commented Oct 11, 2023

Uh oh!

js8544 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Oct 11, 2023

Uh oh!

pitrou commented Oct 11, 2023

Uh oh!

github-actions Bot commented Oct 11, 2023

Uh oh!

Uh oh!

js8544 commented Oct 16, 2023

Uh oh!

conbench-apache-arrow Bot commented Oct 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

niyue commented Sep 16, 2023 •

edited by github-actions Bot

Loading

niyue commented Sep 17, 2023 •

edited

Loading