GH-38920: [C++][Gandiva] Refactor function holder to return arrow Result

[niyue]

Rationale for this change

• This PR tries to make Gandiva FunctionHolder classes to return arrow::Result instead of using output parameters, and this tries to address the follow up task mentioned in GH-38589: [C++][Gandiva] Support registering external C functions #38632 (comment) and makes the code slightly simpler

What changes are included in this PR?

• A refactoring task to return arrow::Result in Gandiva FunctionHolder classes

Are these changes tested?

It should be covered by existing unit tests.

Are there any user-facing changes?

No

• Closes: [C++][Gandiva] Refactor function holder to return arrow Result #38920

[niyue]

@kou I submit this PR to address the follow up task mentioned in #38632 (comment).

But after I finish most of the refactoring, I realize the template function may still be needed since the function holder map requires returning a parent class shared_ptr (FunctionHolderPtr) but each FunctionHolder sub class's Make returns a concrete class shared_ptr.

But this PR at least reduces 130 lines of code, so I think it may still be useful and submit it here.

[kou]

Thanks. Could you open an issue for this?
This is not a MINOR task now. :-)
Our MINOR definition: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#minor-fixes

[github-actions]

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

[niyue]

Could you open an issue for this?

Sure. I opened a bug #38879, which described the to_date_utf8_utf8_int32 invalid input bug I fixed in this PR. And the refactoring task is described in this PR description. Let me know if I need to create a separated issue for that.

[kou]

I opened a bug #38879, which described the to_date_utf8_utf8_int32 invalid input bug I fixed in this PR. And the refactoring task is described in this PR description. Let me know if I need to create a separated issue for that.

Thanks.
Could you open one more issue for refactoring?
This has many refactoring related changes. If we mix the bug fix change and refactoring related changes in one PR, it's difficult to find the bug fix related change.

[niyue]

Could you open one more issue for refactoring?

Okay. Do you think if I need to submit a separated PR, this one for bug fixing and the other one for refactoring?

Update:
I now separated this PR into two PRs, and the corresponding issues are separated as well

• GH-38920: [C++][Gandiva] Refactor function holder to return arrow Result #38873 (this one) for refactoring task
  • issue: [C++][Gandiva] Refactor function holder to return arrow Result #38920
• GH-38879: [C++][Gandiva] Fix Gandiva to_date function's validation for supress errors parameter niyue/arrow#3 for bug fixing
  • this is a stacked PR (because the newly added test cases use the new refactored API), and I will re-target this bug fixing PR to arrow repo once this refactoring PR is merged
  • issue: [C++][Gandiva] to_date_utf8_utf8_int32 function's suppress_errors parameter causes segfault for non literal input #38879

I made this PR to be the refactoring task PR since most of the review comments are for refactoring task. The PR title/description is updated as well.

[github-actions]

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

[pitrou]

This is a welcome improvement.

[pitrou]

cc @js8544

[pitrou]

Ci failures look unrelated, I'll merge.

Thanks a lot for doing this @niyue !

[conbench-apache-arrow]

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

There were 4 benchmark results indicating a performance regression:

• Commit Run on ursa-i9-9960x at 2023-11-30 06:38:39Z
  • file-read (R) with compression=uncompressed, dataset=fanniemae_2016Q4, file_type=feather, language=R, output_type=dataframe
  • file-read (R) with compression=lz4, dataset=fanniemae_2016Q4, file_type=feather, language=R, output_type=table
• and 2 more (see the report linked below)

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.