[SPARK-16318][SQL] Implement all remaining xpath functions by petermaxlee · Pull Request #13991 · apache/spark

petermaxlee · 2016-06-30T06:48:10Z

What changes were proposed in this pull request?

This patch implements all remaining xpath functions that Hive supports and not natively supported in Spark: xpath_int, xpath_short, xpath_long, xpath_float, xpath_double, xpath_string, and xpath.

How was this patch tested?

Added unit tests and end-to-end tests.

petermaxlee · 2016-06-30T06:48:51Z

Can you guys take a look at this? @cloud-fan @dongjoon-hyun @gatorsmile

SparkQA · 2016-06-30T08:54:45Z

Test build #61530 has finished for PR 13991 at commit 66fae41.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2016-06-30T14:43:59Z

seems the boolean and string one doesn't share same implementation?(xpathUtil.evalBoolean and xpathUtil.evalString)

gatorsmile · 2016-06-30T17:17:38Z

Could you add some test cases for the boundary values? That means, trying the maximum and minimum values for each different data type. Also, the values that are out of the boundary.

gatorsmile · 2016-06-30T17:20:42Z

Which Exception will we throw? What is the message we issue? When users see the message, can they understand the cause?

gatorsmile · 2016-06-30T17:23:38Z

Could you add more test cases for corrupted xml and illegal inputs? So far, the test suite XPathExpressionSuite only covers the basic cases.

gatorsmile · 2016-06-30T17:28:58Z

Normally, for each function or data type we implement in an enterprise product, we need to document the limit/restriction. That is why I am asking for the boundary values, negative cases and error handling.

cloud-fan · 2016-07-01T00:01:14Z

+1 for @gatorsmile 's advices

petermaxlee · 2016-07-06T06:52:52Z

Thanks for the feedback. I will update this later.

rxin · 2016-07-08T00:52:35Z

For this one I think we should consider supporting only foldable literals for the path component. It's probably satisfy 99.999% of the use cases, and simplify the code.

rxin · 2016-07-08T01:22:26Z

Also - rather than having concrete implementations for all of these, why don't we use RuntimeReplaceable?

SparkQA · 2016-07-08T02:59:09Z

Test build #61947 has finished for PR 13991 at commit 2d48ae5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

petermaxlee · 2016-07-08T03:47:28Z

I pushed a new change to this. We now have better error messages and test coverage for those. These expressions also now require foldable paths.

I also changed the test values to make sure we are not restricting the range of values an expression can return (e.g. for XPathInt the test case makes sure it can return a value larger than short). However, I don't think it'd make sense to test overflow behavior because those are mostly undefined throughout Spark SQL.

Re: @rxin's suggestion on using RuntimeReplaceable -- I did try that, but I'm afraid the current implementation of RuntimeReplaceable isn't meant for stacking multiple expressions together, and type checking actually breaks if XPathInt replaces itself with Cast(XPathDouble). I looked into fixing that, but it seemed more complicated and it doesn't actually save much space (it actually takes more code to use RuntimeReplaceable).

SparkQA · 2016-07-08T05:15:46Z

Test build #61953 has finished for PR 13991 at commit d30d891.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-08T05:18:30Z

Test build #61954 has finished for PR 13991 at commit 02df488.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

petermaxlee · 2016-07-08T05:30:36Z

I just added the general xpath function that returns an array of string too.

SparkQA · 2016-07-08T07:12:49Z

Test build #61957 has finished for PR 13991 at commit d7d5f8d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class XPathList(xml: Expression, path: Expression) extends XPathExtract

SparkQA · 2016-07-08T07:18:32Z

Test build #61958 has finished for PR 13991 at commit 48311bb.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-08T08:21:52Z

Test build #61967 has finished for PR 13991 at commit a86bc04.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-08T09:41:52Z

Test build #61975 has finished for PR 13991 at commit 24deac9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

petermaxlee · 2016-07-08T18:32:14Z

@hvanhovell can you take a look at this?

SparkQA · 2016-07-08T20:51:35Z

Test build #61992 has finished for PR 13991 at commit fecc293.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-08T23:49:26Z

Test build #62000 has finished for PR 13991 at commit 0c60d87.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class ParseUrl(children: Seq[Expression])

SparkQA · 2016-07-09T23:12:56Z

Test build #3175 has finished for PR 13991 at commit 0c60d87.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- case class ParseUrl(children: Seq[Expression])

cloud-fan · 2016-07-11T03:16:06Z

retest this please

cloud-fan · 2016-07-11T03:16:14Z

LGTM, pending jenkins

petermaxlee · 2016-07-11T04:17:13Z

@cloud-fan Jenkins already ran twice successfully before.

cloud-fan · 2016-07-11T04:47:27Z

The latest result is This patch does not merge cleanly., I just wanna double check it.

petermaxlee · 2016-07-11T04:51:02Z

I guess whatever generates that message is buggy?

SparkQA · 2016-07-11T05:02:30Z

Test build #62070 has finished for PR 13991 at commit 0c60d87.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class ParseUrl(children: Seq[Expression])

yhuai · 2016-07-11T05:22:31Z

