You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhengruifeng (via GitHub)" <gi...@apache.org> on 2023/08/22 11:26:23 UTC
[GitHub] [spark] zhengruifeng opened a new pull request, #42608: [SPARK-42017][PYTHON][CONNECT][TESTS] Enable `ColumnParityTests. test_access_column `
zhengruifeng opened a new pull request, #42608:
URL: https://github.com/apache/spark/pull/42608
### What changes were proposed in this pull request?
Enable `ColumnParityTests. test_access_column`, this issue described in https://issues.apache.org/jira/browse/SPARK-42017 was addressed in https://github.com/apache/spark/commit/91e97f92fe76f9718cd16af0c761d5530bdb37ee
### Why are the changes needed?
for test coverage
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
enabled UT
### Was this patch authored or co-authored using generative AI tooling?
NO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #42608: [SPARK-42017][PYTHON][CONNECT][TESTS] Enable `ColumnParityTests.test_access_column`
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #42608:
URL: https://github.com/apache/spark/pull/42608#discussion_r1302368571
##########
python/pyspark/sql/connect/dataframe.py:
##########
@@ -1623,6 +1624,10 @@ def __getitem__(self, item: Union[int, str, Column, List, Tuple]) -> Union[Colum
alias = self._get_alias()
if self._plan is None:
raise SparkConnectException("Cannot analyze on empty plan.")
+
+ if "." not in item and "*" not in item and "`" not in item and item not in self.columns:
Review Comment:
similar to https://github.com/apache/spark/commit/91e97f92fe76f9718cd16af0c761d5530bdb37ee, but need to cases like `*`, `a.*`, ``aa.bb``
##########
python/pyspark/sql/connect/dataframe.py:
##########
@@ -1623,6 +1624,10 @@ def __getitem__(self, item: Union[int, str, Column, List, Tuple]) -> Union[Colum
alias = self._get_alias()
if self._plan is None:
raise SparkConnectException("Cannot analyze on empty plan.")
+
+ if "." not in item and "*" not in item and "`" not in item and item not in self.columns:
Review Comment:
similar to https://github.com/apache/spark/commit/91e97f92fe76f9718cd16af0c761d5530bdb37ee, but need to skip cases like `*`, `a.*`, ``aa.bb``
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on pull request #42608: [SPARK-42017][PYTHON][CONNECT] `df['col_name']` should validate the column name
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #42608:
URL: https://github.com/apache/spark/pull/42608#issuecomment-1690824764
merged to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #42608: [SPARK-42017][PYTHON][CONNECT][TESTS] Make `df['col_name']` validate the column name
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #42608:
URL: https://github.com/apache/spark/pull/42608#discussion_r1302605786
##########
python/pyspark/testing/connectutils.py:
##########
@@ -76,6 +76,7 @@ class MockRemoteSession:
def __init__(self):
self.hooks = {}
self.session_id = str(uuid.uuid4())
+ self.is_stopped = True
Review Comment:
I tried `hasattr`, but `hasattr(self._session, '_client')` won't work, I guess that is due to `MockRemoteSession` has its own `__getattr__` method
but let me check `if not hasattr(self._session, "is_mocksession")`, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #42608: [SPARK-42017][PYTHON][CONNECT][TESTS] Make `df['col_name']` validate the column name
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #42608:
URL: https://github.com/apache/spark/pull/42608#discussion_r1302600900
##########
python/pyspark/testing/connectutils.py:
##########
@@ -76,6 +76,7 @@ class MockRemoteSession:
def __init__(self):
self.hooks = {}
self.session_id = str(uuid.uuid4())
+ self.is_stopped = True
Review Comment:
this change is needed to make `test_connect_plan` with `MockRemoteSession` pass, otherwise
https://github.com/zhengruifeng/spark/actions/runs/5946008704/job/16125973924
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #42608: [SPARK-42017][PYTHON][CONNECT][TESTS] Make `df['col_name']` validate the column name
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #42608:
URL: https://github.com/apache/spark/pull/42608#discussion_r1302604529
##########
python/pyspark/testing/connectutils.py:
##########
@@ -76,6 +76,7 @@ class MockRemoteSession:
def __init__(self):
self.hooks = {}
self.session_id = str(uuid.uuid4())
+ self.is_stopped = True
Review Comment:
I think you can name it `self.is_mocksession`, and `if not hasattr(self._session, "is_mocksession"):` with commenting that it indicates `MockRemoteSession`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #42608: [SPARK-42017][PYTHON][CONNECT][TESTS] Make `df['col_name']` validate the column name
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #42608:
URL: https://github.com/apache/spark/pull/42608#discussion_r1302606225
##########
python/pyspark/testing/connectutils.py:
##########
@@ -76,6 +76,7 @@ class MockRemoteSession:
def __init__(self):
self.hooks = {}
self.session_id = str(uuid.uuid4())
+ self.is_stopped = True
Review Comment:
let me switch to `is_mocksession`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #42608: [SPARK-42017][PYTHON][CONNECT][TESTS] Enable `ColumnParityTests.test_access_column`
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #42608:
URL: https://github.com/apache/spark/pull/42608#discussion_r1302449910
##########
python/pyspark/sql/connect/dataframe.py:
##########
@@ -1623,6 +1624,10 @@ def __getitem__(self, item: Union[int, str, Column, List, Tuple]) -> Union[Colum
alias = self._get_alias()
if self._plan is None:
raise SparkConnectException("Cannot analyze on empty plan.")
+
+ if "." not in item and "*" not in item and "`" not in item and item not in self.columns:
Review Comment:
Should we maybe just:
```python
col = _to_col_with_plan_id(
col=alias if alias is not None else item,
plan_id=self._plan._plan_id,
)
self.select(col)
return col
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #42608: [SPARK-42017][PYTHON][CONNECT][TESTS] Make `df['col_name']` validate the column name
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #42608:
URL: https://github.com/apache/spark/pull/42608#discussion_r1302605786
##########
python/pyspark/testing/connectutils.py:
##########
@@ -76,6 +76,7 @@ class MockRemoteSession:
def __init__(self):
self.hooks = {}
self.session_id = str(uuid.uuid4())
+ self.is_stopped = True
Review Comment:
I tried `hasattr`, but it won't work, I guess that is due to `MockRemoteSession` has its own `__getattr__` method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #42608: [SPARK-42017][PYTHON][CONNECT][TESTS] Enable `ColumnParityTests.test_access_column`
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #42608:
URL: https://github.com/apache/spark/pull/42608#discussion_r1302385936
##########
python/pyspark/sql/connect/dataframe.py:
##########
@@ -1623,6 +1624,10 @@ def __getitem__(self, item: Union[int, str, Column, List, Tuple]) -> Union[Colum
alias = self._get_alias()
if self._plan is None:
raise SparkConnectException("Cannot analyze on empty plan.")
+
+ if "." not in item and "*" not in item and "`" not in item and item not in self.columns:
Review Comment:
I now hesitate whether this is worthwhile @HyukjinKwon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng commented on a diff in pull request #42608: [SPARK-42017][PYTHON][CONNECT][TESTS] Enable `ColumnParityTests.test_access_column`
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #42608:
URL: https://github.com/apache/spark/pull/42608#discussion_r1302514076
##########
python/pyspark/sql/connect/dataframe.py:
##########
@@ -1623,6 +1624,10 @@ def __getitem__(self, item: Union[int, str, Column, List, Tuple]) -> Union[Colum
alias = self._get_alias()
if self._plan is None:
raise SparkConnectException("Cannot analyze on empty plan.")
+
+ if "." not in item and "*" not in item and "`" not in item and item not in self.columns:
Review Comment:
good idea!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zhengruifeng closed pull request #42608: [SPARK-42017][PYTHON][CONNECT] `df['col_name']` should validate the column name
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng closed pull request #42608: [SPARK-42017][PYTHON][CONNECT] `df['col_name']` should validate the column name
URL: https://github.com/apache/spark/pull/42608
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org