You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "itholic (via GitHub)" <gi...@apache.org> on 2023/02/13 06:52:22 UTC
[GitHub] [spark] itholic opened a new pull request, #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
itholic opened a new pull request, #39991:
URL: https://github.com/apache/spark/pull/39991
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR proposes to migrate `TypeError` into error framework for Spark Connect Column API.
### Why are the changes needed?
To improve errors by leveraging the PySpark error framework for Spark Connect Column APIs.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
Fixed & added UTs.
--
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 pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39991:
URL: https://github.com/apache/spark/pull/39991#issuecomment-1436398640
Still fails. Mind fixing them please?
--
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 pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39991:
URL: https://github.com/apache/spark/pull/39991#issuecomment-1429285011
@itholic mind fixing the test?
--
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 pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39991:
URL: https://github.com/apache/spark/pull/39991#issuecomment-1427450463
cc @MaxGekk would you mind taking a look please when you find some time.
--
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] itholic commented on pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on PR #39991:
URL: https://github.com/apache/spark/pull/39991#issuecomment-1433037827
Fixed the test. 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] itholic commented on a diff in pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #39991:
URL: https://github.com/apache/spark/pull/39991#discussion_r1109499277
##########
python/pyspark/errors/error_classes.py:
##########
@@ -39,6 +39,11 @@
"Function `<func_name>` should return Column, got <return_type>."
]
},
+ "IS_NOT_ITERABLE" : {
Review Comment:
I intentionally added `IS_` since the error message format is different from other `NOT_***` error classes, e.g.
```
Argument `<arg_name>` should be a ** or **, got <arg_type>."
```
But I also agree with it looks a bit redundant compare to others.
Let me just use `NOT_ITERABLE` anyway.
Thanks! :-)
##########
python/pyspark/errors/error_classes.py:
##########
@@ -39,6 +39,11 @@
"Function `<func_name>` should return Column, got <return_type>."
]
},
+ "IS_NOT_ITERABLE" : {
Review Comment:
Actually I intentionally added `IS_` since the error message format is different from other `NOT_***` error classes, e.g.
```
Argument `<arg_name>` should be a ** or **, got <arg_type>."
```
But I also agree with it looks a bit redundant compare to others.
Let me just use `NOT_ITERABLE` anyway.
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] HyukjinKwon commented on pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39991:
URL: https://github.com/apache/spark/pull/39991#issuecomment-1445518870
Merged to master and branch-3.4.
--
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] amaliujia commented on pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on PR #39991:
URL: https://github.com/apache/spark/pull/39991#issuecomment-1439330072
LGTM
--
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] itholic commented on pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on PR #39991:
URL: https://github.com/apache/spark/pull/39991#issuecomment-1439304707
FYI @HyukjinKwon CI passed!
--
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 closed pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
URL: https://github.com/apache/spark/pull/39991
--
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] amaliujia commented on a diff in pull request #39991: [SPARK-42419][CONNECT][PYTHON] Migrate into error framework for Spark Connect Column API.
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #39991:
URL: https://github.com/apache/spark/pull/39991#discussion_r1109061823
##########
python/pyspark/errors/error_classes.py:
##########
@@ -39,6 +39,11 @@
"Function `<func_name>` should return Column, got <return_type>."
]
},
+ "IS_NOT_ITERABLE" : {
Review Comment:
Nit: `IS_` seems to be redundant. Other message names do not adopt `IS_`.
--
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