valentinsoe commented on code in PR #7392:
URL: https://github.com/apache/datafusion-comet/pull/7392#discussion_r4471630330
##########
native/spark-expr/src/url_funcs/parse_url.rs:
##########
@@ -181,11 +181,16 @@ fn parse_url_component<'a>(
fragment,
} = decompose_uri(value);
- if scheme.is_none() {
- if value.contains("://") {
- return Err(invalid_url_err(value));
- }
- return Ok(None);
+ // A value without a scheme is a relative reference; a leading ':' is an
empty
+ // scheme name, which java.net.URI rejects ("Expected scheme name at index
0").
+ if scheme.is_none() && value.starts_with(':') {
+ return Err(invalid_url_err(value));
+ }
+
+ // java.net.URI rejects an empty authority ("//", "http://";) unless a path,
+ // query or fragment follows it ("Expected authority").
+ if authority == Some("") && path.is_empty() && query.is_none() &&
fragment.is_none() {
+ return Err(invalid_url_err(value));
}
Review Comment:
Done, both simplified as you suggested.
##########
spark/src/test/resources/sql-tests/expressions/url/parse_url.sql:
##########
@@ -170,3 +170,33 @@ SELECT parse_url('http://host:abc/', 'AUTHORITY')
-- unbalanced IPv6 bracket is invalid
query
SELECT parse_url('http://[::1/path', 'AUTHORITY')
+
+-- relative references (no scheme) are parsed like java.net.URI
+statement
+CREATE TABLE test_parse_url_relative(url string) USING parquet
+
+statement
+INSERT INTO test_parse_url_relative VALUES
+ ('/'),
+ ('notaurl'),
+ ('a/b:c?id=1'),
+ ('?id=7'),
+ ('#frag'),
+ ('/p#a?id=1'),
+ ('/shop/item/123?ref=abc&id=42#top'),
+ ('/r?to=http://x.com/a&id=1'),
+ ('//example.com'),
+ ('//user:[email protected]:8080/p?id=1#f'),
+ ('///p?id=3'),
+ ('//#f'),
+ ('//?id=4'),
+ (':foo'),
+ ('//'),
+ ('http://'),
+ ('http://example.com/r?to=https://y.com&id=9')
Review Comment:
Added `('')` and `(NULL)`, and `(':')` next to `(':foo')`.
##########
native/spark-expr/src/url_funcs/parse_url.rs:
##########
@@ -795,4 +800,52 @@ mod tests {
let result = parse_url_component("http://[::1/path";, "HOST", None);
assert!(result.is_err());
}
+
+ #[test]
+ fn test_relative_reference() -> Result<()> {
+ // Expected values are from Spark 4.1.1 (java.net.URI).
+ for (url, part, key, expected) in [
+ ("/shop?ref=abc&id=42#top", "PATH", None, Some("/shop")),
+ ("/shop?ref=abc&id=42#top", "QUERY", Some("id"), Some("42")),
+ ("/shop?ref=abc&id=42#top", "REF", None, Some("top")),
+ ("/shop?ref=abc&id=42#top", "HOST", None, None),
+ ("?id=7", "FILE", None, Some("?id=7")),
+ ("notaurl", "PATH", None, Some("notaurl")),
+ ("a/b:c?id=1", "PATH", None, Some("a/b:c")),
+ (
+ "//user:[email protected]:8080/p",
+ "HOST",
+ None,
+ Some("example.com"),
+ ),
+ (
+ "//user:[email protected]:8080/p",
+ "USERINFO",
+ None,
+ Some("user:pw"),
+ ),
+ ("///p?id=3", "AUTHORITY", None, None),
+ ("//?id=4", "QUERY", None, Some("id=4")),
+ ("/r?to=http://x.com/a&id=1";, "QUERY", Some("id"), Some("1")),
+ ] {
+ assert_eq!(
+ parse_url_component(url, part, key)?,
+ expected,
+ "{url} {part}"
+ );
+ }
+ Ok(())
+ }
+
+ #[test]
+ fn test_invalid_relative_reference() {
+ for url in [":", ":foo", "://missing-scheme", "//", "http://";] {
+ for part in ["HOST", "PATH", "PROTOCOL", "FILE"] {
+ assert!(
+ parse_url_component(url, part, None).is_err(),
+ "{url} {part}"
+ );
+ }
+ }
+ }
Review Comment:
Removed both. The SQL files cover those cases and compare with Spark
directly. I kept one small Rust test, `test_invisible_chars`, for characters
that are hard to see in a SQL file (DEL, NBSP, NEL, U+2028).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
