[SPARK-50031][SQL] Add the `TryParseUrl` expression #48500

jovanm-db · 2024-10-16T15:30:00Z

What changes were proposed in this pull request?

This PR adds try_parse_url expression that sets failOnError to false by default.

Why are the changes needed?

INVALID_URL contains suggested fix for turning off ANSI mode. Now that in Spark 4.0.0 we have moved to ANSI mode on by default, we want to keep suggestions of this kind to the minimum. There exist implementations of try_* functions which provide safe way to get behavior as for ANSI mode off and suggestions of this kind should be sufficient.

In this case, try expressions were missing so new expressions were added to patch up the missing implementations.

Does this PR introduce any user-facing change?

Yes, new expression added.

How was this patch tested?

Tests added.

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

No.

mihailom-db

Great work Jovan, please address the comments I left and we can move this PR out of draft phase.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/urlExpressions.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

common/utils/src/main/resources/error/error-conditions.json

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

mihailom-db

LGTM, just one more nit comment

common/utils/src/main/resources/error/error-conditions.json

Co-authored-by: Mihailo Milosevic <[email protected]>

MaxGekk · 2024-10-16T21:41:48Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/urlExpressions.scala

+      > SELECT _FUNC_('http://spark.apache.org/path?query=1', 'HOST');
+       spark.apache.org
+      > SELECT _FUNC_('http://spark.apache.org/path?query=1', 'QUERY');
+       query=1
+      > SELECT _FUNC_('http://spark.apache.org/path?query=1', 'QUERY', 'query');
+       1


Can you show an example of the NULL result?

HyukjinKwon · 2024-10-17T00:31:24Z

python/pyspark/sql/functions/builtin.py

@@ -12912,6 +12912,108 @@ def substr(
        return _invoke_function_over_columns("substr", str, pos)


+@_try_remote_functions


can we add a one unittest in, e.g., test_functions.py? then the tests will be resued in both Spark connect and classic.

dongjoon-hyun

Hi, @jovanm-db .

IIUC, the PR title is misleading because the main contribution will be introducing a new built-in function try_parse_url at Apache Spark 4.0.0. Improve error messages for ParseURL looks like a side-effect to me.

The PR (and the commit) had better have one theme. I'd like to recommend to remove the change of common/utils/src/main/resources/error/error-conditions.json file from this PR and you can make another PR to handle it independently.

mihailom-db · 2024-10-17T14:58:13Z

@dongjoon-hyun I do agree that the title is not reflecting the right change, but opening a new PR might be an overkill and also cause confusion in development later. The main point of this PR is what ticket suggests Remove the ANSI config suggestion in INVALID_URL. I would suggest renaming the title, but keeping the changes in one PR, as these changes suggest the reason for removing and also the solution/substitute suggestion we provide for spark users. Separating them would make developer work much harder, and the second PR you suggested would be mostly tests changes.

dongjoon-hyun · 2024-10-17T15:04:23Z

@dongjoon-hyun I do agree that the title is not reflecting the right change, but opening a new PR might be an overkill and also cause confusion in development later. The main point of this PR is what ticket suggests Remove the ANSI config suggestion in INVALID_URL. I would suggest renaming the title, but keeping the changes in one PR, as these changes suggest the reason for removing and also the solution/substitute suggestion we provide for spark users. Separating them would make developer work much harder, and the second PR you suggested would be mostly tests changes.

No, I disagree with your opinion, "opening a new PR might be an overkill and also cause confusion in development later", @jovanm-db . Technically, adding try_parse_url and recommending try_parse_url is not the same at all. They should have two JIRA IDs. :)

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/urlExpressions.scala

Stale

mihailom-db

LGTM, please address the comments so we can merge this in

mihailom-db · 2024-10-21T10:52:26Z

python/pyspark/sql/functions/builtin.py

