You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "shujingyang-db (via GitHub)" <gi...@apache.org> on 2023/12/12 19:04:48 UTC
[PR] [SPARK-46382] XML: Capture values interspersed between elements [spark]
shujingyang-db opened a new pull request, #44318:
URL: https://github.com/apache/spark/pull/44318
<!--
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
'core/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.
-->
In XML, elements typically consist of a name and a value, with the value enclosed between the opening and closing tags. But XML also allows to include arbitrary values interspersed between these elements. To address this, we provide an option named `valueTags`, which is enabled by default, to capture these values. Consider the following example:
```
<ROW>
<a>1</a>
value1
<b>
value2
<c>2</c>
value3
</b>
</ROW>
```
In this example, `<a>`, `<b>`, and `<c>` are named elements with their respective values enclosed within tags. There are arbitrary values value1 value2 value3 interspersed between the elements. Please note that there can be multiple occurrences of values in a single element (i.e. there are value2, value3 in the element <b>)
We should parse the values between tags into the valueTags field. If there are multiple occurrences of value tags, the value tag field will be converted to an array type.
### 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.
-->
We should parse the values otherwise there would be data loss
### 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'.
-->
Yes
### 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.
-->
Unit test
### 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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #44318: [SPARK-46382][SQL] XML: Capture values interspersed between elements
URL: https://github.com/apache/spark/pull/44318
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1434534192
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -243,6 +244,22 @@ class XmlInferSchema(options: XmlOptions, caseSensitive: Boolean)
}
}
+ @tailrec
+ def inferAndCheckEndElement(parser: XMLEventReader): Boolean = {
+ parser.peek match {
+ case _: EndElement | _: EndDocument => true
+ case _: StartElement => false
+ case c: Characters if !c.isWhiteSpace =>
+ val characterType = inferFrom(c.getData)
+ parser.nextEvent()
+ addOrUpdateType(options.valueTag, characterType)
Review Comment:
A valueTag that locates after a closing tag in the inner element and before the closing tag in the outer element will cover this scenario.
```
<a>
value2
<b>1</b>
value3
</a>
```
We covered this case in the most our test cases
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1430945581
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -194,27 +210,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (isEmptyString(c)) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
+ case None => null
+ }
+ case _ =>
+ val row = convertObject(parser, st)
+ if (!isEmptyString(c)) {
+ addOrUpdate(row.toSeq(st).toArray, st, options.valueTag, c.getData, addToTail = false)
+ } else {
+ row
+ }
}
case (_: Characters, _: StringType) =>
Review Comment:
We don't need to move the next event. `currentStructureAsString` will move the parser pointer.
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1431029561
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -1729,9 +1729,15 @@ class XmlSuite extends QueryTest with SharedSparkSession {
val TAG_NAME = "tag"
val VALUETAG_NAME = "_VALUE"
val schema = buildSchema(
+ field(VALUETAG_NAME),
Review Comment:
We sort the field name in ascending order. The `_VALUE` comes before `_attr`
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1430944306
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -171,16 +171,18 @@ class XmlInferSchema(options: XmlOptions, caseSensitive: Boolean)
case _: EndElement => StringType
case _ => inferField(parser)
}
- case c: Characters if !c.isWhiteSpace =>
+ // what about new line character
+ case c: Characters if !isEmptyString(c) =>
Review Comment:
IMHO inferObject can't do this. This branch handles both primitive types and nested objects. If we return `inferObject(parser)`, the primitive types will be inferred as a structFields of valueTag
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1434433839
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -534,4 +541,55 @@ class XmlInferSchema(options: XmlOptions, caseSensitive: Boolean)
}
}
}
+
+ /**
+ * This helper function merges the data type of value tags and inner elements.
+ * It could only be structure data. Consider the following case,
+ * <a>
+ * value1
+ * <b>1</b>
+ * value2
+ * </a>
+ * Input: ''a struct<b int, _VALUE string>'' and ''_VALUE string''
+ * Return: ''a struct<b int, _VALUE array<string>>''
+ * @param objectType inner elements' type
+ * @param valueTagType value tag's type
+ */
+ private[xml] def addOrUpdateValueTagType(
+ objectType: DataType,
+ valueTagType: DataType): DataType = {
+ (objectType, valueTagType) match {
+ case (st: StructType, _) =>
+ // TODO(shujing): case sensitive?
+ val valueTagIndexOpt = st.getFieldIndex(options.valueTag)
+
+ valueTagIndexOpt match {
+ // If the field name exists in the inner elements,
+ // merge the type and infer the combined field as an array type if necessary
+ case Some(index) if !st(index).dataType.isInstanceOf[ArrayType] =>
+ updateStructField(
+ st,
+ index,
+ ArrayType(compatibleType(st(index).dataType, valueTagType)))
+ case Some(index) =>
+ updateStructField(st, index, compatibleType(st(index).dataType, valueTagType))
Review Comment:
Yes, this branch handles this case of array type. If it's an array, we will merge the element types.
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1437327958
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -201,27 +217,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (c.isWhiteSpace) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
+ case None => null
+ }
+ case _ =>
+ val row = convertObject(parser, st)
Review Comment:
Thanks for bringing this up! I added some test cases for comments. We still need this branch as`convertObject` cannot handle value 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
Re: [PR] [SPARK-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1431028302
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParserUtils.scala:
##########
@@ -70,7 +69,6 @@ object StaxXmlParserUtils {
/**
* Checks if current event points the EndElement.
*/
- @tailrec
Review Comment:
Good catch! Adding it back
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "sandip-db (via GitHub)" <gi...@apache.org>.
sandip-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1434410674
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -243,6 +244,22 @@ class XmlInferSchema(options: XmlOptions, caseSensitive: Boolean)
}
}
+ @tailrec
+ def inferAndCheckEndElement(parser: XMLEventReader): Boolean = {
+ parser.peek match {
+ case _: EndElement | _: EndDocument => true
+ case _: StartElement => false
+ case c: Characters if !c.isWhiteSpace =>
+ val characterType = inferFrom(c.getData)
+ parser.nextEvent()
+ addOrUpdateType(options.valueTag, characterType)
Review Comment:
Is there a test case for this scenario?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -201,27 +217,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (c.isWhiteSpace) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
+ case None => null
+ }
+ case _ =>
+ val row = convertObject(parser, st)
+ if (!c.isWhiteSpace) {
Review Comment:
We need to document the behavior of whitespaces for valueTag. Also, the following scenarios, which contain whitespaces with quotes:
```
<ROW><a>" "</a></ROW>
<ROW><b>" "<c>1</c></b></ROW>
<ROW><d><e attr=" "></e></d></ROW>
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -201,27 +217,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (c.isWhiteSpace) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
+ case None => null
+ }
+ case _ =>
+ val row = convertObject(parser, st)
Review Comment:
Will this handle values separated by comment or cdata? If so, we don't need `case _: EndElement` above.
```
<ROW>
<a> 1 <!--this is a comment--> 2 </a>
</ROW>
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -534,4 +541,55 @@ class XmlInferSchema(options: XmlOptions, caseSensitive: Boolean)
}
}
}
+
+ /**
+ * This helper function merges the data type of value tags and inner elements.
+ * It could only be structure data. Consider the following case,
+ * <a>
+ * value1
+ * <b>1</b>
+ * value2
+ * </a>
+ * Input: ''a struct<b int, _VALUE string>'' and ''_VALUE string''
+ * Return: ''a struct<b int, _VALUE array<string>>''
+ * @param objectType inner elements' type
+ * @param valueTagType value tag's type
+ */
+ private[xml] def addOrUpdateValueTagType(
+ objectType: DataType,
+ valueTagType: DataType): DataType = {
+ (objectType, valueTagType) match {
+ case (st: StructType, _) =>
+ // TODO(shujing): case sensitive?
Review Comment:
while the case for valueTag is unlikely to change, its better to add case sensitivity logic to it to make it consistent with other fields. Can be a separate PR. Not a high prio.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -534,4 +541,55 @@ class XmlInferSchema(options: XmlOptions, caseSensitive: Boolean)
}
}
}
+
+ /**
+ * This helper function merges the data type of value tags and inner elements.
+ * It could only be structure data. Consider the following case,
+ * <a>
+ * value1
+ * <b>1</b>
+ * value2
+ * </a>
+ * Input: ''a struct<b int, _VALUE string>'' and ''_VALUE string''
+ * Return: ''a struct<b int, _VALUE array<string>>''
+ * @param objectType inner elements' type
+ * @param valueTagType value tag's type
+ */
+ private[xml] def addOrUpdateValueTagType(
+ objectType: DataType,
+ valueTagType: DataType): DataType = {
+ (objectType, valueTagType) match {
+ case (st: StructType, _) =>
+ // TODO(shujing): case sensitive?
+ val valueTagIndexOpt = st.getFieldIndex(options.valueTag)
+
+ valueTagIndexOpt match {
+ // If the field name exists in the inner elements,
+ // merge the type and infer the combined field as an array type if necessary
+ case Some(index) if !st(index).dataType.isInstanceOf[ArrayType] =>
+ updateStructField(
+ st,
+ index,
+ ArrayType(compatibleType(st(index).dataType, valueTagType)))
+ case Some(index) =>
+ updateStructField(st, index, compatibleType(st(index).dataType, valueTagType))
Review Comment:
Won't `st(index).dataType` will be of `ArrayType`?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -534,4 +541,55 @@ class XmlInferSchema(options: XmlOptions, caseSensitive: Boolean)
}
}
}
+
+ /**
+ * This helper function merges the data type of value tags and inner elements.
+ * It could only be structure data. Consider the following case,
+ * <a>
+ * value1
+ * <b>1</b>
+ * value2
+ * </a>
+ * Input: ''a struct<b int, _VALUE string>'' and ''_VALUE string''
+ * Return: ''a struct<b int, _VALUE array<string>>''
+ * @param objectType inner elements' type
+ * @param valueTagType value tag's type
+ */
+ private[xml] def addOrUpdateValueTagType(
+ objectType: DataType,
+ valueTagType: DataType): DataType = {
+ (objectType, valueTagType) match {
+ case (st: StructType, _) =>
+ // TODO(shujing): case sensitive?
+ val valueTagIndexOpt = st.getFieldIndex(options.valueTag)
+
+ valueTagIndexOpt match {
+ // If the field name exists in the inner elements,
+ // merge the type and infer the combined field as an array type if necessary
+ case Some(index) if !st(index).dataType.isInstanceOf[ArrayType] =>
+ updateStructField(
+ st,
+ index,
+ ArrayType(compatibleType(st(index).dataType, valueTagType)))
+ case Some(index) =>
+ updateStructField(st, index, compatibleType(st(index).dataType, valueTagType))
Review Comment:
Lets add this test case scenario where `Array<LongType>` is updated to `Array<DoubleType>`:
```
<ROW>
<a>
1
<b>2</b>
3
<b>4</b>
5.0
</a>
</ROW>
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -201,27 +217,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (c.isWhiteSpace) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
+ case None => null
+ }
+ case _ =>
+ val row = convertObject(parser, st)
+ if (!c.isWhiteSpace) {
+ addOrUpdate(row.toSeq(st).toArray, st, options.valueTag, c.getData, addToTail = false)
Review Comment:
Why `addToTail` is false here?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -194,27 +210,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (isEmptyString(c)) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
Review Comment:
yes, I get that. It looks like the assumption is that `convertField` will either return a `Row` or a singleton valueTag with just value.
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1437189334
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -534,4 +541,55 @@ class XmlInferSchema(options: XmlOptions, caseSensitive: Boolean)
}
}
}
+
+ /**
+ * This helper function merges the data type of value tags and inner elements.
+ * It could only be structure data. Consider the following case,
+ * <a>
+ * value1
+ * <b>1</b>
+ * value2
+ * </a>
+ * Input: ''a struct<b int, _VALUE string>'' and ''_VALUE string''
+ * Return: ''a struct<b int, _VALUE array<string>>''
+ * @param objectType inner elements' type
+ * @param valueTagType value tag's type
+ */
+ private[xml] def addOrUpdateValueTagType(
+ objectType: DataType,
+ valueTagType: DataType): DataType = {
+ (objectType, valueTagType) match {
+ case (st: StructType, _) =>
+ // TODO(shujing): case sensitive?
Review Comment:
Thanks for answering this question! I create a Jira ticket for it
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1434536452
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -201,27 +217,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (c.isWhiteSpace) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
+ case None => null
+ }
+ case _ =>
+ val row = convertObject(parser, st)
+ if (!c.isWhiteSpace) {
+ addOrUpdate(row.toSeq(st).toArray, st, options.valueTag, c.getData, addToTail = false)
Review Comment:
This is because in this case, we encounter the interspersed value first and then the nested objects. We want to make sure that the value tag appears before the nested objects
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #44318:
URL: https://github.com/apache/spark/pull/44318#issuecomment-1871652553
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1430950463
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -194,27 +210,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (isEmptyString(c)) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
Review Comment:
`convertTo` converts primitive values based on its data type. It will not return a `Row`
--
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-46382][SQL] XML: Capture values interspersed between elements [spark]
Posted by "sandip-db (via GitHub)" <gi...@apache.org>.
sandip-db commented on code in PR #44318:
URL: https://github.com/apache/spark/pull/44318#discussion_r1427373383
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -567,6 +589,61 @@ class StaxXmlParser(
castTo(data, FloatType).asInstanceOf[Float]
}
}
+ private[xml] def isEmptyString(c: Characters): Boolean = {
+ if (options.ignoreSurroundingSpaces) {
+ c.getData.trim.isEmpty
+ } else {
+ c.isWhiteSpace
+ }
+ }
+
+ @tailrec
+ private def parseAndCheckEndElement(
+ row: Array[Any],
+ schema: StructType,
+ parser: XMLEventReader): Boolean = {
+ parser.peek match {
+ case _: EndElement | _: EndDocument => true
+ case _: StartElement => false
+ case c: Characters if !isEmptyString(c) =>
+ parser.nextEvent()
+ addOrUpdate(row, schema, options.valueTag, c.getData)
+ parseAndCheckEndElement(row, schema, parser)
+ case _ =>
+ parser.nextEvent()
+ parseAndCheckEndElement(row, schema, parser)
+ }
+ }
+
+ private def addOrUpdate(
+ row: Array[Any],
+ schema: StructType,
+ name: String,
+ string: String,
Review Comment:
```suggestion
data: String,
```
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -2178,4 +2166,49 @@ class XmlSuite extends QueryTest with SharedSparkSession {
)
testWriteReadRoundTrip(df, Map("nullValue" -> "null", "prefersDecimal" -> "true"))
}
+
+ test("capture values interspersed between elements - simple") {
+ val df = spark.read.format("xml")
+ .option("rowTag", "ROW")
+ .option("multiLine", "true")
+ .load(getTestResourcePath(resDir + "values-simple.xml"))
Review Comment:
Simple XML data can be embedded in test suite itself like: [using spark.createDataset](https://github.com/apache/spark/blob/c045a425bf0c472f164e3ef75a8a2c68d72d61d3/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala#L1537) or [writing to a temp file](https://github.com/apache/spark/blob/c045a425bf0c472f164e3ef75a8a2c68d72d61d3/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala#L2294)
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -255,7 +275,12 @@ class StaxXmlParser(
case e: StartElement =>
kvPairs +=
(UTF8String.fromString(StaxXmlParserUtils.getName(e.asStartElement.getName, options)) ->
- convertField(parser, valueType))
+ convertField(parser, valueType))
+ case c: Characters if !isEmptyString(c) =>
Review Comment:
```suggestion
case c: Characters if !c.isWhiteSpace =>
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -194,27 +210,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (isEmptyString(c)) {
Review Comment:
Lets not allow any whitespace values for `valueTag`.
```suggestion
if (!c.isWhiteSpace) {
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParserUtils.scala:
##########
@@ -70,7 +69,6 @@ object StaxXmlParserUtils {
/**
* Checks if current event points the EndElement.
*/
- @tailrec
Review Comment:
Why was this removed?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -171,16 +171,18 @@ class XmlInferSchema(options: XmlOptions, caseSensitive: Boolean)
case _: EndElement => StringType
case _ => inferField(parser)
}
- case c: Characters if !c.isWhiteSpace =>
+ // what about new line character
+ case c: Characters if !isEmptyString(c) =>
Review Comment:
For this case, can't we return `inferObject(parser)`?
In `inferObject(parser)`, the case for `StructType` can be updated to "unnest" `StructType` with just `valueTag`.
Without this, there is lot of code duplication logic for valueTag.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -194,27 +210,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (isEmptyString(c)) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
+ case None => null
+ }
+ case _ =>
+ val row = convertObject(parser, st)
+ if (!isEmptyString(c)) {
Review Comment:
```suggestion
if (!c.isWhiteSpace) {
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -194,27 +210,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (isEmptyString(c)) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
+ case None => null
+ }
+ case _ =>
+ val row = convertObject(parser, st)
+ if (!isEmptyString(c)) {
+ addOrUpdate(row.toSeq(st).toArray, st, options.valueTag, c.getData, addToTail = false)
+ } else {
+ row
+ }
}
case (_: Characters, _: StringType) =>
Review Comment:
Is `parser.next` not required here?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -567,6 +589,61 @@ class StaxXmlParser(
castTo(data, FloatType).asInstanceOf[Float]
}
}
+ private[xml] def isEmptyString(c: Characters): Boolean = {
+ if (options.ignoreSurroundingSpaces) {
+ c.getData.trim.isEmpty
Review Comment:
this is same as c.isWhiteSpace
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -1729,9 +1729,15 @@ class XmlSuite extends QueryTest with SharedSparkSession {
val TAG_NAME = "tag"
val VALUETAG_NAME = "_VALUE"
val schema = buildSchema(
+ field(VALUETAG_NAME),
Review Comment:
Why the fields were rearranged?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -194,27 +210,30 @@ class StaxXmlParser(
case (_: EndElement, _: DataType) => null
case (c: Characters, ArrayType(st, _)) =>
// For `ArrayType`, it needs to return the type of element. The values are merged later.
+ parser.next
convertTo(c.getData, st)
case (c: Characters, st: StructType) =>
- // If a value tag is present, this can be an attribute-only element whose values is in that
- // value tag field. Or, it can be a mixed-type element with both some character elements
- // and other complex structure. Character elements are ignored.
- val attributesOnly = st.fields.forall { f =>
- f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
- }
- if (attributesOnly) {
- // If everything else is an attribute column, there's no complex structure.
- // Just return the value of the character element, or null if we don't have a value tag
- st.find(_.name == options.valueTag).map(
- valueTag => convertTo(c.getData, valueTag.dataType)).orNull
- } else {
- // Otherwise, ignore this character element, and continue parsing the following complex
- // structure
- parser.next
- parser.peek match {
- case _: EndElement => null // no struct here at all; done
- case _ => convertObject(parser, st)
- }
+ parser.next
+ parser.peek match {
+ case _: EndElement =>
+ // It couldn't be an array of value tags
+ // as the opening tag is immediately followed by a closing tag.
+ if (isEmptyString(c)) {
+ return null
+ }
+ val indexOpt = getFieldNameToIndex(st).get(options.valueTag)
+ indexOpt match {
+ case Some(index) =>
+ convertTo(c.getData, st.fields(index).dataType)
Review Comment:
Shouldn't this be returning a `Row`? If yes, please make sure that there is a test scenario covering this.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -398,15 +424,11 @@ class StaxXmlParser(
badRecordException = badRecordException.orElse(Some(e))
}
- case c: Characters if !c.isWhiteSpace && isRootAttributesOnly =>
- nameToIndex.get(options.valueTag) match {
- case Some(index) =>
- row(index) = convertTo(c.getData, schema(index).dataType)
- case None => // do nothing
- }
+ case c: Characters if !isEmptyString(c) =>
Review Comment:
```suggestion
case c: Characters if !c.isWhiteSpace =>
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -159,7 +159,7 @@ class XmlInferSchema(options: XmlOptions, caseSensitive: Boolean)
parser.peek match {
case _: EndElement => NullType
case _: StartElement => inferObject(parser)
- case c: Characters if c.isWhiteSpace =>
+ case c: Characters if isEmptyString(c) =>
Review Comment:
```suggestion
case c: Characters if c.isWhiteSpace =>
```
##########
sql/core/src/test/resources/test-data/xml-resources/values-simple.xml:
##########
@@ -0,0 +1,11 @@
+<?xml version="1.0"?>
+<ROWSET>
+ <ROW>
+ value1
+ <a>
+ value2
+ <b>1</b>
+ value3
+ </a>
+ </ROW>
Review Comment:
```suggestion
</a>
value4
</ROW>
```
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -1145,18 +1145,18 @@ class XmlSuite extends QueryTest with SharedSparkSession {
.option("inferSchema", true)
.xml(getTestResourcePath(resDir + "mixed_children.xml"))
val mixedRow = mixedDF.head()
- assert(mixedRow.getAs[Row](0).toSeq === Seq(" lorem "))
- assert(mixedRow.getString(1) === " ipsum ")
+ assert(mixedRow.getAs[Row](0) === Row(List("issue", "text ignored"), "lorem"))
Review Comment:
Update `text ignored` with something else.
--
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