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/03/31 10:39:28 UTC
[GitHub] [spark] itholic opened a new pull request, #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
itholic opened a new pull request, #40624:
URL: https://github.com/apache/spark/pull/40624
### What changes were proposed in this pull request?
This PR proposes to migrate remaining `TypeError` from `python/pyspark/sql/connect/dataframe.py` into `PySparkTypeError`.
### Why are the changes needed?
To migrate all user-facing errors into PySpark error framework.
### 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, it's internal error framework improvement.
### 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.
-->
The existing CI should pass.
--
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 #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #40624:
URL: https://github.com/apache/spark/pull/40624#discussion_r1156612358
##########
python/pyspark/errors/error_classes.py:
##########
@@ -59,6 +64,21 @@
"Argument `<arg_name>` should be a bool, dict, float, int or str, got <arg_type>."
Review Comment:
Okay, then let me go with option 1 even though the detail of the error definitions is a bit different.
--
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 #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #40624:
URL: https://github.com/apache/spark/pull/40624#discussion_r1156612474
##########
python/pyspark/errors/error_classes.py:
##########
@@ -59,6 +64,21 @@
"Argument `<arg_name>` should be a bool, dict, float, int or str, got <arg_type>."
Review Comment:
Can I do this in the separate ticket?
--
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 #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #40624:
URL: https://github.com/apache/spark/pull/40624#discussion_r1156590284
##########
python/pyspark/errors/error_classes.py:
##########
@@ -59,6 +64,21 @@
"Argument `<arg_name>` should be a bool, dict, float, int or str, got <arg_type>."
Review Comment:
Yes. Since type-related error classes are increasing, it seems like it's time for refactoring.
Maybe there are two method in my mind for refactoring as below:
- We can create a main error class called such as `TYPE_MISMATCH` and convert all type-related errors to sub-error classes of `TYPE_MISMATCH`.
- Alternatively, as you suggested, we can create a `TYPE_MISMATCH` error class and add a parameter like `allowed_types` to manage all TYPE-related errors at once.
Which method do you think is more reasonable?
--
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 #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #40624:
URL: https://github.com/apache/spark/pull/40624#discussion_r1156613078
##########
python/pyspark/errors/error_classes.py:
##########
@@ -59,6 +64,21 @@
"Argument `<arg_name>` should be a bool, dict, float, int or str, got <arg_type>."
Review Comment:
Because applying the sub-error class can be a little complicated, so I want to proceed after finishing the rest of the PRs currently opened.
--
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 #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40624:
URL: https://github.com/apache/spark/pull/40624#discussion_r1156585013
##########
python/pyspark/errors/error_classes.py:
##########
@@ -59,6 +64,21 @@
"Argument `<arg_name>` should be a bool, dict, float, int or str, got <arg_type>."
Review Comment:
should all these type have a different error classes? I was was thinking about adding these allowed types as a parameter, cc @MaxGekk FYI
--
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 #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40624:
URL: https://github.com/apache/spark/pull/40624#discussion_r1156592427
##########
python/pyspark/errors/error_classes.py:
##########
@@ -59,6 +64,21 @@
"Argument `<arg_name>` should be a bool, dict, float, int or str, got <arg_type>."
Review Comment:
Whatever matching to the current error classes in Scala would be nice.
--
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 #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #40624:
URL: https://github.com/apache/spark/pull/40624#discussion_r1156612474
##########
python/pyspark/errors/error_classes.py:
##########
@@ -59,6 +64,21 @@
"Argument `<arg_name>` should be a bool, dict, float, int or str, got <arg_type>."
Review Comment:
Can I do this in the separate ticket? Just created one for this: SPARK-43020
--
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 #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #40624:
URL: https://github.com/apache/spark/pull/40624#discussion_r1156590284
##########
python/pyspark/errors/error_classes.py:
##########
@@ -59,6 +64,21 @@
"Argument `<arg_name>` should be a bool, dict, float, int or str, got <arg_type>."
Review Comment:
Yes. Since type-related error classes are increasing, it seems like it's time for refactoring.
Maybe there are two method in my mind for refactoring as below:
- We can create a main error class called such as `TYPE_MISMATCH` and convert all type-related errors to sub-error classes of `TYPE_MISMATCH`.
- Alternatively, as you suggested, we can create a `TYPE_MISMATCH` error class and add a parameter like `allowed_types` to manage all type-related errors at once under the single error class.
Which method do you think is more reasonable?
--
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 #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
URL: https://github.com/apache/spark/pull/40624
--
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 #40624: [SPARK-42995][CONNECT][PYTHON] Migrate Spark Connect DataFrame errors into error class
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #40624:
URL: https://github.com/apache/spark/pull/40624#issuecomment-1497270264
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