[SPARK-41717][CONNECT] Deduplicate print and _repr_html_ at LogicalPlan #39223
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HyukjinKwon
changed the title
HyukjinKwon
force-pushed
the
SPARK-41717
branch
from
195b718
to
5cf8b0c
Compare
HyukjinKwon
force-pushed
the
SPARK-41717
branch
from
5cf8b0c
to
ae2e544
Compare
|
cc @amaliujia @zhengruifeng @grundprinzip FYI |
HyukjinKwon
force-pushed
the
SPARK-41717
branch
from
ae2e544
to
bc75693
Compare
HyukjinKwon
changed the title
zhengruifeng
approved these changes
Dec 27, 2022
|
Related tests passed. Merged to master. |
| @@ -95,14 +96,68 @@ def to_proto(self, session: "SparkConnectClient", debug: bool = False) -> proto. | |||
|
|
|||
| return plan | |||
|
|
|||
| # TODO(SPARK-41717): Implement the command logic for print and _repr_html_ | |||
| def _parameters_to_print(self, parameters: Mapping[str, Any]) -> Mapping[str, Any]: | |||
| params = {} | |||
There was a problem hiding this comment.
The logic of this method is complicated enough to deserve a docstring.
There was a problem hiding this comment.
Sure, let me add some docs.
| @@ -207,7 +240,7 @@ def print(self, indent: int = 0) -> str: | |||
| def _repr_html_(self) -> str: | |||
There was a problem hiding this comment.
Why is this method not removed?
There was a problem hiding this comment.
This wasn't removed because LocalRelation contains data that's PyArrow table, and this isn't printed out pretty together with our format out of the box. So it was manually overridden here.
| @@ -265,13 +277,13 @@ class Project(LogicalPlan): | |||
|
|
|||
| def __init__(self, child: Optional["LogicalPlan"], *columns: "ColumnOrName") -> None: | |||
| super().__init__(child) | |||
| self._raw_columns = list(columns) | |||
There was a problem hiding this comment.
Because this one was only the exception that does not follow the variable naming style in this file. All are either name or _name, and the automatic printing logic relies on this naming.
|
Thanks for doing it. Please prepare a follow-up PR that adds documentation to the parameter generating function because that is not "trivial" and thus deserves more explanation. |
HyukjinKwon
added a commit
that referenced
this pull request
Dec 27, 2022
…s_to_print, print and _repr_html_ at LogicalPlan ### What changes were proposed in this pull request? This PR is a followup of #39223 that addresses #39223 (comment), that adds some docstring for the fixed methods. ### Why are the changes needed? For better readability. ### Does this PR introduce _any_ user-facing change? No, these aren't API. It adds some doc for developers. ### How was this patch tested? Linter should verify them. Closes #39237 from HyukjinKwon/SPARK-41717-followup. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
HyukjinKwon
added a commit
that referenced
this pull request
Nov 24, 2023
… Pandas Functions API with Spark Connect ### What changes were proposed in this pull request? This PR proposes to rename `_func` to `_functions` in the protobuf instances for Pandas Functions API with Spark Connect so the string presentation includes them (see also #39223). ### Why are the changes needed? In order to have the pretty string format for protobuf messages in Python side. ### Does this PR introduce _any_ user-facing change? Yes, ```bash ./bin/pyspark --remote local ``` ```python df = spark.range(1) print(df.mapInPandas(lambda x: x, df.schema)._plan.print()) ``` **Before:** ``` <MapPartitions is_barrier='False'> <Range start='0', end='1', step='1', num_partitions='None'> ``` **After:** ``` <MapPartitions function='<lambda>(id)', is_barrier='False'> <Range start='0', end='1', step='1', num_partitions='None'> ``` ### How was this patch tested? Manually tested as above. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #43991 from HyukjinKwon/fix-print-proto. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR deduplicates
printand_repr_html_logic by having a comment logic to handle them by looking up the signature and type hints (similar with what we're doing at Spark SQL's query string, e.g.,TreeNode.stringArgs).Why are the changes needed?
To make it easier to maintain.
Does this PR introduce any user-facing change?
Virtually no.
How was this patch tested?
I tested almost all cases, and complex cases too manually. In addition, I added a unittest too.