docs: Document the convention for internal public APIs

[2010YOUY01]

Which issue does this PR close?

• Closes #.

Rationale for this change

The purpose should be clear from the PR diff itself.

It is an existing convention, this PR document it in API policy.

There are other ~5 places have this pattern, I plan to make a follow-up PR to update their comments.

What changes are included in this PR?

What is the testing strategy for this PR?

Are there any user-facing changes?

[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 apache/main
    Building datafusion-physical-plan v55.1.0 (current)
       Built [  40.523s] (current)
     Parsing datafusion-physical-plan v55.1.0 (current)
      Parsed [   0.169s] (current)
    Building datafusion-physical-plan v55.1.0 (baseline)
       Built [  39.692s] (baseline)
     Parsing datafusion-physical-plan v55.1.0 (baseline)
      Parsed [   0.179s] (baseline)
    Checking datafusion-physical-plan v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.690s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- 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.50.0/src/lints/inherent_method_now_doc_hidden.ron

Failed in:
  HashTableLookupExpr::new in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/fb40c84bec637928a1762fae0ca632ba01c2913e/datafusion/physical-plan/src/joins/hash_join/partitioned_hash_eval.rs:299

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

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.42%. Comparing base (a522cd5) to head (5c08605).
⚠️ Report is 35 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25495      +/-   ##
==========================================
+ Coverage   82.37%   82.42%   +0.04%     
==========================================
  Files        1138     1139       +1     
  Lines      433506   435372    +1866     
  Branches   433506   435372    +1866     
==========================================
+ Hits       357102   358836    +1734     
+ Misses      54850    54832      -18     
- Partials    21554    21704     +150     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[adriangb]

Thank you. I agree documenting an existing convention is a good step.

@alamb do you see any issue with this?

[alamb]

Thanks @2010YOUY01 and @adriangb -- this makes sense to me

I would personally suggest we try and use a feature for this method rather than not documenting certain methods, but I think this is already an improvement in my mind

[alamb]

I recommend we also document "why" we have such APIs when so much of DataFusion's flexibility is predicated on extensible APIs.

Maybe the idea is that these APIs are really meant as internal implementation details, but for some logistical reason they have to be in different crates. Thus the "internal" APIs aren't intended as extension points, and may change / etc without warning?

[alamb]

I think in general multiple types of APIs just makes DataFUsion more complicated and I would like to suggest we minimize the number of such APIs as much as possible

For example, for test helpers, we could potentially use a feature (test_helpers for example) to mark that they are not part of the public API rather than marking them as a doc

[alamb]

would recommend putting this detail in the intro

[2010YOUY01]

Thank you for the review @adriangb and @alamb

Thanks @2010YOUY01 and @adriangb -- this makes sense to me

I would personally suggest we try and use a feature for this method rather than not documenting certain methods, but I think this is already an improvement in my mind

I agree using features for test utils is better, and help us exclude one special case

parent-crate (dev-dependencies includes `test_util` feature)
child-crate: with `test_util` feature for public testing APIs

However this might not work well for internal optimizer apis, if a user depends on both parent and child crate, the hidden feature can get implicitly enabled.

To clean up the remaining internal public APIs, I plan to

• Hide test utilities with a new feature
• Use the existing approach for others

[rluvaton]

Nit: Please add another example - benchmarks, which was the original reason why GroupColumn was exposed and now people use it which complicate changing its api