+    url: "ColumnOrName", partToExtract: "ColumnOrName", key: Optional["ColumnOrName"] = None
+) -> Column:
+    """
+    URL function: Extracts a specified part from a URL. If a key is provided,


Could you update this description to give the similar message as other try functions?

mihailom-db · 2024-10-21T10:53:59Z

python/pyspark/sql/tests/test_functions.py

@@ -333,6 +333,14 @@ def test_rand_functions(self):
        rndn2 = df.select("key", F.randn(0)).collect()
        self.assertEqual(sorted(rndn1), sorted(rndn2))

+    def test_try_parse_url(self):


Could we also add a case here that returns null? This is to make sure python works as well with invalid urls.

MaxGekk · 2024-10-21T16:48:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/urlExpressions.scala

@@ -169,6 +169,37 @@ object ParseUrl {
  private val REGEXSUBFIX = "=([^&]*)"
 }

+/**
+ * Extracts a part from a URL


This comment is useless. Let's remove it.

MaxGekk · 2024-10-21T16:49:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/urlExpressions.scala

+ * Extracts a part from a URL
+ */
+@ExpressionDescription(
+  usage = "_FUNC_(url, partToExtract[, key]) - Extracts a part from a URL.",


ParseUrl has the same description. Could write a few words what destiguish new expression from ParseUrl.

MaxGekk · 2024-10-22T13:02:38Z

@jovanm-db Please, update PR's description according the recent changes in the PR.

This PR improves messages for ANSI related issues for ParseUrl.
new suggestions are added to do try_parse_url.

MaxGekk · 2024-10-22T15:44:56Z

+1, LGTM. Merging to master.
Thank you, @jovanm-db and @dongjoon-hyun @HyukjinKwon @srielau @mihailom-db for review.

MaxGekk · 2024-10-22T15:47:01Z

@jovanm-db Congratulations with your first contribution to Apache Spark!

dongjoon-hyun · 2024-10-22T17:57:45Z

Thank you, @jovanm-db and @MaxGekk and all.

Congratulations to @jovanm-db .

### What changes were proposed in this pull request? This PR adds `try_parse_url` expression that sets `failOnError` to false by default. ### Why are the changes needed? INVALID_URL contains suggested fix for turning off ANSI mode. Now that in Spark 4.0.0 we have moved to ANSI mode on by default, we want to keep suggestions of this kind to the minimum. There exist implementations of `try_*` functions which provide safe way to get behavior as for ANSI mode off and suggestions of this kind should be sufficient. In this case, try expressions were missing so new expressions were added to patch up the missing implementations. ### Does this PR introduce _any_ user-facing change? Yes, new expression added. ### How was this patch tested? Tests added. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48500 from jovanm-db/invalidUrl. Authored-by: Jovan Markovic <[email protected]> Signed-off-by: Max Gekk <[email protected]>

### What changes were proposed in this pull request? This PR updates the suggested fix of `INVALID_URL` error to use `try_parse_url` function added in [this](#48500) PR instead of turning off ANSI mode. ### Why are the changes needed? INVALID_URL contains suggested fix for turning off ANSI mode. Now that in Spark 4.0.0 we have moved to ANSI mode on by default, we want to keep suggestions of this kind to the minimum. There exist implementations of try_* functions which provide safe way to get behavior as for ANSI mode off and suggestions of this kind should be sufficient. In this case, try expressions were missing so new expressions were added to patch up the missing implementations. ### Does this PR introduce _any_ user-facing change? Yes. ### How was this patch tested? There are tests that check error messages. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #48616 from jovanm-db/improvedError. Authored-by: Jovan Markovic <[email protected]> Signed-off-by: Max Gekk <[email protected]>

…nnect module ### What changes were proposed in this pull request? Add tests in `PlanGenerationTestSuite` for `try_parse_url` function introduced in [this](#48500) PR and generate the corresponding golden files. ### Why are the changes needed? The added tests ensure the new function behaves as expected. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Tests added. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #48637 from jovanm-db/goldenFiles. Authored-by: Jovan Markovic <[email protected]> Signed-off-by: Max Gekk <[email protected]>

Add try_parse_url expression

4656fbd

github-actions bot added SQL DOCS PYTHON CONNECT labels Oct 16, 2024

mihailom-db reviewed Oct 16, 2024

View reviewed changes

Fix errors

fbc201b

mihailom-db approved these changes Oct 16, 2024

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Outdated Show resolved Hide resolved

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala Outdated Show resolved Hide resolved

Fix formatting

f49745a

mihailom-db approved these changes Oct 16, 2024

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Outdated Show resolved Hide resolved

Fix formatting

5793bff

Co-authored-by: Mihailo Milosevic <[email protected]>

jovanm-db changed the title ~~[WIP][SPARK-49638][SQL] Improve error messages for ParseUrl #49638~~ [SPARK-49638][SQL] Improve error messages for ParseUrl Oct 16, 2024

srielau approved these changes Oct 16, 2024

View reviewed changes

MaxGekk reviewed Oct 16, 2024

View reviewed changes

HyukjinKwon reviewed Oct 17, 2024

View reviewed changes

HyukjinKwon approved these changes Oct 17, 2024

View reviewed changes

Add tests

3122ac6

dongjoon-hyun previously requested changes Oct 17, 2024

View reviewed changes

Fix test and formatting

a2a1e8c

jovanm-db changed the title ~~[SPARK-49638][SQL] Improve error messages for ParseUrl~~ [SPARK-50031][SQL] Add TryParseAlternative Oct 18, 2024

jovanm-db changed the title ~~[SPARK-50031][SQL] Add TryParseAlternative~~ [SPARK-50031][SQL] Add TryParseUrl alternative Oct 18, 2024

Revert changes and split tasks into a new ticket

5071226

MaxGekk requested changes Oct 20, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/urlExpressions.scala Show resolved Hide resolved

Merge branch 'apache:master' into invalidUrl

470f446

mihailom-db approved these changes Oct 21, 2024

View reviewed changes

jovanm-db added 2 commits October 21, 2024 13:09

Fix test and docs

9f2ccc6

Fix docs

3513600

MaxGekk requested changes Oct 21, 2024

View reviewed changes

jovanm-db added 2 commits October 22, 2024 11:25

Fix docs

6656eaa

Fix lint

5434b30

mihailom-db approved these changes Oct 22, 2024

View reviewed changes

MaxGekk approved these changes Oct 22, 2024

View reviewed changes

MaxGekk changed the title ~~[SPARK-50031][SQL] Add TryParseUrl alternative~~ [SPARK-50031][SQL] Add the TryParseUrl expression Oct 22, 2024

MaxGekk closed this in abc4986 Oct 22, 2024

jovanm-db mentioned this pull request Oct 23, 2024

[SPARK-49638][SQL] Remove the ANSI config suggestion in INVALID_URL #48616

Closed

jovanm-db mentioned this pull request Oct 24, 2024

[SPARK-50031][SQL][FOLLOW-UP] Add tests for try_parse_url in the connect module #48637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-50031][SQL] Add the `TryParseUrl` expression #48500

[SPARK-50031][SQL] Add the `TryParseUrl` expression #48500

jovanm-db commented Oct 16, 2024 •

edited

Loading

mihailom-db left a comment

mihailom-db left a comment

MaxGekk Oct 16, 2024

HyukjinKwon Oct 17, 2024

dongjoon-hyun left a comment •

edited

Loading

mihailom-db commented Oct 17, 2024

dongjoon-hyun commented Oct 17, 2024 •

edited

Loading

mihailom-db left a comment

mihailom-db Oct 21, 2024

mihailom-db Oct 21, 2024

MaxGekk Oct 21, 2024

MaxGekk Oct 21, 2024

MaxGekk commented Oct 22, 2024 •

edited

Loading

MaxGekk commented Oct 22, 2024

MaxGekk commented Oct 22, 2024

dongjoon-hyun commented Oct 22, 2024

		@@ -12912,6 +12912,108 @@ def substr(
		return _invoke_function_over_columns("substr", str, pos)


		@_try_remote_functions

[SPARK-50031][SQL] Add the TryParseUrl expression #48500

[SPARK-50031][SQL] Add the TryParseUrl expression #48500

Conversation

jovanm-db commented Oct 16, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

mihailom-db left a comment

Choose a reason for hiding this comment

mihailom-db left a comment

Choose a reason for hiding this comment

MaxGekk Oct 16, 2024

Choose a reason for hiding this comment

HyukjinKwon Oct 17, 2024

Choose a reason for hiding this comment

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

mihailom-db commented Oct 17, 2024

dongjoon-hyun commented Oct 17, 2024 • edited Loading

mihailom-db left a comment

Choose a reason for hiding this comment

mihailom-db Oct 21, 2024

Choose a reason for hiding this comment

mihailom-db Oct 21, 2024

Choose a reason for hiding this comment

MaxGekk Oct 21, 2024

Choose a reason for hiding this comment

MaxGekk Oct 21, 2024

Choose a reason for hiding this comment

MaxGekk commented Oct 22, 2024 • edited Loading

MaxGekk commented Oct 22, 2024

MaxGekk commented Oct 22, 2024

dongjoon-hyun commented Oct 22, 2024

[SPARK-50031][SQL] Add the `TryParseUrl` expression #48500

[SPARK-50031][SQL] Add the `TryParseUrl` expression #48500

jovanm-db commented Oct 16, 2024 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

dongjoon-hyun commented Oct 17, 2024 •

edited

Loading

MaxGekk commented Oct 22, 2024 •

edited

Loading