Repository navigation
Conversation
sunchao
added
run-benchmark-check
github-actions
Bot
added
enhancement
|
cc @andygrove @comphead @viirya for review |
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
The fast path sits after has_invalid_uri_chars and feeds the same bracket, scheme and :// checks, so it changes results only if the two decomposers disagree, and in a local probe of 3M random strings (558k starting with http:// or https://) they never did on inputs that pass validation. The query-key regex and group 2 match Spark's UrlExpressionEvalUtils, and FromIterator<Option<&str>> copies into fresh buffers for all three layouts. Three comments.
Separately, and not caused by this PR: a JDK 17 probe of java.net.URI shows main already differs from Spark on a few inputs. http://a_b.com/p and http://u@v@h/p give a null host in Java (registry-based authority), where extract_host returns a_b.com and h, and http://h/%zz and http://h/p#a#b throw URISyntaxException in Java, so Spark returns NULL or raises, while has_invalid_uri_chars lets both through. That seems worth its own issue.
| file, | ||
| query, | ||
| fragment, | ||
| }) = decompose_http(value).or_else(|| decompose_generic(value)) |
There was a problem hiding this 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:
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_httpIn 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).
There was a problem hiding this 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.
| // 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"), |
There was a problem hiding this comment.
This row repeats line 496 exactly.
|
|
||
| - 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`. |
There was a problem hiding this 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?
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
The HTTP(S) fast path is equivalent to the regex on validated input: it splits at the first #, then at the first ?, which is what [^?#]* / [^#]* do. Borrowed slices are copied into fresh output buffers for all three layouts. The FTP control (1.03x) suggests nearly all of the gain comes from skipping the regex rather than from dropping the per-row Strings. That makes the single-splitter route comphead suggested the more valuable direction: it would carry the same speedup to every scheme instead of only lowercase http:///https:// prefixes.
| } | ||
|
|
||
| #[test] | ||
| fn sliced_string_layouts_produce_owned_output() -> Result<()> { |
There was a problem hiding this 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.
Which issue does this PR close?
No linked issue; this optimizes the existing native URL expressions.
Rationale for this change
Native
parse_urlandtry_parse_urlcurrently allocate a temporary string for each extracted component and run a general URI regex for ordinary HTTP/HTTPS URLs. That work is repeated for every input row.What changes are included in this PR?
Borrow component slices until Arrow copies them into independently owned output buffers.
FILEuses the contiguous path/query slice, and query-key matches retain the existing regex capture semantics. Lowercase HTTP/HTTPS prefixes use delimiter decomposition with the existing validation; other URLs keep the native regex parser.Extend the benchmark to cover all three Arrow string layouts, short and long URLs, nulls, Unicode, query keys, invalid inputs, and FTP controls.
How are these changes tested?
git diff --checkpass.--lib --tests --bench parse_url -- -D warnings).Benchmark measurements
Compared source commit
a0c4ac35dewith upstream7f73d86fb841f5662f1343cf52b6e6b31eb17dfb, using the identical extended benchmark on both versions. These are complete native UDF calls, including argument broadcasting and Arrow output construction, over 8,192 rows per batch. They are not end-to-end Spark query measurements.Selected results show the Criterion mean time per batch. Speedup is base time divided by PR time.
Utf8Utf8Utf8Utf8ViewUtf8Utf8Utf8Utf8Full results for all 25 benchmark cases
FTP exercises the generic parser. Measurements varied across runs: an early run flagged a slowdown, but head/base/base/head repeats did not reproduce a consistent direction. The table reports only the final source after an equivalent prefix-check lint cleanup. Criterion flagged no regression in that final run. These measurements do not establish statistical equivalence or a precise FTP speedup.
Environment: Linux x86_64, AMD EPYC Milan, Rust 1.98.1, release profile with LTO disabled and 16 codegen units, default Rust allocator. Full paired runs: 20 samples, 1-second warmup, 2-second requested measurement time; FTP repeats: 40 samples and 3-second requested measurement time. Criterion extended sampling when necessary.
Local validation limitation: the environment's registry mirror lacks versions pinned by the upstream lockfile, including
uuid 1.29.0. Tests, Clippy and both benchmark builds use the same uncommitted alternate lockfile in an isolated source copy. It retains DataFusion 55.2.0 and Arrow 59.3.1 but changes some transitive versions, including ICU 2.4 to 2.3. The submitted manifests and lockfile are unchanged. Exact-lock workspace build/Clippy and JVM integration tests remain for CI; the Maven portion of localmake formatcould not write to the default read-only Maven cache.From the root of a checkout of this PR, run the same benchmark harness on both revisions and save the baseline in a shared Criterion directory. Both builds use the same dependency lock.