[2010YOUY01]

Important

Fork CI setup: For pull requests from a fork to main, enable workflows in your fork's Actions tab before you push.
See the fork CI setup guide.

Which issue does this PR close?

• Closes #.

Rationale for this change

physical-plan/src/aggregates/mod.rs has 4k LoC, excluding tests.

This PR tries to split out optimizer utilities that transforms AggregateExec.

What changes are included in this PR?

• commit 1: move utilities without change
• commit 2: fix existing visibility issue

Commit 2 marks several pub with internal-only, see this doc for the reasons:
https://datafusion.apache.org/contributor-guide/api-health.html#datafusion-internal-public-apis

The optimizer utilities on AggregateExec are standard internal-public APIs, as optimizers have strict invariants to follow to use them correctly, they're not intended to become public API. (some no longer need to be pub for the same reason)

What is the testing strategy for this PR?

existing tests

Are there any user-facing changes?

Yes, see above rationale.

Though, technically those hidden APIs are still accessible by downstream, but this PR adds markers to clarify its purpose.

[github-actions]

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning 350ca76505c9513d16e7dd9910bcd9bde2c911ed
    Building datafusion-physical-plan v55.1.0 (current)
       Built [  46.799s] (current)
     Parsing datafusion-physical-plan v55.1.0 (current)
      Parsed [   0.237s] (current)
    Building datafusion-physical-plan v55.1.0 (baseline)
       Built [  13.921s] (baseline)
     Parsing datafusion-physical-plan v55.1.0 (baseline)
      Parsed [   0.236s] (baseline)
    Checking datafusion-physical-plan v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.737s] 229 checks: 227 pass, 2 fail, 0 warn, 31 skip

--- failure function_now_doc_hidden: pub function is now #[doc(hidden)] ---

Description:
A pub function is now #[doc(hidden)], removing it from the crate's public API.
        ref: https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#hidden
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/function_now_doc_hidden.ron

Failed in:
  function topk_types_supported in file /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/aggregates/optimizations.rs:613

--- failure inherent_method_now_doc_hidden: inherent method #[doc(hidden)] added ---

Description:
A method or associated fn is now #[doc(hidden)], removing it from the crate's public API.
        ref: https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#hidden
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/inherent_method_now_doc_hidden.ron

Failed in:
  AggregateExec::input_keeps_groups_local in file /home/runner/work/datafusion/datafusion/target/semver-checks-shared/semver-checks/git-350ca76505c9513d16e7dd9910bcd9bde2c911ed/377557479a9f94058587143b5b13e00c1207d5dd/datafusion/physical-plan/src/aggregates/mod.rs:1776
  AggregateExec::get_minmax_desc in file /home/runner/work/datafusion/datafusion/target/semver-checks-shared/semver-checks/git-350ca76505c9513d16e7dd9910bcd9bde2c911ed/377557479a9f94058587143b5b13e00c1207d5dd/datafusion/physical-plan/src/aggregates/mod.rs:1929
  AggregateExec::is_unordered_unfiltered_group_by_distinct in file /home/runner/work/datafusion/datafusion/target/semver-checks-shared/semver-checks/git-350ca76505c9513d16e7dd9910bcd9bde2c911ed/377557479a9f94058587143b5b13e00c1207d5dd/datafusion/physical-plan/src/aggregates/mod.rs:1938

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  63.917s] datafusion-physical-plan

[kumarUjjawal]

These three helpers (get_minmax_desc, is_unordered_unfiltered_group_by_distinct, and topk_types_supported) were public in 55.1.0. Making them private breaks downstream callers, should we use #[doc(hidden)].

Could we retain deprecated public wrappers, or document the removals and migration path in the 56.0.0 upgrade guide?

[andygrove]

+1 for using #[doc(hidden)] rather than changing visibility in a minor release.

[2010YOUY01]

Thanks for the review, addressed. (use #[doc(hidden)] instead of removing pub)

Those 3 APIs are special cases. I think a more accurate description is "made public by mistake": they were made public for internal optimizer use, but they don't actually need to be public.

However, it's possible (though unlikely) that downstream users already depend on them, so I agree we should mark them as #[doc(hidden)] first, and try to make them private after another release.

[kumarUjjawal]

Looks good 👍