As a follow-up task. Can you take a look at the following query files and add useful tests in your test? Thanks.

.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/describe_xpath.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/input_testxpath.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/input_testxpath2.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/input_testxpath3.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/input_testxpath4.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/udf_xpath.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/udf_xpath_boolean.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/udf_xpath_double.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/udf_xpath_float.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/udf_xpath_int.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/udf_xpath_long.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/udf_xpath_short.q
.//sql/hive/src/test/resources/ql/src/test/queries/clientpositive/udf_xpath_string.q

petermaxlee · 2016-07-11T05:24:00Z

Actually I created the unit tests based on those.

petermaxlee · 2016-07-11T05:24:49Z

BTW I have to say Hive's test coverage in this area is very spotty, so I don't actually think it's great to follow, but I used those.

yhuai · 2016-07-11T05:28:54Z

OK. Thanks. Then, it will be good to add more tests for cases that are not covered by those hive tests.

cloud-fan · 2016-07-11T05:30:54Z

Thanks, merging to master! This doesn't merge clearly to 2.0, @petermaxlee can you submit a new PR against 2.0? thanks!

petermaxlee · 2016-07-11T05:32:59Z

@cloud-fan thanks for merging!

@yhuai I think the degree to which we want to add more tests also depend on how much we trust the library we are using. XPath (with Query) is almost as complicated as SQL itself.

This patch implements all remaining xpath functions that Hive supports and not natively supported in Spark: xpath_int, xpath_short, xpath_long, xpath_float, xpath_double, xpath_string, and xpath. Added unit tests and end-to-end tests. Author: petermaxlee <petermaxlee@gmail.com> Closesapache#13991 from petermaxlee/SPARK-16318.

petermaxlee · 2016-07-11T05:41:29Z

Here it is for branch-2.0.
#14131

## What changes were proposed in this pull request? This patch implements all remaining xpath functions that Hive supports and not natively supported in Spark: xpath_int, xpath_short, xpath_long, xpath_float, xpath_double, xpath_string, and xpath. This is based on #13991 but for branch-2.0. ## How was this patch tested? Added unit tests and end-to-end tests. Author: petermaxlee <petermaxlee@gmail.com> Closes#14131 from petermaxlee/xpath-branch-2.0.

cloud-fan reviewed Jun 30, 2016
View reviewed changes

gatorsmile reviewed Jun 30, 2016
View reviewed changes

petermaxlee force-pushed the SPARK-16318 branch from 66fae41 to 2d48ae5Compare July 8, 2016 00:49

petermaxlee changed the title ~~[SPARK-16318][SQL] Implement various xpath functions~~[SPARK-16318][SQL] Implement all remaining xpath functionsJul 8, 2016

petermaxlee added 7 commits July 8, 2016 11:35

[SPARK-16318][SQL] Implement various xpath functions
39e0d0a

Better error handling, test coverage.
4908840

Shorten test cases
ad3d851

Added xpath (list)
8fa67c8

end-to-end tests
2f36ccd

Remove RuntimeReplaceable
9ac6397

Fix test
fecc293

petermaxlee force-pushed the SPARK-16318 branch from 24deac9 to fecc293Compare July 8, 2016 18:36

Merge remote-tracking branch 'apache/master' into SPARK-16318
0c60d87

asfgit closed this in 82f0874Jul 11, 2016

petermaxlee mentioned this pull request Jul 11, 2016
[SPARK-16318][SQL] Implement all remaining xpath functions (branch-2.0) #14131
Closed

Uh oh!

Conversation

petermaxlee commented Jun 30, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

petermaxlee commented Jun 30, 2016

Uh oh!

SparkQA commented Jun 30, 2016

Uh oh!

cloud-fanJun 30, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Jun 30, 2016

Uh oh!

gatorsmileJun 30, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Jun 30, 2016

Uh oh!

gatorsmile commented Jun 30, 2016

Uh oh!

cloud-fan commented Jul 1, 2016

Uh oh!

petermaxlee commented Jul 6, 2016

Uh oh!

rxin commented Jul 8, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxin commented Jul 8, 2016

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

petermaxlee commented Jul 8, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

petermaxlee commented Jul 8, 2016

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

petermaxlee commented Jul 8, 2016

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

SparkQA commented Jul 8, 2016

Uh oh!

SparkQA commented Jul 9, 2016

Uh oh!

cloud-fan commented Jul 11, 2016

Uh oh!

cloud-fan commented Jul 11, 2016

Uh oh!

petermaxlee commented Jul 11, 2016

Uh oh!

cloud-fan commented Jul 11, 2016

Uh oh!

petermaxlee commented Jul 11, 2016

Uh oh!

SparkQA commented Jul 11, 2016

Uh oh!

yhuai commented Jul 11, 2016

Uh oh!

petermaxlee commented Jul 11, 2016

Uh oh!

petermaxlee commented Jul 11, 2016

Uh oh!

yhuai commented Jul 11, 2016

Uh oh!

cloud-fan commented Jul 11, 2016

petermaxlee commented Jun 30, 2016•
edited
Loading

rxin commented Jul 8, 2016•
edited
Loading

petermaxlee commented Jul 8, 2016•
edited
Loading