You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "cloud-fan (via GitHub)" <gi...@apache.org> on 2024/03/26 12:15:48 UTC
[PR] [SPARK-47564][SQL] Always throw FAILED_READ_FILE error when fail to read files [spark]
cloud-fan opened a new pull request, #45723:
URL: https://github.com/apache/spark/pull/45723
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### 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 is a followup of https://github.com/apache/spark/pull/44953 to refine the newly added `FAILED_READ_FILE` error. It's better to always throw `FAILED_READ_FILE` error if anything goes wrong during file reading. This is more predictable and easier for users to do error handling. This PR adds sub error classes to `FAILED_READ_FILE` so that users can know what went wrong quicker.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
better error reporting
### 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, `FAILED_READ_FILE` is not released yet.
### 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.
-->
existing tests
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
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
Re: [PR] [SPARK-47564][SQL] Always throw FAILED_READ_FILE error when fail to read files [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #45723: [SPARK-47564][SQL] Always throw FAILED_READ_FILE error when fail to read files
URL: https://github.com/apache/spark/pull/45723
--
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
Re: [PR] [SPARK-47564][SQL] Always throw FAILED_READ_FILE error when fail to read files [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45723:
URL: https://github.com/apache/spark/pull/45723#discussion_r1540544473
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroLogicalTypeSuite.scala:
##########
@@ -436,7 +436,7 @@ abstract class AvroLogicalTypeSuite extends QueryTest with SharedSparkSession {
val ex = intercept[SparkException] {
spark.read.format("avro").load(s"$dir.avro").collect()
}
- assert(ex.getErrorClass == "FAILED_READ_FILE")
+ assert(ex.getErrorClass.startsWith("FAILED_READ_FILE"))
Review Comment:
We may keep adding more sub error classes to `FAILED_READ_FILE` and I want the tests to be stable.
--
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
Re: [PR] [SPARK-47564][SQL] Always throw FAILED_READ_FILE error when fail to read files [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45723:
URL: https://github.com/apache/spark/pull/45723#discussion_r1539855370
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1257,6 +1249,31 @@
"message" : [
"Encountered error while reading file <path>."
],
+ "subClass" : {
+ "CANNOT_READ_FILE_FOOTER" : {
+ "message" : [
+ "Could not read footer. Please ensure that the file is in either ORC or Parquet format.",
+ "If not, please convert it to a valid format. If the file is in the valid format, please check if it is corrupt.",
+ "If it is, you can choose to either ignore it or fix the corruption."
+ ]
+ },
+ "FILE_NOT_EXIST" : {
+ "message" : [
+ "File does not exist. It is possible the underlying files have been updated.",
+ "You can explicitly invalidate the cache in Spark by running 'REFRESH TABLE tableName' command in SQL or by recreating the Dataset/DataFrame involved."
+ ]
+ },
+ "NO_HINT" : {
+ "message" : [
+ ""
+ ]
+ },
+ "PARQUET_COLUMN_DATA_TYPE_MISMATCH" : {
+ "message" : [
+ "Data type mismatches when reading Parquet column <column>: Expected: <expectedType>, Found: <actualType>."
Review Comment:
too much `:`.
```suggestion
"Data type mismatches when reading Parquet column <column>. Expected Spark type <expectedType>, actual Parquet type <actualType>."
```
--
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
Re: [PR] [SPARK-47564][SQL] Always throw FAILED_READ_FILE error when fail to read files [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45723:
URL: https://github.com/apache/spark/pull/45723#discussion_r1540537958
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -805,33 +799,34 @@ private[sql] object QueryExecutionErrors extends QueryErrorsBase with ExecutionE
cause = null)
}
- def fileNotFoundError(e: FileNotFoundException): SparkFileNotFoundException = {
- new SparkFileNotFoundException(
- errorClass = "_LEGACY_ERROR_TEMP_2062",
- messageParameters = Map("message" -> e.getMessage))
+ def fileNotExistError(path: String, e: Exception): Throwable = {
+ new SparkException(
Review Comment:
Why did you change `SparkFileNotFoundException` to `SparkException`. This can break user's apps, I think. If they catch the exact `SparkFileNotFoundException`, they will miss your `SparkException`. Especially, after you changed the error class too. How should user know about your changes?
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1257,6 +1249,31 @@
"message" : [
"Encountered error while reading file <path>."
],
+ "subClass" : {
+ "CANNOT_READ_FILE_FOOTER" : {
+ "message" : [
+ "Could not read footer. Please ensure that the file is in either ORC or Parquet format.",
+ "If not, please convert it to a valid format. If the file is in the valid format, please check if it is corrupt.",
+ "If it is, you can choose to either ignore it or fix the corruption."
+ ]
+ },
+ "FILE_NOT_EXIST" : {
+ "message" : [
+ "File does not exist. It is possible the underlying files have been updated.",
+ "You can explicitly invalidate the cache in Spark by running 'REFRESH TABLE tableName' command in SQL or by recreating the Dataset/DataFrame involved."
+ ]
+ },
+ "NO_HINT" : {
+ "message" : [
+ ""
+ ]
+ },
+ "PARQUET_COLUMN_DATA_TYPE_MISMATCH" : {
+ "message" : [
+ "Data type mismatches when reading Parquet column <column>: Expected: <expectedType>, Found: <actualType>."
Review Comment:
too much `:`.
```suggestion
"Data type mismatches when reading Parquet column <column>. Expected <expectedType> but found <actualType>."
```
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1257,6 +1249,31 @@
"message" : [
"Encountered error while reading file <path>."
],
+ "subClass" : {
+ "CANNOT_READ_FILE_FOOTER" : {
+ "message" : [
+ "Could not read footer. Please ensure that the file is in either ORC or Parquet format.",
+ "If not, please convert it to a valid format. If the file is in the valid format, please check if it is corrupt.",
+ "If it is, you can choose to either ignore it or fix the corruption."
+ ]
+ },
+ "FILE_NOT_EXIST" : {
+ "message" : [
+ "File does not exist. It is possible the underlying files have been updated.",
+ "You can explicitly invalidate the cache in Spark by running 'REFRESH TABLE tableName' command in SQL or by recreating the Dataset/DataFrame involved."
+ ]
+ },
+ "NO_HINT" : {
+ "message" : [
+ ""
+ ]
+ },
+ "PARQUET_COLUMN_DATA_TYPE_MISMATCH" : {
+ "message" : [
+ "Data type mismatches when reading Parquet column <column>: Expected: <expectedType>, Found: <actualType>."
Review Comment:
BTW, since we are talking about the parquet format, shouldn't we mention that one type is a logical type, but another is a physical type? Otherwise you compare apples to oranges.
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroLogicalTypeSuite.scala:
##########
@@ -436,7 +436,7 @@ abstract class AvroLogicalTypeSuite extends QueryTest with SharedSparkSession {
val ex = intercept[SparkException] {
spark.read.format("avro").load(s"$dir.avro").collect()
}
- assert(ex.getErrorClass == "FAILED_READ_FILE")
+ assert(ex.getErrorClass.startsWith("FAILED_READ_FILE"))
Review Comment:
Why don't compare to `FAILED_READ_FILE.NO_HINT`?
--
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
Re: [PR] [SPARK-47564][SQL] Always throw FAILED_READ_FILE error when fail to read files [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45723:
URL: https://github.com/apache/spark/pull/45723#discussion_r1540547369
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1257,6 +1249,31 @@
"message" : [
"Encountered error while reading file <path>."
],
+ "subClass" : {
+ "CANNOT_READ_FILE_FOOTER" : {
+ "message" : [
+ "Could not read footer. Please ensure that the file is in either ORC or Parquet format.",
+ "If not, please convert it to a valid format. If the file is in the valid format, please check if it is corrupt.",
+ "If it is, you can choose to either ignore it or fix the corruption."
+ ]
+ },
+ "FILE_NOT_EXIST" : {
+ "message" : [
+ "File does not exist. It is possible the underlying files have been updated.",
+ "You can explicitly invalidate the cache in Spark by running 'REFRESH TABLE tableName' command in SQL or by recreating the Dataset/DataFrame involved."
+ ]
+ },
+ "NO_HINT" : {
+ "message" : [
+ ""
+ ]
+ },
+ "PARQUET_COLUMN_DATA_TYPE_MISMATCH" : {
+ "message" : [
+ "Data type mismatches when reading Parquet column <column>: Expected: <expectedType>, Found: <actualType>."
Review Comment:
I find the logical vs physical type a bit confusing. How about `Expected Spark type: ..., actual Parquet type ...`.
--
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
Re: [PR] [SPARK-47564][SQL] Always throw FAILED_READ_FILE error when fail to read files [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45723:
URL: https://github.com/apache/spark/pull/45723#issuecomment-2022610264
+1, LGTM. Merging to master.
Thank you, @cloud-fan and @yaooqinn @HyukjinKwon for review.
--
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
Re: [PR] [SPARK-47564][SQL] Always throw FAILED_READ_FILE error when fail to read files [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45723:
URL: https://github.com/apache/spark/pull/45723#discussion_r1540545924
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -805,33 +799,34 @@ private[sql] object QueryExecutionErrors extends QueryErrorsBase with ExecutionE
cause = null)
}
- def fileNotFoundError(e: FileNotFoundException): SparkFileNotFoundException = {
- new SparkFileNotFoundException(
- errorClass = "_LEGACY_ERROR_TEMP_2062",
- messageParameters = Map("message" -> e.getMessage))
+ def fileNotExistError(path: String, e: Exception): Throwable = {
+ new SparkException(
Review Comment:
Note: at the end, we threw `SparkException` before: https://github.com/apache/spark/pull/44953/files#diff-158a89983d40e2ebba7048dd10bac07b6c4fa03d70ee608923b093b314219f19L254 . This change is to make the behavior closer to before.
--
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
Re: [PR] [SPARK-47564][SQL] Always throw FAILED_READ_FILE error when fail to read files [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45723:
URL: https://github.com/apache/spark/pull/45723#discussion_r1540551006
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -1257,6 +1249,31 @@
"message" : [
"Encountered error while reading file <path>."
],
+ "subClass" : {
+ "CANNOT_READ_FILE_FOOTER" : {
+ "message" : [
+ "Could not read footer. Please ensure that the file is in either ORC or Parquet format.",
+ "If not, please convert it to a valid format. If the file is in the valid format, please check if it is corrupt.",
+ "If it is, you can choose to either ignore it or fix the corruption."
+ ]
+ },
+ "FILE_NOT_EXIST" : {
+ "message" : [
+ "File does not exist. It is possible the underlying files have been updated.",
+ "You can explicitly invalidate the cache in Spark by running 'REFRESH TABLE tableName' command in SQL or by recreating the Dataset/DataFrame involved."
+ ]
+ },
+ "NO_HINT" : {
+ "message" : [
+ ""
+ ]
+ },
+ "PARQUET_COLUMN_DATA_TYPE_MISMATCH" : {
+ "message" : [
+ "Data type mismatches when reading Parquet column <column>: Expected: <expectedType>, Found: <actualType>."
Review Comment:
SGTM
--
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