You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "sandip-db (via GitHub)" <gi...@apache.org> on 2023/11/08 04:03:08 UTC
[PR] [SPARK-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
sandip-db opened a new pull request, #43710:
URL: https://github.com/apache/spark/pull/43710
### What changes were proposed in this pull request?
rowTag option is required for reading XML files. This PR adds a SQL error class for missing rowTag option.
### Why are the changes needed?
rowTag option is required for reading XML files. This PR adds a SQL error class for missing rowTag option.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Updated the unit test to check for error message.
### 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
Re: [PR] [SPARK-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #43710:
URL: https://github.com/apache/spark/pull/43710#discussion_r1387600755
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3899,6 +3899,12 @@
},
"sqlState" : "42605"
},
+ "XML_ROW_TAG_OPTION_REQUIRED" : {
Review Comment:
Yep, I agree. The error class name should point out a problem, possible solutions should be in `message`.
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43710:
URL: https://github.com/apache/spark/pull/43710#discussion_r1387571783
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlOptions.scala:
##########
@@ -66,7 +66,11 @@ private[sql] class XmlOptions(
val compressionCodec = parameters.get(COMPRESSION).map(CompressionCodecs.getCodecClassName)
val rowTagOpt = parameters.get(XmlOptions.ROW_TAG).map(_.trim)
- require(!rowTagRequired || rowTagOpt.isDefined, s"'${XmlOptions.ROW_TAG}' option is required.")
+
+ if (rowTagRequired && rowTagOpt.isEmpty) {
+ throw QueryCompilationErrors.xmlRowTagRequiredError(XmlOptions.ROW_TAG)
Review Comment:
I got it now. I'm missing the `rowTagRequired`. The `rowTagRequired` is false if the call the `to_xml`.
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43710:
URL: https://github.com/apache/spark/pull/43710#discussion_r1386220959
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -30,12 +30,13 @@ import org.apache.commons.lang3.exception.ExceptionUtils
import org.apache.hadoop.conf.Configuration
import org.apache.hadoop.io.{LongWritable, Text}
import org.apache.hadoop.io.compress.GzipCodec
-
Review Comment:
Please revert it.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -30,12 +30,13 @@ import org.apache.commons.lang3.exception.ExceptionUtils
import org.apache.hadoop.conf.Configuration
import org.apache.hadoop.io.{LongWritable, Text}
import org.apache.hadoop.io.compress.GzipCodec
-
import org.apache.spark.SparkException
+
Review Comment:
ditto.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3805,4 +3805,11 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
errorClass = "DATA_SOURCE_ALREADY_EXISTS",
messageParameters = Map("provider" -> name))
}
+
+ def xmlRowTagRequiredError(optionName: String): Throwable = {
+ new AnalysisException(
+ errorClass = "XML_ROW_TAG_OPTION_REQUIRED",
+ messageParameters = Map("rowTag" -> optionName)
Review Comment:
```suggestion
messageParameters = Map("rowTag" -> toSQLId(optionName))
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlOptions.scala:
##########
@@ -66,7 +66,11 @@ private[sql] class XmlOptions(
val compressionCodec = parameters.get(COMPRESSION).map(CompressionCodecs.getCodecClassName)
val rowTagOpt = parameters.get(XmlOptions.ROW_TAG).map(_.trim)
- require(!rowTagRequired || rowTagOpt.isDefined, s"'${XmlOptions.ROW_TAG}' option is required.")
+
+ if (rowTagRequired && rowTagOpt.isEmpty) {
+ throw QueryCompilationErrors.xmlRowTagRequiredError(XmlOptions.ROW_TAG)
Review Comment:
Because `val rowTag = rowTagOpt.getOrElse(XmlOptions.DEFAULT_ROW_TAG)`, do we really need throw the error?
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "sandip-db (via GitHub)" <gi...@apache.org>.
sandip-db commented on code in PR #43710:
URL: https://github.com/apache/spark/pull/43710#discussion_r1386617633
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlOptions.scala:
##########
@@ -66,7 +66,11 @@ private[sql] class XmlOptions(
val compressionCodec = parameters.get(COMPRESSION).map(CompressionCodecs.getCodecClassName)
val rowTagOpt = parameters.get(XmlOptions.ROW_TAG).map(_.trim)
- require(!rowTagRequired || rowTagOpt.isDefined, s"'${XmlOptions.ROW_TAG}' option is required.")
+
+ if (rowTagRequired && rowTagOpt.isEmpty) {
+ throw QueryCompilationErrors.xmlRowTagRequiredError(XmlOptions.ROW_TAG)
Review Comment:
As discussed in the earlier [PR](https://github.com/apache/spark/pull/43389), the default row tag is not going to match any real XML files and confuse users. This PR standardize this error for other services built on top of this.
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43710:
URL: https://github.com/apache/spark/pull/43710#issuecomment-1805319284
Lemme merge first but please let me know if there's anything that doesn't look making sense to you.
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43710:
URL: https://github.com/apache/spark/pull/43710#discussion_r1387422030
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlOptions.scala:
##########
@@ -66,7 +66,11 @@ private[sql] class XmlOptions(
val compressionCodec = parameters.get(COMPRESSION).map(CompressionCodecs.getCodecClassName)
val rowTagOpt = parameters.get(XmlOptions.ROW_TAG).map(_.trim)
- require(!rowTagRequired || rowTagOpt.isDefined, s"'${XmlOptions.ROW_TAG}' option is required.")
+
+ if (rowTagRequired && rowTagOpt.isEmpty) {
+ throw QueryCompilationErrors.xmlRowTagRequiredError(XmlOptions.ROW_TAG)
Review Comment:
if so, we should change `val rowTag = rowTagOpt.getOrElse(XmlOptions.DEFAULT_ROW_TAG)` to `val rowTag = rowTagOpt.get`
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43710:
URL: https://github.com/apache/spark/pull/43710#discussion_r1388783036
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3899,6 +3899,12 @@
},
"sqlState" : "42605"
},
+ "XML_ROW_TAG_MISSING" : {
+ "message" : [
+ "<rowTag> option is required for reading files in XML format."
+ ],
+ "sqlState" : "42000"
Review Comment:
cc @MaxGekk to confirm.
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #43710: [SPARK-45562][SQL] XML: Add SQL error class for missing rowTag option
URL: https://github.com/apache/spark/pull/43710
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43710:
URL: https://github.com/apache/spark/pull/43710#issuecomment-1805319416
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
Re: [PR] [SPARK-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43710:
URL: https://github.com/apache/spark/pull/43710#discussion_r1387573558
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3899,6 +3899,12 @@
},
"sqlState" : "42605"
},
+ "XML_ROW_TAG_OPTION_REQUIRED" : {
Review Comment:
How about `XML_ROW_TAG_MISSING`?
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "sandip-db (via GitHub)" <gi...@apache.org>.
sandip-db commented on code in PR #43710:
URL: https://github.com/apache/spark/pull/43710#discussion_r1387566546
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlOptions.scala:
##########
@@ -66,7 +66,11 @@ private[sql] class XmlOptions(
val compressionCodec = parameters.get(COMPRESSION).map(CompressionCodecs.getCodecClassName)
val rowTagOpt = parameters.get(XmlOptions.ROW_TAG).map(_.trim)
- require(!rowTagRequired || rowTagOpt.isDefined, s"'${XmlOptions.ROW_TAG}' option is required.")
+
+ if (rowTagRequired && rowTagOpt.isEmpty) {
+ throw QueryCompilationErrors.xmlRowTagRequiredError(XmlOptions.ROW_TAG)
Review Comment:
Currently, `to_xml` doesn't throw an error if `rowTag` is not specified because it uses the default. If we do the change that you suggested above, even `to_xml` will require `rowTag` and throw an error if one is not provided. This PR is just standardizing the error that gets thrown when rowTag is not provided on "read". It doesn't change any other behavior.
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #43710:
URL: https://github.com/apache/spark/pull/43710#discussion_r1387559815
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlOptions.scala:
##########
@@ -66,7 +66,11 @@ private[sql] class XmlOptions(
val compressionCodec = parameters.get(COMPRESSION).map(CompressionCodecs.getCodecClassName)
val rowTagOpt = parameters.get(XmlOptions.ROW_TAG).map(_.trim)
- require(!rowTagRequired || rowTagOpt.isDefined, s"'${XmlOptions.ROW_TAG}' option is required.")
+
+ if (rowTagRequired && rowTagOpt.isEmpty) {
+ throw QueryCompilationErrors.xmlRowTagRequiredError(XmlOptions.ROW_TAG)
Review Comment:
So, the error appears if users call `to_xml` and do not specify the rowTag option?
--
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-45562][SQL] XML: Add SQL error class for missing rowTag option [spark]
Posted by "sandip-db (via GitHub)" <gi...@apache.org>.
sandip-db commented on code in PR #43710:
URL: https://github.com/apache/spark/pull/43710#discussion_r1387500399
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlOptions.scala:
##########
@@ -66,7 +66,11 @@ private[sql] class XmlOptions(
val compressionCodec = parameters.get(COMPRESSION).map(CompressionCodecs.getCodecClassName)
val rowTagOpt = parameters.get(XmlOptions.ROW_TAG).map(_.trim)
- require(!rowTagRequired || rowTagOpt.isDefined, s"'${XmlOptions.ROW_TAG}' option is required.")
+
+ if (rowTagRequired && rowTagOpt.isEmpty) {
+ throw QueryCompilationErrors.xmlRowTagRequiredError(XmlOptions.ROW_TAG)
Review Comment:
That would make `rowTag` required for other paths like `to_xml`, where it is ok to use the default.
--
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-45562][SQL][FOLLOW-UP] XML: Add SQL error class for missing rowTag option [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43710:
URL: https://github.com/apache/spark/pull/43710#issuecomment-1805321110
BTW, let's don't forget to add `[FOLLOW-UP]` tag
--
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