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/10/10 19:12:00 UTC
[PR] [SPARK-45488] XML: Add support for value in 'rowTag' element [spark]
shujingyang-db opened a new pull request, #43319:
URL: https://github.com/apache/spark/pull/43319
<!--
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.
-->
The following XML with rowTag 'book' will yield a schema with just "_id" column and not the value:
<p><book id="1">Great Book</book> </p>
Let's parse value as well.
### 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.
-->
The semantics for attributes and `valueTag` should be consistent
### 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
### 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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1355372857
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -122,7 +122,12 @@ class StaxXmlParser(
}
val parser = StaxXmlParserUtils.filteredReader(xml)
val rootAttributes = StaxXmlParserUtils.gatherRootAttributes(parser)
- Some(convertObject(parser, schema, options, rootAttributes))
+ // A structure object is an attribute-only element
+ // if it only consists of attributes and valueTags.
+ val isRootAttributesOnly = schema.fields.forall { f =>
+ f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
+ }
+ Some(convertObject(parser, schema, options, rootAttributes, isRootAttributesOnly))
Review Comment:
We only read values if the struct only consists of attributes and `valueTag`. If there's another field, the value in between the tags will be ignored.
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1355375216
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -219,11 +219,27 @@ private[sql] object XmlInferSchema {
dataTypes += inferredType
nameToDataType += (field -> dataTypes)
+ case c: Characters if !c.isWhiteSpace =>
+ // This can be an attribute-only object
+ val valueTagType = inferFrom(c.getData, options)
+ nameToDataType += options.valueTag -> ArrayBuffer(valueTagType)
Review Comment:
IMHO it will make the loop much more complicated as we need to examine the case you mentioned above
```
<ROW>4<tag>5</tag></ROW>
<ROW><tag>6</tag>7</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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on PR #43319:
URL: https://github.com/apache/spark/pull/43319#issuecomment-1757750260
For what it's worth, this is the reason I didn't implement this a while ago:
https://github.com/databricks/spark-xml/issues/428#issuecomment-565562921
It's not wrong to try to support this but I think there are more corner cases this raises that aren't addressed here - what about `<book>Great <foo>Book</foo> to read!</book>` ? what would be the schema and what goes into `_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-45488][SQL] XML: Add support for value in 'rowTag' element [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43319:
URL: https://github.com/apache/spark/pull/43319#issuecomment-1759065426
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1355376309
##########
sql/core/src/test/resources/test-data/xml-resources/root-level-value.xml:
##########
@@ -0,0 +1,9 @@
+<?xml version="1.0"?>
+<ROWSET>
+ <ROW>value1</ROW>
+ <ROW attr="attr1">value2</ROW>
+ <ROW>
+ value3
+ <!-- this is a comment-->
+ </ROW>
+</ROWSET>
Review Comment:
Sure I will add another test case for this. In this case, `5` and `6` will also be skipped. We only read values if the struct only consists of attributes and `valueTag`. If there's another field (i.e. `tag`), the value in between the tags will be ignored.
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "sandip-db (via GitHub)" <gi...@apache.org>.
sandip-db commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1354166034
##########
sql/core/src/test/resources/test-data/xml-resources/root-level-value.xml:
##########
@@ -0,0 +1,9 @@
+<?xml version="1.0"?>
+<ROWSET>
+ <ROW>value1</ROW>
+ <ROW attr="attr1">value2</ROW>
+ <ROW>
+ value3
+ <!-- this is a comment-->
+ </ROW>
+</ROWSET>
Review Comment:
pls add and test with the following ROW:
```
<ROW>4<tag>5</tag></ROW>
<ROW><tag>6</tag>7</ROW>
<ROW attr="8"></ROW>
```
4 and 7 should be skipped.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -219,11 +219,27 @@ private[sql] object XmlInferSchema {
dataTypes += inferredType
nameToDataType += (field -> dataTypes)
+ case c: Characters if !c.isWhiteSpace =>
+ // This can be an attribute-only object
+ val valueTagType = inferFrom(c.getData, options)
+ nameToDataType += options.valueTag -> ArrayBuffer(valueTagType)
+
case _: EndElement =>
shouldStop = StaxXmlParserUtils.checkEndElement(parser)
case _ => // do nothing
}
+ }
+ // A structure object is an attribute-only element
+ // if it only consists of attributes and valueTags.
+ // If not, we will remove the valueTag field from the schema
+ val attributesOnly = nameToDataType.forall {
+ case (fieldName, dataTypes) =>
+ dataTypes.length == 1 &&
+ (fieldName == options.valueTag || fieldName.startsWith(options.attributePrefix))
Review Comment:
This is fragile as an element name can begin with the `options.attributePrefix`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -219,11 +219,27 @@ private[sql] object XmlInferSchema {
dataTypes += inferredType
nameToDataType += (field -> dataTypes)
+ case c: Characters if !c.isWhiteSpace =>
+ // This can be an attribute-only object
+ val valueTagType = inferFrom(c.getData, options)
+ nameToDataType += options.valueTag -> ArrayBuffer(valueTagType)
Review Comment:
Can this be added outside the `while` loop after examining that no child elements were present?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala:
##########
@@ -122,7 +122,12 @@ class StaxXmlParser(
}
val parser = StaxXmlParserUtils.filteredReader(xml)
val rootAttributes = StaxXmlParserUtils.gatherRootAttributes(parser)
- Some(convertObject(parser, schema, options, rootAttributes))
+ // A structure object is an attribute-only element
+ // if it only consists of attributes and valueTags.
+ val isRootAttributesOnly = schema.fields.forall { f =>
+ f.name == options.valueTag || f.name.startsWith(options.attributePrefix)
+ }
+ Some(convertObject(parser, schema, options, rootAttributes, isRootAttributesOnly))
Review Comment:
Couldn't we just pass `true` instead of `isRootAttributesOnly` here? There is a check for `nameToIndex.get(options.valueTag)` below anyway.
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1355376309
##########
sql/core/src/test/resources/test-data/xml-resources/root-level-value.xml:
##########
@@ -0,0 +1,9 @@
+<?xml version="1.0"?>
+<ROWSET>
+ <ROW>value1</ROW>
+ <ROW attr="attr1">value2</ROW>
+ <ROW>
+ value3
+ <!-- this is a comment-->
+ </ROW>
+</ROWSET>
Review Comment:
Sure I will add another test case for this. In this case, `5` and `6` will also be skipped. We only read values if the struct only consists of attributes and `valueTag`. If there's another field (i.e. `tag`), the value in between the tags will be ignored.
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1354143438
##########
sql/core/src/test/resources/test-data/xml-resources/root-level-value.xml:
##########
@@ -0,0 +1,9 @@
+<?xml version="1.0"?>
+<ROWSET>
+ <ROW>value1</ROW>
+ <ROW attr="attr1">value2</ROW>
+ <ROW>
+ value3
+ <!-- this is a comment-->
+ </ROW>
+</ROWSET>
Review Comment:
Sure. I can do it in a follow-up PR.
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1354192165
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -219,11 +219,27 @@ private[sql] object XmlInferSchema {
dataTypes += inferredType
nameToDataType += (field -> dataTypes)
+ case c: Characters if !c.isWhiteSpace =>
+ // This can be an attribute-only object
+ val valueTagType = inferFrom(c.getData, options)
+ nameToDataType += options.valueTag -> ArrayBuffer(valueTagType)
+
case _: EndElement =>
shouldStop = StaxXmlParserUtils.checkEndElement(parser)
case _ => // do nothing
}
+ }
+ // A structure object is an attribute-only element
+ // if it only consists of attributes and valueTags.
+ // If not, we will remove the valueTag field from the schema
+ val attributesOnly = nameToDataType.forall {
+ case (fieldName, dataTypes) =>
Review Comment:
We use `2-space` indentation. Could you double-check around this line?
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43319:
URL: https://github.com/apache/spark/pull/43319#issuecomment-1756730028
cc @sandip-db if you find some time to 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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1355401700
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -219,11 +219,27 @@ private[sql] object XmlInferSchema {
dataTypes += inferredType
nameToDataType += (field -> dataTypes)
+ case c: Characters if !c.isWhiteSpace =>
+ // This can be an attribute-only object
+ val valueTagType = inferFrom(c.getData, options)
+ nameToDataType += options.valueTag -> ArrayBuffer(valueTagType)
+
case _: EndElement =>
shouldStop = StaxXmlParserUtils.checkEndElement(parser)
case _ => // do nothing
}
+ }
+ // A structure object is an attribute-only element
+ // if it only consists of attributes and valueTags.
+ // If not, we will remove the valueTag field from the schema
+ val attributesOnly = nameToDataType.forall {
+ case (fieldName, dataTypes) =>
+ dataTypes.length == 1 &&
+ (fieldName == options.valueTag || fieldName.startsWith(options.attributePrefix))
Review Comment:
I agree on it. However this is our current [behavior](https://github.com/shujingyang-db/spark/blob/8fd10a40641c831155ffd644e331f0b818f72700/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala#L198), we might need to keep aligned with 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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1353981938
##########
sql/core/src/test/resources/test-data/xml-resources/root-level-value.xml:
##########
@@ -0,0 +1,9 @@
+<?xml version="1.0"?>
+<ROWSET>
+ <ROW>value1</ROW>
+ <ROW attr="attr1">value2</ROW>
+ <ROW>
+ value3
+ <!-- this is a comment-->
+ </ROW>
+</ROWSET>
Review Comment:
If possible, I would prefer put this XML into a string variable within `XmlSuite.scala` to make the test easier to read, and self-contained.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -1645,4 +1645,43 @@ class XmlSuite extends QueryTest with SharedSparkSession {
.xml(getTestResourcePath(resDir + "fias_house.xml"))
assert(df.collect().length === 37)
}
+
+ test("root-level value tag for attributes-only object") {
+
+ val schema = buildSchema(field("_attr"), field("_VALUE"))
+ val results = Seq(
+ // user specified schema
+ spark.read
+ .schema(schema)
+ .xml(getTestResourcePath(resDir + "root-level-value.xml")).collect(),
+ // schema inference
+ spark.read
+ .xml(getTestResourcePath(resDir + "root-level-value.xml")).collect())
+ results.foreach { result =>
+ assert(result.length === 3)
+ assert(result(0).getAs[String]("_VALUE") == "value1")
+ assert(result(1).getAs[String]("_attr") == "attr1"
+ && result(1).getAs[String]("_VALUE") == "value2")
+ // comments aren't included in valueTag
+ assert(result(2).getAs[String]("_VALUE") == "\n value3\n ")
+ }
+ }
+
+ test("root-level value tag for attributes-only object - from xml") {
+ val xmlData =
+ s"""
Review Comment:
```suggestion
"""
```
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -1645,4 +1645,43 @@ class XmlSuite extends QueryTest with SharedSparkSession {
.xml(getTestResourcePath(resDir + "fias_house.xml"))
assert(df.collect().length === 37)
}
+
+ test("root-level value tag for attributes-only object") {
+
+ val schema = buildSchema(field("_attr"), field("_VALUE"))
+ val results = Seq(
+ // user specified schema
+ spark.read
+ .schema(schema)
+ .xml(getTestResourcePath(resDir + "root-level-value.xml")).collect(),
+ // schema inference
+ spark.read
+ .xml(getTestResourcePath(resDir + "root-level-value.xml")).collect())
+ results.foreach { result =>
+ assert(result.length === 3)
+ assert(result(0).getAs[String]("_VALUE") == "value1")
+ assert(result(1).getAs[String]("_attr") == "attr1"
+ && result(1).getAs[String]("_VALUE") == "value2")
+ // comments aren't included in valueTag
+ assert(result(2).getAs[String]("_VALUE") == "\n value3\n ")
+ }
+ }
+
+ test("root-level value tag for attributes-only object - from xml") {
Review Comment:
```suggestion
test("SPARK-45488: root-level value tag for attributes-only object - from xml") {
```
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -1645,4 +1645,43 @@ class XmlSuite extends QueryTest with SharedSparkSession {
.xml(getTestResourcePath(resDir + "fias_house.xml"))
assert(df.collect().length === 37)
}
+
+ test("root-level value tag for attributes-only object") {
Review Comment:
```suggestion
test("SPARK-45488: root-level value tag for attributes-only object") {
```
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1354189568
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/XmlInferSchema.scala:
##########
@@ -219,11 +219,27 @@ private[sql] object XmlInferSchema {
dataTypes += inferredType
nameToDataType += (field -> dataTypes)
+ case c: Characters if !c.isWhiteSpace =>
+ // This can be an attribute-only object
+ val valueTagType = inferFrom(c.getData, options)
+ nameToDataType += options.valueTag -> ArrayBuffer(valueTagType)
+
case _: EndElement =>
shouldStop = StaxXmlParserUtils.checkEndElement(parser)
case _ => // do nothing
}
+ }
+ // A structure object is an attribute-only element
+ // if it only consists of attributes and valueTags.
Review Comment:
Indentation for the above two lines?
--
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-45488][SQL] XML: Add support for value in 'rowTag' element [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1356022084
##########
sql/core/src/test/resources/test-data/xml-resources/root-level-value.xml:
##########
@@ -0,0 +1,9 @@
+<?xml version="1.0"?>
+<ROWSET>
+ <ROW>value1</ROW>
+ <ROW attr="attr1">value2</ROW>
+ <ROW>
+ value3
+ <!-- this is a comment-->
+ </ROW>
+</ROWSET>
Review Comment:
Added a test `root-level value tag for not attributes-only object`👍
##########
sql/core/src/test/resources/test-data/xml-resources/root-level-value.xml:
##########
@@ -0,0 +1,9 @@
+<?xml version="1.0"?>
+<ROWSET>
+ <ROW>value1</ROW>
+ <ROW attr="attr1">value2</ROW>
+ <ROW>
+ value3
+ <!-- this is a comment-->
+ </ROW>
+</ROWSET>
Review Comment:
Added a test `root-level value tag for not attributes-only object`👍
--
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-45488][SQL] XML: Add support for value in 'rowTag' element [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #43319: [SPARK-45488][SQL] XML: Add support for value in 'rowTag' element
URL: https://github.com/apache/spark/pull/43319
--
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-45488][SQL] XML: Add support for value in 'rowTag' element [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43319:
URL: https://github.com/apache/spark/pull/43319#issuecomment-1758848151
The test failures look unrelated but mind retriggering https://github.com/shujingyang-db/spark/runs/17621262726 again?
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on PR #43319:
URL: https://github.com/apache/spark/pull/43319#issuecomment-1758122788
@srowen Thanks for bringing up this case! In the case of
```
<book>Great <foo>Book</foo> to read!</book>
```
If `rowTag` is `book`, the resulting schema will be `foo STRING` and skip the characters `Great` and ` to read!` in between the tags. This is because we only read values in between tags if the struct _only_ consists of attributes and valueTag. Our definition to the `valueTag` is:
> valueTag: The tag used for the value when there are attributes in the element having no child. Default is _VALUE.
If there's another field, the value in between the tags will be ignored.
If the user specifies the schema to be `foo STRING, _VALUE STRING`, we will also ignore the value and leave `_VALUE` empty. ([current behavior](https://github.com/shujingyang-db/spark/blob/8fd10a40641c831155ffd644e331f0b818f72700/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/xml/StaxXmlParser.scala#L198))
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "shujingyang-db (via GitHub)" <gi...@apache.org>.
shujingyang-db commented on PR #43319:
URL: https://github.com/apache/spark/pull/43319#issuecomment-1756866256
@dongjoon-hyun Thanks for the heads-up! I activated the GitHub Action resource and am retrying the tests.
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1354201319
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -1645,4 +1645,43 @@ class XmlSuite extends QueryTest with SharedSparkSession {
.xml(getTestResourcePath(resDir + "fias_house.xml"))
assert(df.collect().length === 37)
}
+
+ test("SPARK-45488: root-level value tag for attributes-only object") {
+
+ val schema = buildSchema(field("_attr"), field("_VALUE"))
+ val results = Seq(
+ // user specified schema
+ spark.read
+ .schema(schema)
+ .xml(getTestResourcePath(resDir + "root-level-value.xml")).collect(),
+ // schema inference
+ spark.read
+ .xml(getTestResourcePath(resDir + "root-level-value.xml")).collect())
+ results.foreach { result =>
+ assert(result.length === 3)
+ assert(result(0).getAs[String]("_VALUE") == "value1")
+ assert(result(1).getAs[String]("_attr") == "attr1"
+ && result(1).getAs[String]("_VALUE") == "value2")
+ // comments aren't included in valueTag
+ assert(result(2).getAs[String]("_VALUE") == "\n value3\n ")
+ }
+ }
+
+ test("SPARK-45488: root-level value tag for attributes-only object - from xml") {
+ val xmlData =
+ """
+ |<ROW attr="attr1">123456</ROW>
+ |""".stripMargin
Review Comment:
One-liner will be better in this case.
```scala
val xmlData = """<ROW attr="attr1">123456</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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43319:
URL: https://github.com/apache/spark/pull/43319#discussion_r1354193492
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/XmlSuite.scala:
##########
@@ -1645,4 +1645,43 @@ class XmlSuite extends QueryTest with SharedSparkSession {
.xml(getTestResourcePath(resDir + "fias_house.xml"))
assert(df.collect().length === 37)
}
+
+ test("SPARK-45488: root-level value tag for attributes-only object") {
+
Review Comment:
Shall we remove this empty line?
--
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-45488] XML: Add support for value in 'rowTag' element [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #43319:
URL: https://github.com/apache/spark/pull/43319#issuecomment-1756919011
Thank you for updating. I have minor comments in addition to Hyukjin's comment.
--
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