comphead commented on code in PR #7329: URL: https://github.com/apache/datafusion-comet/pull/7329#discussion_r4419590424
########## docs/source/contributor-guide/expression-audits/url_funcs.md: ########## @@ -25,6 +25,8 @@ - Performance (tuned 2026-07-11, PR [#4893](https://github.com/apache/datafusion-comet/pull/4893)): cache the compiled query regex in a thread-local single-slot cache keyed by the query key, eliminating per-row `Regex::new()` recompilation in the QUERY-with-key path (the key is constant across a batch in practice). Up to 50x faster. Benchmark: `benches/parse_url.rs`. +- Performance (tuned 2026-10-30, PR [#7329](https://github.com/apache/datafusion-comet/pull/7329)): borrow raw URL components and use delimiter decomposition for HTTP/HTTPS. Non-null HTTP(S) component microbenchmarks are 5.5–9.4x faster and query-key extraction is 3.1x faster. The PR records the alternate-lockfile measurement environment and generic-parser controls. Benchmark: `benches/parse_url.rs`. Review Comment: The other entries here name the technique, the speedup, the PR and the benchmark. The lockfile sentence describes how this PR was measured, which the description already covers, and will not help someone checking later whether `parse_url` has been tuned. Could it go? ########## 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: The Appendix B regex is a fixed delimiter split, so one hand-written splitter could replace both `decompose_http` and `decompose_generic` for every input, removing `URI_REGEX`, the second decomposer and the parity test, and extending the speedup to `ftp://`, `HTTP://`, `mailto:` and scheme-relative inputs (the FTP control is 1.03x today). With leftmost-first matching, `(([^:/?#]+):)?` is taken exactly when the first of `:/?#` is a `:` at index > 0, and the rest always matches unless there is a `\n` in the fragment, which `has_invalid_uri_chars` already rejects. Roughly: ```rust let (scheme, rest) = match value.find([':', '/', '?', '#']) { Some(i) if i > 0 && value.as_bytes()[i] == b':' => (Some(&value[..i]), &value[i + 1..]), _ => (None, value), }; // then the authority split on "//" and the remainder as in decompose_http ``` In a local probe outside the repo, against regex 1.13.1 from the lockfile, such a splitter matched `URI_REGEX` on 1.78M random inputs that pass validation, and decomposed 8,192 benchmark-shaped FTP URLs in about 0.16 ms against 4.3 to 5.4 ms for the regex (decomposition only, not the UDF). ########## 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"), Review Comment: This row repeats line 496 exactly. -- 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]
