comphead commented on code in PR #7392:
URL: https://github.com/apache/datafusion-comet/pull/7392#discussion_r4460103857
##########
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:
`scheme.is_none() &&` is redundant, since `decompose_uri` only reports a
scheme when the `:` is at an index above 0, so `value.starts_with(':')` is
enough. In the second check, `file` is the path plus any `?query`, so
`file.is_empty() && fragment.is_none()` says the same thing. I read this from
`decompose_uri` and have not run it.
##########
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:
`://missing-scheme` is already covered by
`test_malformed_with_scheme_separator` and by the ANSI and
`parse_url_enabled.sql` fixtures. `:foo`, `//` and `http://` are in the new SQL
table, and `:foo` and `http://` are also in the ANSI file. Both new branches
return before `part` is read, so the loop over four parts checks the same early
return four times. Most rows of `test_relative_reference` repeat the SQL table
too, which is compared with Spark directly. Could the Rust tests keep only what
the SQL files do not cover?
##########
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:
The other tables in this file include a `(NULL)` row, but this one has none.
Could you add `(NULL)` and `('')`? The empty string returns before
`decompose_uri` (`PATH` and `FILE` give `''`, the rest NULL), so it is a
separate branch right next to the new code.
--
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]
