Use parse_url kernel for PROTOCOL parsing #9481

thirtiseven · 2023-10-19T06:31:17Z

Contributes to #8963

This PR add plugin support for PROTOCOL parsing in parse_url.

Related JNI PR: NVIDIA/spark-rapids-jni#1502

Most of the code is reused from the old PR #8761

Performance test:

spark.time(df.selectExpr("COUNT(parse_url(url, 'PROTOCOL')) as pr1", "COUNT(parse_url(url, 'PROTOCOL')) as pr2", "COUNT(parse_url(url, 'PROTOCOL')) as pr3", "COUNT(parse_url(url, 'PROTOCOL')) as pr4", "COUNT(parse_url(url, 'PROTOCOL')) as pr5", "COUNT(parse_url(url, 'PROTOCOL')) as pr6", "COUNT(parse_url(url, 'PROTOCOL')) as pr7", "COUNT(parse_url(url, 'PROTOCOL')) as pr8", "COUNT(parse_url(url, 'PROTOCOL')) as pr9", "COUNT(parse_url(url, 'PROTOCOL')) as pr0").show())

against 651191 urls' dataset, repeated 100 times.

CPU: 10752 ms
GPU: 1032 ms

It's about 10x faster

Signed-off-by: Haoyang Li <[email protected]>

docs/compatibility.md

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala

tests/src/test/scala/com/nvidia/spark/rapids/UrlFunctionsSuite.scala

Signed-off-by: Haoyang Li <[email protected]>

jlowe · 2023-10-23T15:18:28Z

integration_tests/src/main/python/url_test.py

+                ),
+            'ParseUrl')
+
+def test_parse_url_too_many_args():


This seems like a test of Spark itself rather than the plugin? IIUC this error is going to be thrown not because the plugin throws it but during the query analysis which occurs before the plugin runs.

Yes you are right! I removed this case and related unnecessary error handling code.

jlowe · 2023-10-23T15:19:17Z

integration_tests/src/main/python/url_test.py

+def test_parse_url_protocol():
+    assert_gpu_and_cpu_are_equal_collect(
+            lambda spark : unary_op_df(spark, url_gen).selectExpr(
+                "a",
+                "parse_url(a, 'PROTOCOL')"
+                ))
+
+def test_parse_url_protocol_edge_cases():
+    assert_gpu_and_cpu_are_equal_collect(
+            lambda spark : unary_op_df(spark, edge_cases_gen).selectExpr(
+                "a",
+                "parse_url(a, 'PROTOCOL')"
+                ))


Nit: Parameterize this test on the data generation rather than duplicate it

jlowe · 2023-10-23T15:20:44Z

integration_tests/src/main/python/url_test.py

+@allow_non_gpu('ProjectExec', 'ParseUrl')
+def test_parse_url_host_fallback():
+    assert_gpu_fallback_collect(
+            lambda spark : unary_op_df(spark, url_gen).selectExpr(
+                "a",
+                "parse_url(a, 'HOST')"
+                ),
+            'ParseUrl')
+
+@allow_non_gpu('ProjectExec', 'ParseUrl')
+def test_parse_url_path_fallback():
+    assert_gpu_fallback_collect(
+            lambda spark : unary_op_df(spark, url_gen).selectExpr(
+                "a",
+                "parse_url(a, 'PATH')"
+                ),
+            'ParseUrl')
+
+@allow_non_gpu('ProjectExec', 'ParseUrl')
+def test_parse_url_query_fallback():
+    assert_gpu_fallback_collect(
+            lambda spark : unary_op_df(spark, url_gen).selectExpr(
+                "a",
+                "parse_url(a, 'QUERY')"
+                ),
+            'ParseUrl')
+
+@allow_non_gpu('ProjectExec', 'ParseUrl')   
+def test_parse_url_ref_fallback():
+    assert_gpu_fallback_collect(
+            lambda spark : unary_op_df(spark, url_gen).selectExpr(
+                "a",
+                "parse_url(a, 'REF')"
+                ),
+            'ParseUrl')


all of these should be refactored to a single test parameterized on the literal rather than duplicated.

jlowe · 2023-10-23T15:21:59Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala

+          ParamCheck("partToExtract", TypeSig.lit(TypeEnum.STRING), TypeSig.STRING)),
+          // Should really be an OptionalParam


This should have a withPsNote denoting that it only supports PROTOCOL so that limitation shows up in the documentation. Otherwise the documentation implies we fully support this as long as a literal is supplied.

