Skip to content

[SPARK-47345][SQL][TESTS] Xml functions suite#45466

Closed
yhosny wants to merge 4 commits intoapache:masterfrom
yhosny:xml-functions-suite
Closed

[SPARK-47345][SQL][TESTS] Xml functions suite#45466
yhosny wants to merge 4 commits intoapache:masterfrom
yhosny:xml-functions-suite

Conversation

@yhosny
Copy link
Contributor

@yhosny yhosny commented Mar 11, 2024

What changes were proposed in this pull request?

Convert JsonFunctiosnSuite.scala to XML equivalent. Note that XML doesn’t implement all json functions like json_tuple, get_json_object, etc.

Why are the changes needed?

Improve unit test coverage.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit tests.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label Mar 11, 2024
@zhengruifeng zhengruifeng changed the title [SPARK-47345] [SQL]: Xml functions suite [SPARK-47345][SQL][TESTS] Xml functions suite Mar 12, 2024
@zhengruifeng
Copy link
Contributor

add [TESTS] since it only adds new tests

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the test failure Run / Build modules: pyspark-connect is not related to the new test suite.

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except of minor comments.

test("infers schemas using options") {
val df = spark.range(1)
.select(schema_of_xml(lit("<ROW><a>1</a></ROW>"),
Map("allowUnquotedFieldNames" -> "true").asJava))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, fix indentation here.

class XmlFunctionsSuite extends QueryTest with SharedSparkSession {
import testImplicits._


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this line

@MaxGekk
Copy link
Member

MaxGekk commented Mar 15, 2024

I have checked the new test suite locally:

$ build/sbt "sql/test:testOnly *XmlFunctionsSuite"
[info] XmlFunctionsSuite:
22:50:44.233 WARN org.apache.hadoop.util.NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable
[info] - from_xml (5 seconds, 317 milliseconds)
[info] - from_xml with option (timestampFormat) (382 milliseconds)
[info] - from_xml with option (rowTag) (153 milliseconds)
[info] - from_xml with option (dateFormat) (180 milliseconds)
...
[info] All tests passed.

+1, LGTM. Merging to master.
Thank you, @yhosny and @zhengruifeng for review.

@MaxGekk MaxGekk closed this in 7c81bdf Mar 15, 2024
)
}

test("schema_of_xml - infers the schema of foldable JSON string") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON -> XML

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a followup: #45577

dongjoon-hyun pushed a commit that referenced this pull request Mar 19, 2024
…ctionsSuite

### What changes were proposed in this pull request?

This PR is a followup of #45466 that addresses #45466 (comment). It renames a test name from JSON to XML within `XmlFunctionsSuite`

### Why are the changes needed?

To have the correct test title so we don't get confused why/which test failed.

### Does this PR introduce _any_ user-facing change?

No, test-only.

### How was this patch tested?

CI in this PR should validate the change.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #45577 from HyukjinKwon/SPARK-47345-followup.

Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants