Repository navigation
chore: Hide internal public utility APIs - #25963
Conversation
|
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 |
| /// This is not a public API and is for internal use only; see [API policy] for details. | ||
| /// | ||
| /// [API policy]: https://datafusion.apache.org/contributor-guide/api-health.html#internal-public-apis | ||
| #[doc(hidden)] |
There was a problem hiding this comment.
This API seem like reasonable to be public, however now it requires cooperation from several other optimizer rules, to make it safe. The comments right above can demonstrate it is tricky to use.
Note downstreams can still have access to it, this marker is only a hint for (a) might change often (b) tricky to use it correctly, as explained in the link
There was a problem hiding this comment.
This one in particular I would like to maybe add a comment about "might change" or "is tricky to use" if posisble to make it easier to understand the internal marking
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25963 +/- ##
==========================================
+ Coverage 82.59% 82.77% +0.18%
==========================================
Files 1145 1148 +3
Lines 443934 451075 +7141
Branches 443934 451075 +7141
==========================================
+ Hits 366669 373385 +6716
+ Misses 55009 54937 -72
- Partials 22256 22753 +497 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// - the `predicate` expression is not aliased | ||
| /// - the `predicate` expression contains no window function calls | ||
| /// | ||
| /// # Public Only for Internal Use: |
There was a problem hiding this comment.
in fact the comment above says "for internal use in Datafusion only"
| /// This is not a public API and is for internal use only; see [API policy] for details. | ||
| /// | ||
| /// [API policy]: https://datafusion.apache.org/contributor-guide/api-health.html#internal-public-apis | ||
| #[doc(hidden)] |
There was a problem hiding this comment.
This one in particular I would like to maybe add a comment about "might change" or "is tricky to use" if posisble to make it easier to understand the internal marking
|
@alamb Thank you for the review! Feedbacks addressed. |
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes #. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> See the previous PR for rationale - apache#25495 This PR adds #[doc(hidden)] and comments for public APIs that is used as internal utility. The major categories are - already includes `#doc(hidden)` - Already explained in the comment that this public API is internal-only - Optimizer utilities, with strict preconditions to use it correctly. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> comments only ## What is the testing strategy for this PR? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. -->
Which issue does this PR close?
Rationale for this change
See the previous PR for rationale
This PR adds #[doc(hidden)] and comments for public APIs that is used as internal utility.
The major categories are
#doc(hidden)What changes are included in this PR?
comments only
What is the testing strategy for this PR?
Are there any user-facing changes?