comphead opened a new issue, #7330:
URL: https://github.com/apache/datafusion-comet/issues/7330
### Describe the bug
Native `parse_url` (and `try_parse_url`) runs by default (`CometParseUrl` is
Compatible), but it accepts some URLs that Spark's `java.net.URI` treats
differently, so Comet and Spark return different results.
Spark builds `new URI(url)` (`ParseUrl` in 3.5,
`UrlExpressionEvalUtils.getUrl` in 4.x). On `URISyntaxException` it returns
NULL, or raises `INVALID_URL` when `failOnError` (ANSI) is set, and `HOST` is
`URI.getHost`. Comet's native path validates only spaces, `{`, `}`, `<`, `>`
and control characters (`has_invalid_uri_chars`), then splits the URL with the
RFC 3986 Appendix B regex. `extract_host` takes whatever follows the last `@`
in the authority.
### Steps to reproduce
`java.net.URI` on JDK 17 for each input (local probe):
| Input | `java.net.URI` | Spark `parse_url` | Comet native (from reading
`parse_url.rs`, not run) |
| --- | --- | --- | --- |
| `http://a_b.com/p` | parses, `getHost()` is null (registry-based
authority) | `HOST` is NULL | `HOST` is `a_b.com` |
| `http://u@v@h/p` | parses, `getHost()` is null | `HOST` is NULL | `HOST`
is `h` |
| `http://h/%zz` | `URISyntaxException` | NULL for every part, or
`INVALID_URL` under ANSI | returns the components, e.g. `PATH` is `/%zz` |
| `http://h/p#a#b` | `URISyntaxException` | NULL for every part, or
`INVALID_URL` under ANSI | returns the components, e.g. `PATH` is `/p` |
```sql
SELECT parse_url('http://a_b.com/p', 'HOST'),
parse_url('http://u@v@h/p', 'HOST'),
parse_url('http://h/%zz', 'PATH'),
parse_url('http://h/p#a#b', 'PATH');
```
The query needs to read the URLs from a table (or disable constant folding)
for the expression to reach Comet.
### Expected behavior
The same results as Spark: a null host for registry-based authorities, and
NULL (or `INVALID_URL` under ANSI) for URLs that `java.net.URI` rejects, such
as invalid `%` escapes and a second `#`. `USERINFO` of a registry-based
authority may need the same check, since `URI.getUserInfo` is null there too.
### Additional context
Found while reviewing #7329, which does not change this behavior: its new
HTTP fast path matches the existing regex decomposition, so the gap is on
`main` already. The current `parse_url.sql` fixtures use only well-formed URLs,
so none of these inputs is covered.
cc @sunchao
--
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]
