valentinsoe opened a new pull request, #7392: URL: https://github.com/apache/datafusion-comet/pull/7392
## Which issue does this PR close? Closes #7385. ## Rationale for this change Native `parse_url` / `try_parse_url` return NULL for every part of a URL without a scheme (`/p?id=1`, `?id=7`, `//host/p`, `notaurl`). Spark's `ParseUrl` uses `java.net.URI`, which parses these as relative references, so with Comet enabled `parse_url(page, 'QUERY', 'gclid')` on logged page paths silently returns NULL instead of the value. ## What changes are included in this PR? In `parse_url_component`, the early `return Ok(None)` for scheme-less values is replaced by the two cases where `java.net.URI` actually rejects such a value: - a leading `:` (empty scheme name, "Expected scheme name at index 0") → `INVALID_URL`; - an empty authority with nothing after it (`//`, `http://`, "Expected authority") → `INVALID_URL`. Everything else goes through the existing `decompose_uri` / part extraction, which already splits relative references per RFC 3986 Appendix B. Side effects, both matching Spark: - `://` inside a relative URL's query (`/r?to=http://x.com&id=1`) no longer raises `INVALID_URL`. - `http://` (scheme, empty authority, nothing else) used to return PROTOCOL `http`; it now raises `INVALID_URL` (ANSI) / returns NULL, as Spark does. Not in scope: Comet's character validation (`has_invalid_uri_chars`) is narrower than `java.net.URI`'s (e.g. `|`, a malformed `%` escape, a second `#`), and `extract_host` doesn't model registry-based authorities (`//ex_ample.com/p`). Those gaps already exist for absolute URLs and are tracked in #7330; with this change scheme-less inputs reach them too (Spark returns NULL for such inputs, Comet now returns parsed values instead of NULL). ## How are these changes tested? - Rust unit tests: `test_relative_reference` (13 relative references × all 8 parts, expectations taken from Spark 4.1.1), `test_invalid_relative_reference` (`:`, `:foo`, `://missing-scheme`, `//`, `http://`), `test_try_parse_url_relative_reference` (3-arg QUERY key through `dispatch_parse_url`). They fail on `main` and pass with the fix; all existing `spark-expr` tests pass. - SQL file tests: a relative-URL table queried for every part in `parse_url.sql`, plus relative and error cases in `parse_url_ansi.sql`. `CometSqlFileTestSuite parse_url` passes locally (Spark 4.1 profile) and fails on `main` at the new queries. - Differential check (local only, not committed) of the native UDFs against Spark 4.1.1 `try_parse_url` + `java.net.URI` validity on ~110k generated URLs and ~40k real-world relative page paths (each also tried with a host prepended): no change in results for absolute URLs except the `http://`-style inputs above; relative references now match Spark except the inputs covered by #7330. - `cargo bench --bench parse_url` (main vs this branch): existing absolute-URL cases unchanged (all within ±4%). Scheme-less rows now do the normal extraction instead of returning early. 🤖 Generated with [Claude Code](https://claude.com/claude-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]
