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-45279: [C++][Compute] Move all Grouper tests to grouper_test.cc #45280

Merged
merged 4 commits into from
Jan 16, 2025

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Jan 16, 2025

What changes are included in this PR?

  1. Move tests for the Grouper and RowSegmenter implementations to arrow/compute/row/grouper_test.cc where they belong (this also makes acero/hash_aggregate_test.cc slightly shorter)
  2. Introduce a arrow_compute_testing object library to hold compute-related testing utilities
  3. Remove duplicate definitions of ExecBatchFromJSON from Dataset and Acero
  4. Make arrow/compute/kernels/test_util.h internal

Are these changes tested?

Yes, by existing tests and CI.

Are there any user-facing changes?

No.

Copy link

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

@github-actions github-actions bot added awaiting review Awaiting review Component: C++ and removed awaiting review Awaiting review labels Jan 16, 2025
@pitrou pitrou requested a review from zanmato1984 January 16, 2025 09:54
@zanmato1984
Copy link
Contributor

Wow, really nice refinement! Many things in it are what I have always wanted. Cheers!

Is it ready for review now or do I have to wait until it is no longer in draft?

@pitrou
Copy link
Member Author

pitrou commented Jan 16, 2025

@zanmato1984 I was just waiting to fix CI, but yes, it's ready for review now.

@pitrou pitrou marked this pull request as ready for review January 16, 2025 10:11
@pitrou pitrou requested a review from westonpace as a code owner January 16, 2025 10:11
@github-actions github-actions bot added the awaiting review Awaiting review label Jan 16, 2025
@pitrou
Copy link
Member Author

pitrou commented Jan 16, 2025

(of course, most of the changes here are just code being moved around)

@pitrou
Copy link
Member Author

pitrou commented Jan 16, 2025

I would ideally like to merge this soon as I've also started to work on #45269

Copy link
Contributor

@zanmato1984 zanmato1984 left a comment

Choose a reason for hiding this comment

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

LGTM with several nits.

cpp/src/arrow/compute/kernels/scalar_compare_benchmark.cc Outdated Show resolved Hide resolved
cpp/src/arrow/compute/test_util_internal.cc Outdated Show resolved Hide resolved
cpp/src/arrow/compute/test_util_internal.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting committer review Awaiting committer review awaiting review Awaiting review and removed awaiting review Awaiting review awaiting committer review Awaiting committer review labels Jan 16, 2025
@pitrou pitrou merged commit df82d4c into apache:main Jan 16, 2025
37 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Jan 16, 2025
@pitrou pitrou deleted the grouper_test branch January 16, 2025 12:01
Copy link

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

There were no benchmark performance regressions. 🎉

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

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.

2 participants