jlowe · 2023-10-23T15:22:39Z

integration_tests/src/main/python/url_test.py

+    "http://foo.com/blah_blah_(wikipedia)_(again)",
+    "http://www.example.com/wpstyle/?p=364",
+    "https://www.example.com/foo/?bar=baz&inga=42&quux",
+    # "http://✪df.ws/123",


The failing cases need to be uncommented and passing before this should be merged.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/urlFunctions.scala

Signed-off-by: Haoyang Li <[email protected]>

thirtiseven · 2023-11-16T10:16:28Z

Depends on NVIDIA/spark-rapids-jni#1554

Signed-off-by: Haoyang Li <[email protected]>

hyperbolic2346

I think the tests can be enabled now. Question about URL vs URI, but I'm fine either way.

docs/additional-functionality/advanced_configs.md

docs/compatibility.md

Signed-off-by: Haoyang Li <[email protected]>

thirtiseven · 2023-11-28T08:32:05Z

Thanks all for the review! I think all comments have been addressed and it's ready to merge once jni PR is merged.

sameerz · 2023-12-01T16:22:51Z

Please retarget to 24.02.

thirtiseven · 2023-12-05T02:59:52Z

Verified locally that all protocol tests passed after NVIDIA/spark-rapids-jni#1554 merged, both for IT and the kaggle dataset.

thirtiseven · 2023-12-06T07:59:23Z

build

Signed-off-by: Haoyang Li <[email protected]>

thirtiseven · 2023-12-06T08:42:11Z

build

thirtiseven · 2023-12-12T02:13:58Z

Hi @revans2 @jlowe Could you please take another look at this?

revans2 · 2023-12-12T19:49:56Z

@hyperbolic2346 cold you also take a look as your change request is blocking this from going in.

@thirtiseven I think @jlowe is out on vacation so you probably should not wait for him to come back.

On a side note, this is crazy fast!

I ran

spark.time(spark.range(0, 1000000000L, 1, 48).selectExpr("CONCAT('htpp://', CAST(id as STRING)) as u").selectExpr("parse_url(u, 'PROTOCOL') as p").selectExpr("MAX(p)", "MIN(p)").show())

both on the GPU and the CPU and the GPU ended up being 452x faster than 12 CPU cores. This is a crappy test that is not real world at all, but still. That is really fast.

hyperbolic2346

I'm fine with this, but I am not a java/scala reviewer.

revans2 · 2023-12-12T20:46:40Z

I'm fine with this, but I am not a java/scala reviewer.

I get that, but I just don't like to use my admin merge powers to push a red merge button unless I have to.

thirtiseven and others added 20 commits July 20, 2023 13:11

WIP: Support parse_url

8235c95

Signed-off-by: Haoyang Li <[email protected]>

Merge branch 'NVIDIA:branch-23.08' into prase_url

9f17539

fix build failures

729fe35

regex refactor

85c3284

Merge branch 'NVIDIA:branch-23.08' into prase_url

6819214

Separate regexes and UTF-8 special characters support

4166362

hostname validation

43acceb

hostname validation

64d8373

ipv4 validation

e6a45d3

verify

8c4dc7a

wip ipv6 and SPARK-44500

fee5a3d

optional protocol and ref validation

e81d8a3

IPV6 VALIDATION

93a9342

clean up

1ad665f

Fix ipv6 validation, it is still wip

3edb929

Fix ipv6 validation and some clean up

daa61ea

Merge branch 'prase_url' into parse_url_protocol

70a5d88

Use parse_url kernel for PROTOCOL parsing

b3abaf6

Signed-off-by: Haoyang Li <[email protected]>

verify

592c642

Signed-off-by: Haoyang Li <[email protected]>

edit compatibility and update IT

9db1b2a

Signed-off-by: Haoyang Li <[email protected]>

jlowe reviewed Oct 19, 2023

View reviewed changes

docs/compatibility.md Outdated Show resolved Hide resolved

jlowe reviewed Oct 19, 2023

View reviewed changes

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala Show resolved Hide resolved

tests/src/test/scala/com/nvidia/spark/rapids/UrlFunctionsSuite.scala Outdated Show resolved Hide resolved

update integration tests

d09f06d

Signed-off-by: Haoyang Li <[email protected]>

sameerz added the feature request New feature or request label Oct 23, 2023

jlowe reviewed Oct 23, 2023

View reviewed changes

jlowe mentioned this pull request Oct 23, 2023

[FEA] Support parse_url function #8512

