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
[SPARK-40816][CONNECT][PYTHON] Rename LogicalPlan.collect to LogicalPlan.to_proto #38279
Conversation
should it be a private method? |
If this class isn't going to be documented as an API, it's fine. |
This class is not API class so sounds like it's ok. only session, dataframe, client, readwriter (possibly catalog) will be API class I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, that is fine
Can one of the admins verify this patch? |
also happening on this PR
|
@@ -80,9 +80,19 @@ def _verify(self, session: "RemoteSparkSession") -> bool: | |||
|
|||
return test_plan == plan | |||
|
|||
def collect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's problably fix the error here together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors should have been fixed.
### What changes were proposed in this pull request? The mypy check in master seems to be broken: <img width="676" alt="Screen Shot 2022-10-17 at 1 09 13 PM" src="https://user-images.githubusercontent.com/1938382/196273759-64372862-6caa-4a9e-b700-b665c7ff7e6c.png"> (see it on #38279 and #38275, also reproducible locally). This PR propose to remove the relevant `type: ignore` comments. ### Why are the changes needed? Fix python lint check. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes #38287 from amaliujia/test_python_lint. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
10d61a1
to
5ce1d17
Compare
Merged to master. |
### What changes were proposed in this pull request? The mypy check in master seems to be broken: <img width="676" alt="Screen Shot 2022-10-17 at 1 09 13 PM" src="https://user-images.githubusercontent.com/1938382/196273759-64372862-6caa-4a9e-b700-b665c7ff7e6c.png"> (see it on apache#38279 and apache#38275, also reproducible locally). This PR propose to remove the relevant `type: ignore` comments. ### Why are the changes needed? Fix python lint check. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes apache#38287 from amaliujia/test_python_lint. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
…lan.to_proto ### What changes were proposed in this pull request? The `collect` method in `class LogicalPlan` is really to generate connect proto plan. It's confusing to use `collect` which overlaps with `collect` in dataframe API that returns materialized data. This PR proposes to rename this method to `to_proto` to match its implementation. ### Why are the changes needed? Improve codebase readability. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes apache#38279 from amaliujia/rename_logical_plan_collect2. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
What changes were proposed in this pull request?
The
collect
method inclass LogicalPlan
is really to generate connect proto plan. It's confusing to usecollect
which overlaps withcollect
in dataframe API that returns materialized data.This PR proposes to rename this method to
to_proto
to match its implementation.Why are the changes needed?
Improve codebase readability.
Does this PR introduce any user-facing change?
No
How was this patch tested?
UT