viirya commented on code in PR #7329:
URL: https://github.com/apache/datafusion-comet/pull/7329#discussion_r4421649643
##########
native/spark-expr/src/url_funcs/parse_url.rs:
##########
@@ -388,11 +449,100 @@ mod tests {
// Unit tests for parse_url_component
// -----------------------------------------------------------------------
+ #[test]
+ fn http_decomposition_matches_generic_parser() {
+ for scheme in ["http", "https"] {
+ for authority in [
+ "example.com",
+ "u:p@host:80",
+ "",
+ "[::1]:80",
+ "[::1",
+ "host:",
+ "例子.com",
+ ] {
+ for tail in [
+ "",
+ "/",
+ "//a/b",
+ "?",
+ "#",
+ "?#",
+ "?x=1#f?y",
+ "#f?x=1",
+ "/é?k=值#片",
+ "/p?a=?b#f#x",
+ ] {
+ let url = format!("{scheme}://{authority}{tail}");
+ assert_eq!(decompose_http(&url), decompose_generic(&url),
"{url}");
+ }
+ }
+ }
+ for url in ["HTTP://host/p", "https:/p", "ftp://host/p";, "mailto:a@b";]
{
+ assert!(decompose_http(url).is_none(), "{url}");
+ }
+ }
+
+ #[test]
+ fn file_and_query_components() -> Result<()> {
+ for (url, part, key, expected) in [
+ ("http://h?x=1#f?y";, "FILE", None, "?x=1"),
+ ("http://h?#";, "FILE", None, "?"),
+ ("HTTP://h/p?#f", "FILE", None, "/p?"),
+ ("ftp://h/p?x=1#f";, "FILE", None, "/p?x=1"),
+ ("https://h/é?k=值#片";, "PATH", None, "/é"),
+ // The key may introduce capture groups; both Spark and the
existing
+ // matcher return group 2 of the full pattern, not its last group.
+ ("https://h/?abc=1&v=2";, "QUERY", Some("(abc)"), "abc"),
+ ("https://h/?abc=1&v=2";, "QUERY", Some("v"), "2"),
+ ("https://h/?abc=1&v=2";, "QUERY", Some("(abc)"), "abc"),
+ ] {
+ let result = parse_url_component(url, part, key)?.unwrap();
+ assert_eq!(result, expected);
+ }
+ // Extraction must still reject an invalid component after the
requested host.
+ assert!(parse_url_component("https://host/p#invalid fragment", "HOST",
None).is_err());
+ Ok(())
+ }
+
+ #[test]
+ fn sliced_string_layouts_produce_owned_output() -> Result<()> {
Review Comment:
This test can't fail because of anything this PR changes. It checks that
Arrow's `FromIterator<Option<&str>>` copies into fresh buffers, which also held
before this PR, since the old code collected `Option<String>`. "The output
remains valid after dropping all inputs" is already guaranteed by `ArrayRef`
being `'static`. The only thing it would catch is a future switch to zero-copy
views of the input, which nothing here is moving toward. Unless that risk is
concrete, I'd drop this test and the matching comment on `process_parse_url`
(line 423), since slicing over the three layouts is Arrow's iterator behavior,
not `parse_url` logic.
##########
native/spark-expr/src/url_funcs/parse_url.rs:
##########
@@ -133,17 +194,18 @@ fn parse_url_component(value: &str, part: &str, key:
Option<&str>) -> Result<Opt
return Err(invalid_url_err(value));
}
- let caps = match URI_REGEX.captures(value) {
- Some(c) => c,
- None => return Ok(None),
+ let Some(UriParts {
+ scheme,
+ authority,
+ path,
+ file,
+ query,
+ fragment,
+ }) = decompose_http(value).or_else(|| decompose_generic(value))
Review Comment:
+1. One more reason it's safe: on any input that passes
`has_invalid_uri_chars`, `URI_REGEX` always matches. Every group is optional or
can be empty, and the only way to fail is a `\n` inside the fragment (`.`
excludes it), which is already rejected as a control character. So the `let …
else { return Ok(None) }` arm here and the `caps.get(5)?` in
`decompose_generic` are unreachable today. A total splitter would make that
explicit, and `URI_REGEX` could move into the tests as the oracle for a parity
test covering all schemes, not just `http`/`https`. I also fuzzed both
`decompose_http` and the splitter sketched above against the regex on ~1.4M
random validated inputs (biased toward `:/?#@[]`, non-ASCII, and
`http://`/`HTTP://`/`//` fragments) and found no mismatch.
--
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]
