[SPARK-47909][PYTHON][CONNECT] Parent DataFrame class for Spark Connect and Spark Classic #46129
Conversation
This file contains hidden or 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
| def explain( | ||
| self, extended: Optional[Union[bool, str]] = None, mode: Optional[str] = None | ||
| ) -> None: | ||
| if extended is not None and mode is not None: |
There was a problem hiding this comment.
we can move such complex preprocessing to the superclasses later
d70c9f1 to
9f1f1bd
Compare
zhengruifeng
approved these changes
Apr 19, 2024
HyukjinKwon
commented
Apr 19, 2024
06a28df to
692a302
Compare
|
Will fix up the tests soon. |
ueshin
reviewed
Apr 19, 2024
| @@ -325,7 +325,7 @@ def active(cls) -> "SparkSession": | |||
|
|
|||
| active.__doc__ = PySparkSession.active.__doc__ | |||
|
|
|||
| def table(self, tableName: str) -> DataFrame: | |||
| def table(self, tableName: str) -> ParentDataFrame: | |||
There was a problem hiding this comment.
I guess we can leave it as-is? And the following changes?
There was a problem hiding this comment.
this was the way MyPy least complained IIRC. Let me take a look again ..
There was a problem hiding this comment.
Seems like the arguments cannot be more specific type, and return types can't be wider types (https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides). So it complains about the argument.
Let me just keep them all as parent dataframe for simplicity because those types aren't user-facing anyway.
There was a problem hiding this comment.
Here is one example of the error:
python/pyspark/sql/classic/dataframe.py:276: error: Argument 1 of "exceptAll" is incompatible with supertype "DataFrame"; supertype defines the argument type as "DataFrame" [override]
Comment on lines
507
to
509
| @overload | ||
| def repartition(self, numPartitions: int, *cols: "ColumnOrName") -> "ParentDataFrame": | ||
| ... |
There was a problem hiding this comment.
I'm wondering if we need @overload definitions in the subclasses?
There was a problem hiding this comment.
I initially added, and removed it back because MyPy complains too much. I will take another look.
There was a problem hiding this comment.
Seems like by right we should redefine the overloads here (python/mypy#5146, python/mypy#10699). However, we're using pyspark.sql.DataFrame type hints even within our codebase .. so I think it's better to don't have them defined here for now.
6a64c37 to
253acad
Compare
|
Should be ready for a look. All tests passed. I squashed/rebased the commits. |
863bf1e to
f0d4d57
Compare
HyukjinKwon
commented
Apr 21, 2024
| @@ -333,6 +333,11 @@ def test_observe(self): | |||
| from pyspark.sql.connect.observation import Observation | |||
|
|
|||
| class MockDF(DataFrame): | |||
There was a problem hiding this comment.
This might be a breaking change if somebody inherits pyspark.sql.DataFrame before, and it has it's own __init__. However, __init__ is not really an API, and users shouldn't really customize/use/invoke them directly.
f0d4d57 to
d6f897a
Compare
d6f897a to
a94f76b
Compare
|
Merged to master. I will followup if there are more comments to address. |
|
|
HyukjinKwon
added a commit
that referenced
this pull request
Apr 23, 2024
…c` references ### What changes were proposed in this pull request? This PR is a followup of #46129 that moves `pyspark.classic` references to the actual test methods so they are not references during `pyspark-connect` only test (that does not have `pyspark.classic` package). ### Why are the changes needed? To recover the CI: https://github.com/apache/spark/actions/runs/8789489804/job/24119356874 ### Does this PR introduce _any_ user-facing change? No, test-only. ### How was this patch tested? Manually ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46171 from HyukjinKwon/SPARK-47909-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
Apr 23, 2024
… Classic ### What changes were proposed in this pull request? Same as #46129 but for `Column` class. ### Why are the changes needed? Same as #46129 ### Does this PR introduce _any_ user-facing change? Same as #46129 ### How was this patch tested? Manually tested, and CI should verify them. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46155 from HyukjinKwon/SPARK-47933. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
ianmcook
added a commit
to ianmcook/spark
that referenced
this pull request
May 9, 2024
JacobZheng0927
pushed a commit
to JacobZheng0927/spark
that referenced
this pull request
May 11, 2024
…ct and Spark Classic
### What changes were proposed in this pull request?
This PR proposes to have a parent `pyspark.sql.DataFrame` class which `pyspark.sql.connect.dataframe.DataFrame` and `pyspark.sql.classic.dataframe.DataFrame` inherit.
**Note** that for backward compatibility concern, `pyspark.sql.DataFrame(...)` will return still a Spark Classic DataFrame.
Before
1. `pyspark.sql.DataFrame` (Spark Claasic)
- docstrings
- Spark Classic logic
2. `pyspark.sql.connect.dataframe.DataFrame` (Spark Connect)
- Spark Connect logic
3. Users can only see the type hints from `pyspark.sql.DataFrame`.
After
1. `pyspark.sql.DataFrame` (Common)
- docstrings
- Support classmethod usages (dispatch to either Spark Connect or Spark Classic)
2. `pyspark.sql.classic.dataframe.DataFrame` (Spark Classic)
- Spark Classic logic
3. `pyspark.sql.connect.dataframe.DataFrame` (Spark Connect)
- Spark Connect logic
4. Users can only see the type hints from `pyspark.sql.DataFrame`.
### Why are the changes needed?
This fixes two issues in the current structure at Spark Connect:
Support usage of regular methods as class methods, e.g.,
```python
from pyspark.sql import DataFrame
df = spark.range(10)
DataFrame.union(df, df)
```
Before
```
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/.../spark/python/pyspark/sql/dataframe.py", line 4809, in union
return DataFrame(self._jdf.union(other._jdf), self.sparkSession)
^^^^^^^^^
File "/.../spark/python/pyspark/sql/connect/dataframe.py", line 1724, in __getattr__
raise PySparkAttributeError(
pyspark.errors.exceptions.base.PySparkAttributeError: [JVM_ATTRIBUTE_NOT_SUPPORTED] Attribute `_jdf` is not supported in Spark Connect as it depends on the JVM. If you need to use this attribute, do not use Spark Connect when creating your session. Visit https://spark.apache.org/docs/latest/sql-getting-started.html#starting-point-sparksession for creating regular Spark Session in detail.
```
After
```
DataFrame[id: bigint]
```
Supports `isinstance` call
```python
from pyspark.sql import DataFrame
isinstance(spark.range(1), DataFrame)
```
Before
```
False
```
After
```
True
```
### Does this PR introduce _any_ user-facing change?
Yes, as described above.
### How was this patch tested?
Manually tested, and CI should verify them.
### Was this patch authored or co-authored using generative AI tooling?
No.
Closes apache#46129 from HyukjinKwon/SPARK-47909.
Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
JacobZheng0927
pushed a commit
to JacobZheng0927/spark
that referenced
this pull request
May 11, 2024
…c` references ### What changes were proposed in this pull request? This PR is a followup of apache#46129 that moves `pyspark.classic` references to the actual test methods so they are not references during `pyspark-connect` only test (that does not have `pyspark.classic` package). ### Why are the changes needed? To recover the CI: https://github.com/apache/spark/actions/runs/8789489804/job/24119356874 ### Does this PR introduce _any_ user-facing change? No, test-only. ### How was this patch tested? Manually ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46171 from HyukjinKwon/SPARK-47909-followup. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
JacobZheng0927
pushed a commit
to JacobZheng0927/spark
that referenced
this pull request
May 11, 2024
… Classic ### What changes were proposed in this pull request? Same as apache#46129 but for `Column` class. ### Why are the changes needed? Same as apache#46129 ### Does this PR introduce _any_ user-facing change? Same as apache#46129 ### How was this patch tested? Manually tested, and CI should verify them. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#46155 from HyukjinKwon/SPARK-47933. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
HyukjinKwon
pushed a commit
that referenced
this pull request
Jun 3, 2024
…and Spark Classic ### What changes were proposed in this pull request? Parent Window class for Spark Connect and Spark Classic ### Why are the changes needed? Same as #46129 ### Does this PR introduce _any_ user-facing change? Same as #46129 ### How was this patch tested? CI ### Was this patch authored or co-authored using generative AI tooling? NO Closes #46841 from zhengruifeng/py_parent_window. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
riyaverm-db
pushed a commit
to riyaverm-db/spark
that referenced
this pull request
Jun 7, 2024
…and Spark Classic ### What changes were proposed in this pull request? Parent Window class for Spark Connect and Spark Classic ### Why are the changes needed? Same as apache#46129 ### Does this PR introduce _any_ user-facing change? Same as apache#46129 ### How was this patch tested? CI ### Was this patch authored or co-authored using generative AI tooling? NO Closes apache#46841 from zhengruifeng/py_parent_window. Authored-by: Ruifeng Zheng <ruifengz@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 proposes to have a parent
pyspark.sql.DataFrameclass whichpyspark.sql.connect.dataframe.DataFrameandpyspark.sql.classic.dataframe.DataFrameinherit.Note that for backward compatibility concern,
pyspark.sql.DataFrame(...)will return still a Spark Classic DataFrame.Before
pyspark.sql.DataFrame(Spark Claasic)pyspark.sql.connect.dataframe.DataFrame(Spark Connect)Users can only see the type hints from
pyspark.sql.DataFrame.After
pyspark.sql.DataFrame(Common)pyspark.sql.classic.dataframe.DataFrame(Spark Classic)pyspark.sql.connect.dataframe.DataFrame(Spark Connect)Users can only see the type hints from
pyspark.sql.DataFrame.Why are the changes needed?
This fixes two issues in the current structure at Spark Connect:
Support usage of regular methods as class methods, e.g.,
Before
After
Supports
isinstancecallBefore
After
Does this PR introduce any user-facing change?
Yes, as described above.
How was this patch tested?
Manually tested, and CI should verify them.
Was this patch authored or co-authored using generative AI tooling?
No.