Skip to content

feat: add support parse_url#3563

Open
rafafrdz wants to merge 14 commits intoapache:mainfrom
rafafrdz:feat/add-support-parse_url
Open

feat: add support parse_url#3563
rafafrdz wants to merge 14 commits intoapache:mainfrom
rafafrdz:feat/add-support-parse_url

Conversation

@rafafrdz
Copy link
Copy Markdown
Contributor

@rafafrdz rafafrdz commented Feb 21, 2026 •
edited
Loading

Summary

  • Added support for Spark parse_url by mapping ParseUrl to the native scalar function parse_url in expression serde.
  • Updated spark_expressions_support.md to mark parse_url as supported.
  • Add tests

Why
This closes one of the missing DataFusion 50 migration functions from issue #2443

Part of #2443

@rafafrdz rafafrdz marked this pull request as draft February 21, 2026 10:22
@rafafrdz rafafrdz marked this pull request as ready for review February 21, 2026 11:05
@mbutrovich
Copy link
Copy Markdown
Contributor

mbutrovich commented Feb 23, 2026

Thanks @rafafrdz! Kicking off CI.

@andygrove
Copy link
Copy Markdown
Member

andygrove commented Feb 23, 2026

This looks good so far @rafafrdz.

CI shows all three parse_url tests failing on Spark 4.0 with COMET: invoke is not supported. This happens because ParseUrl was changed to RuntimeReplaceable in Spark 4.0. It gets replaced by an Invoke expression during analysis, so Comet's serde never sees the ParseUrl node.

On the compatibility side, I noticed that the PR doesn't distinguish between ANSI and legacy mode for invalid URL handling. DataFusion's parse_url throws errors on malformed URLs, which matches Spark's behavior when failOnError=true (ANSI mode), but in legacy mode (failOnError=false), Spark returns null for invalid URLs instead of throwing.

@rafafrdz
Copy link
Copy Markdown
Contributor Author

rafafrdz commented Feb 24, 2026

Thanks for the suggestion @andygrove

What I did:

  • Spark 4.0 RuntimeReplaceable / Invoke path: ParseUrl is now handled when Spark rewrites it to Invoke(ParseUrlEvaluator.evaluate, ...), so Comet serde no longer misses it and we avoid "the invoke is not supported" fallback (please check it out if it's correct or not)
  • ANSI vs legacy behavior: serde now respects failOnError:
    • failOnError = true (ANSI) -> parse_url (error on malformed URLs)
    • failOnError = false (legacy) -> try_parse_url (returns NULL on malformed URLs)

I also registered try_parse_url on the native side and added a test for invalid URL handling in legacy mode (Spark 4.0 path).

@rafafrdz rafafrdz marked this pull request as draft February 26, 2026 10:59
@rafafrdz rafafrdz force-pushed the feat/add-support-parse_url branch from 7c8a8d9 to 9d7720f Compare March 4, 2026 15:01
@rafafrdz rafafrdz marked this pull request as ready for review March 4, 2026 15:02
andygrove
andygrove reviewed Mar 10, 2026
View reviewed changes
Comment thread spark/src/test/resources/sql-tests/expressions/string/parse_url.sql Outdated
-- under the License.

-- ConfigMatrix: parquet.enable.dictionary=false,true
-- MinSparkVersion: 3.5
Copy link
Copy Markdown
Member

@andygrove andygrove Mar 10, 2026

Choose a reason for hiding this comment

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

Does Spark 3.4 not support parse_url?

Copy link
Copy Markdown
Contributor Author

@rafafrdz rafafrdz Mar 19, 2026

Choose a reason for hiding this comment

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

my mistake, done

andygrove
andygrove reviewed Mar 10, 2026
View reviewed changes
Comment thread spark/src/test/resources/sql-tests/expressions/string/parse_url.sql
SELECT parse_url(url, 'AUTHORITY') FROM test_parse_url

query
SELECT parse_url(url, 'USERINFO') FROM test_parse_url
Copy link
Copy Markdown
Member

@andygrove andygrove Mar 10, 2026

Choose a reason for hiding this comment

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

Could you also add a literal arguments query to the SQL file, since literal and column inputs can use different code paths? Something like:

SELECT parse_url('http://spark.apache.org/path?q=1', 'HOST')

Copy link
Copy Markdown
Contributor Author

@rafafrdz rafafrdz Mar 19, 2026

Choose a reason for hiding this comment

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

done

andygrove
andygrove reviewed Mar 10, 2026
View reviewed changes
Comment thread spark/src/test/scala/org/apache/comet/CometStringExpressionSuite.scala Outdated
}

