GH-42126: [C++] Move TakeXXX free functions into TakeMetaFunction and make them private

[felipecrv]

Rationale for this change

Move TakeXXX free functions into TakeMetaFunction and make them private

What changes are included in this PR?

Code move and some small refactorings in preparation for #41700.

Are these changes tested?

By existing tests.

• GitHub Issue: [C++] Move TakeXXX free functions into TakeMetaFunction and make them private #42126

[github-actions]

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

[felipecrv]

@pitrou I took some commits from #41700 to here to reduce the code churn in there.

[pitrou]

I'm assuming this is only moving code around? In which case LGTM (but a question below).

[pitrou]

I'm curious: is there a particular reason for taking the first arg as shared_ptr-const-ref, and the second only as value-const-ref?

[felipecrv]

Driven by the signatures of the Datum constructors [1]. Originally, they were almost all by simple &, then I tried making all of them shared_ptr& but that got overwhelming, so I settled on this mixed combination driven by the need (Array holds shared_ptr<ArrayData> which is what Datum cares about).

[1]

  /// \brief Construct from a Scalar
  Datum(std::shared_ptr<Scalar> value)  // NOLINT implicit conversion
      : value(std::move(value)) {}

  /// \brief Construct from an ArrayData
  Datum(std::shared_ptr<ArrayData> value)  // NOLINT implicit conversion
      : value(std::move(value)) {}

  /// \brief Construct from an ArrayData
  Datum(ArrayData arg)  // NOLINT implicit conversion
      : value(std::make_shared<ArrayData>(std::move(arg))) {}

  /// \brief Construct from an Array
  Datum(const Array& value);  // NOLINT implicit conversion

  /// \brief Construct from an Array
  Datum(const std::shared_ptr<Array>& value);  // NOLINT implicit conversion

  /// \brief Construct from a ChunkedArray
  Datum(std::shared_ptr<ChunkedArray> value);  // NOLINT implicit conversion

  /// \brief Construct from a RecordBatch
  Datum(std::shared_ptr<RecordBatch> value);  // NOLINT implicit conversion

  /// \brief Construct from a Table
  Datum(std::shared_ptr<Table> value);  // NOLINT implicit conversion

  /// \brief Construct from a ChunkedArray.
  ///
  /// This can be expensive, prefer the shared_ptr<ChunkedArray> constructor
  explicit Datum(const ChunkedArray& value);

  /// \brief Construct from a RecordBatch.
  ///
  /// This can be expensive, prefer the shared_ptr<RecordBatch> constructor
  explicit Datum(const RecordBatch& value);

  /// \brief Construct from a Table.
  ///
  /// This can be expensive, prefer the shared_ptr<Table> constructor
  explicit Datum(const Table& value);

[felipecrv]

I want to simplify these signatures further in the next PR as less will actually have to be handled by the "take" MetaFunction as "array_take" learns to handle chunked arrays by itself.

[felipecrv]

I'm assuming this is only moving code around? In which case LGTM (but a question below).

I have the code moves separated from the refactorings in the commits.

[pitrou]

I'm away next week, feel free to merge without waiting for me or @bkietz .

[felipecrv]

I'm away next week, feel free to merge without waiting for me or @bkietz .

I will merge today and any concern can be addressed in #41700.

[conbench-apache-arrow]

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

There were 3 benchmark results indicating a performance regression:

• Commit Run on test-mac-arm at 2024-06-14 08:15:21Z
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-01, scale_factor=1
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-03, scale_factor=1
• and 1 more (see the report linked below)

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