sunchao commented on code in PR #7329:
URL: https://github.com/apache/datafusion-comet/pull/7329#discussion_r4425130282
##########
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:
Replaced both decomposers with one delimiter splitter in fd76067fea. It
preserves the scheme spelling, empty components and raw FILE slice. The old
Appendix B regex is now only a test oracle for HTTP, FTP, opaque and relative
inputs. The separate Java URI compatibility cases are tracked in #7330.
I also compared complete native HOST calls against the previous HTTP(S) fast
path on the checked-in lockfile. These are Criterion means in ms per 8,192
rows, using Utf8.
| Input | Previous fast path | Single splitter |
| --- | ---: | ---: |
| FTP | 8.523 | 1.485 |
| HTTP | 1.573 | 1.603 |
| HTTPS | 1.549 | 1.596 |
FTP improved 5.74×. Criterion did not detect a statistically significant
change for HTTP or HTTPS. All 25 comparisons against the upstream base are in
the [PR benchmark
table](https://github.com/apache/datafusion-comet/pull/7329#benchmark-measurements).
##########
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:
Removed the repeated `(abc)` row in fd76067fea. The two remaining query-key
cases cover capture group 2 and changing the key to `v`.
##########
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:
Removed the lockfile sentence from the audit in c342675322. The entry now
records the technique, measured speedups and benchmark. The PR description has
the measurement setup for the new runs on the checked-in lockfile.
--
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]