Closed

revans2 reviewed Oct 23, 2023

View reviewed changes

hyperbolic2346 mentioned this pull request Oct 23, 2023

Adding IP6 characters into valid character list for URI parsing NVIDIA/spark-rapids-jni#1516

Closed

thirtiseven added 2 commits October 24, 2023 13:55

address comments

3b71c4d

Signed-off-by: Haoyang Li <[email protected]>

remove unnecessary error handling

46527f3

Signed-off-by: Haoyang Li <[email protected]>

Fix two nits

3376376

Signed-off-by: Haoyang Li <[email protected]>

hyperbolic2346 requested changes Nov 21, 2023

View reviewed changes

docs/additional-functionality/advanced_configs.md Show resolved Hide resolved

docs/compatibility.md Outdated Show resolved Hide resolved

thirtiseven added 2 commits November 22, 2023 09:49

Updated results

4e98888

Signed-off-by: Haoyang Li <[email protected]>

clean up

6d916c4

Signed-off-by: Haoyang Li <[email protected]>

thirtiseven self-assigned this Nov 22, 2023

thirtiseven marked this pull request as ready for review November 22, 2023 07:20

thirtiseven mentioned this pull request Nov 23, 2023

Use parse_url kernel for HOST parsing #9845

Merged

rename urlFunctions to GpuParseUrl

1b36090

Signed-off-by: Haoyang Li <[email protected]>

Merge branch 'branch-23.12' into parse_url_protocol

7eca922

thirtiseven changed the base branch from branch-23.12 to branch-24.02 December 1, 2023 16:49

Merge branch 'NVIDIA:branch-24.02' into parse_url_protocol

e4fdf13

thirtiseven requested a review from hyperbolic2346 December 5, 2023 01:56

verify

3ace124

Signed-off-by: Haoyang Li <[email protected]>

hyperbolic2346 requested review from jlowe and revans2 December 6, 2023 18:45

revans2 approved these changes Dec 12, 2023

View reviewed changes

hyperbolic2346 approved these changes Dec 12, 2023

View reviewed changes

revans2 merged commit c6dee9b into NVIDIA:branch-24.02 Dec 12, 2023
38 checks passed

thirtiseven deleted the parse_url_protocol branch December 19, 2023 08:45

This was referenced Apr 11, 2024

Merge branch-24.04 into main NvTimLiu/spark-rapids-jni#6

Merged

Update latest changelog [skip ci] #10683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use parse_url kernel for PROTOCOL parsing #9481

Use parse_url kernel for PROTOCOL parsing #9481

thirtiseven commented Oct 19, 2023 •

edited

Loading

jlowe Oct 23, 2023

thirtiseven Oct 24, 2023

jlowe Oct 23, 2023

thirtiseven Dec 1, 2023

jlowe Oct 23, 2023

thirtiseven Dec 1, 2023

jlowe Oct 23, 2023

thirtiseven Dec 1, 2023

jlowe Oct 23, 2023

thirtiseven Dec 1, 2023

thirtiseven commented Nov 16, 2023

hyperbolic2346 left a comment

thirtiseven commented Nov 28, 2023

sameerz commented Dec 1, 2023

thirtiseven commented Dec 5, 2023

thirtiseven commented Dec 6, 2023

thirtiseven commented Dec 6, 2023

thirtiseven commented Dec 12, 2023

revans2 commented Dec 12, 2023

hyperbolic2346 left a comment •

edited

Loading

revans2 commented Dec 12, 2023

		ParamCheck("partToExtract", TypeSig.lit(TypeEnum.STRING), TypeSig.STRING)),
		// Should really be an OptionalParam

Use parse_url kernel for PROTOCOL parsing #9481

Use parse_url kernel for PROTOCOL parsing #9481

Conversation

thirtiseven commented Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thirtiseven commented Nov 16, 2023

hyperbolic2346 left a comment

Choose a reason for hiding this comment

thirtiseven commented Nov 28, 2023

sameerz commented Dec 1, 2023

thirtiseven commented Dec 5, 2023

thirtiseven commented Dec 6, 2023

thirtiseven commented Dec 6, 2023

thirtiseven commented Dec 12, 2023

revans2 commented Dec 12, 2023

hyperbolic2346 left a comment • edited Loading

Choose a reason for hiding this comment

revans2 commented Dec 12, 2023

thirtiseven commented Oct 19, 2023 •

edited

Loading

hyperbolic2346 left a comment •

edited

Loading