test("parse_url with invalid URL in legacy mode") {
assume(isSpark40Plus)
Copy link
Copy Markdown
Member

@andygrove andygrove Mar 10, 2026

Choose a reason for hiding this comment

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

Could you add a comment explaining why this test is specific to Spark 4?

Copy link
Copy Markdown
Contributor Author

@rafafrdz rafafrdz Mar 19, 2026

Choose a reason for hiding this comment

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

my mistake, drop assumed part

andygrove
andygrove reviewed Mar 10, 2026
View reviewed changes
Comment thread spark/src/test/scala/org/apache/comet/CometStringExpressionSuite.scala Outdated
}

test("parse_url in ANSI mode (Spark 3.5)") {
assume(!isSpark40Plus)
Copy link
Copy Markdown
Member

@andygrove andygrove Mar 10, 2026

Choose a reason for hiding this comment

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

Why does this test exclude Spark 4?

Copy link
Copy Markdown
Contributor Author

@rafafrdz rafafrdz Mar 19, 2026

Choose a reason for hiding this comment

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

dropped it

@andygrove andygrove marked this pull request as draft March 16, 2026 13:31
@andygrove
Copy link
Copy Markdown
Member

andygrove commented Mar 16, 2026

Moving this to draft until feedback is addressed

rafafrdz added 9 commits March 19, 2026 11:48
@rafafrdz
add support parse_url
db21fce
@rafafrdz
edit
9121c6f
@rafafrdz
fix parse_url Spark 4 Invoke rewrite and legacy null semantics
3167045
@rafafrdz
edit
891b851
@rafafrdz
chore: apply spotless formatting
59d38cd
@rafafrdz
test: fix parse_url test suite
60be29e 
- Remove try_parse_url SQL queries: try_parse_url is a Comet-internal
  DataFusion function name used when serializing parse_url with
  failOnError=false. It is not a registered Spark SQL function and
  calling it directly via SQL raises UNRESOLVED_ROUTINE on any Spark
  version. The NULL-on-invalid-URL behaviour is covered by the
  'parse_url with invalid URL in legacy mode' Scala test.
- Replace ftp port 21 with 2121 in test URLs: the Rust url crate omits
  well-known default ports (ftp=21) when serialising authority(), while
  Java URI.getRawAuthority() preserves them verbatim. Using a
  non-default port avoids this pre-existing semantic gap and keeps the
  AUTHORITY test case meaningful on both Spark 3.5 and 4.0.
@rafafrdz
refactor: introduce CometInvokeExpressionSerde trait for generic Invo…
ab5b727 
…ke dispatch
@rafafrdz
refactor: simplify CometInvokeExpressionSerde by removing type parameter
433aa3d 
Use Expression directly instead of a generic type parameter T <: Expression.
This eliminates the asInstanceOf cast in convertFromInvokeUnchecked and the
unchecked method itself, since QueryPlanSerde can now call convertFromInvoke
directly without existential type issues.
@rafafrdz
suggestions
54fc48a
@rafafrdz rafafrdz force-pushed the feat/add-support-parse_url branch from 3b07a93 to 54fc48a Compare March 19, 2026 11:17
@rafafrdz rafafrdz marked this pull request as ready for review March 19, 2026 11:17
@rafafrdz rafafrdz requested a review from andygrove March 19, 2026 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andygrove andygrove Awaiting requested review from andygrove

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rafafrdz @mbutrovich @andygrove