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 2024/02/02 06:10:57 UTC
[PR] [SPARK-46952][SQL] XML: Limit size of corrupt record [spark]
sandip-db opened a new pull request, #44994:
URL: https://github.com/apache/spark/pull/44994
### What changes were proposed in this pull request?
Limit the size of malformed XML string that gets stored in the corrupt column.
### Why are the changes needed?
A large corrupt XML record can be arbitrarily large and may cause OOM.
### Does this PR introduce _any_ user-facing change?
Yes
### How was this patch tested?
Unit test
### 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-46952][SQL] XML: Limit size of corrupt record [spark]
Posted by "sandip-db (via GitHub)" <gi...@apache.org>.
sandip-db commented on code in PR #44994:
URL: https://github.com/apache/spark/pull/44994#discussion_r1475592244
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -145,7 +145,8 @@ class StaxXmlParser(
def doParseColumn(xml: String,
parseMode: ParseMode,
xsdSchema: Option[Schema]): Option[InternalRow] = {
- val xmlRecord = UTF8String.fromString(xml)
+ // limit the size of the XML records to add in bad record exception
Review Comment:
Adding an option to fine tune corrupt record handling may be an overkill.
`UTF8String.fromString` throws `NegativeArraySizeException` for strings of size 1B.
I can increase the limit to close to that (say 512 M) to avoid this exception. WDYT?
Alternatively, I can remove the limit and just do `lazy val` that would throw an the above exception when a large bad record is encountered.
--
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-46952][SQL] XML: Limit size of corrupt record [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44994:
URL: https://github.com/apache/spark/pull/44994#discussion_r1475600342
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -145,7 +145,8 @@ class StaxXmlParser(
def doParseColumn(xml: String,
parseMode: ParseMode,
xsdSchema: Option[Schema]): Option[InternalRow] = {
- val xmlRecord = UTF8String.fromString(xml)
+ // limit the size of the XML records to add in bad record exception
Review Comment:
just `lazy val` sounds simpler ... or we can just add an internal `SQLConf` to control this. e.g.,
```scala
buildConf("spark.sql.analyzer.maxIterations")
.internal()
.doc("...")
.version("4.0.0")
...
```
--
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-46952][SQL] XML: Limit size of corrupt record [spark]
Posted by "sandip-db (via GitHub)" <gi...@apache.org>.
sandip-db commented on code in PR #44994:
URL: https://github.com/apache/spark/pull/44994#discussion_r1475604878
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -145,7 +145,8 @@ class StaxXmlParser(
def doParseColumn(xml: String,
parseMode: ParseMode,
xsdSchema: Option[Schema]): Option[InternalRow] = {
- val xmlRecord = UTF8String.fromString(xml)
+ // limit the size of the XML records to add in bad record exception
Review Comment:
Thx. Updated the PR with just `lazy val`
--
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-46952][SQL] XML: Limit size of corrupt record [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #44994:
URL: https://github.com/apache/spark/pull/44994#issuecomment-1923035371
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-46952][SQL] XML: Limit size of corrupt record [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44994:
URL: https://github.com/apache/spark/pull/44994#discussion_r1475585497
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -145,7 +145,8 @@ class StaxXmlParser(
def doParseColumn(xml: String,
parseMode: ParseMode,
xsdSchema: Option[Schema]): Option[InternalRow] = {
- val xmlRecord = UTF8String.fromString(xml)
+ // limit the size of the XML records to add in bad record exception
Review Comment:
Hm, shouldn't we have an option for 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-46952][SQL] XML: Limit size of corrupt record [spark]
Posted by "sandip-db (via GitHub)" <gi...@apache.org>.
sandip-db commented on code in PR #44994:
URL: https://github.com/apache/spark/pull/44994#discussion_r1475592244
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -145,7 +145,8 @@ class StaxXmlParser(
def doParseColumn(xml: String,
parseMode: ParseMode,
xsdSchema: Option[Schema]): Option[InternalRow] = {
- val xmlRecord = UTF8String.fromString(xml)
+ // limit the size of the XML records to add in bad record exception
Review Comment:
Adding an option to fine tune corrupt record handling may be an overkill.
`UTF8String.fromString` throws `NegativeArraySizeException` for strings of size 1B.
I can increase the limit close to that (say 512 M) to avoid this exception. WDYT?
Alternatively, I can remove the limit and just do `lazy val` that would throw the above exception when a large bad record is encountered.
--
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-46952][SQL] XML: Limit size of corrupt record [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44994:
URL: https://github.com/apache/spark/pull/44994#discussion_r1475600342
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -145,7 +145,8 @@ class StaxXmlParser(
def doParseColumn(xml: String,
parseMode: ParseMode,
xsdSchema: Option[Schema]): Option[InternalRow] = {
- val xmlRecord = UTF8String.fromString(xml)
+ // limit the size of the XML records to add in bad record exception
Review Comment:
just `lazy val` sounds simpler ... or we can just add an internal `SQLConf` to control this. e.g.,
```scala
buildConf("spark.sql.xml.blahblah.size")
.internal()
.doc("...")
.version("4.0.0")
...
```
--
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-46952][SQL] XML: Limit size of corrupt record [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #44994: [SPARK-46952][SQL] XML: Limit size of corrupt record
URL: https://github.com/apache/spark/pull/44994
--
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