Skip to content

Commit 52f23ae

Browse files
kumarUjjawalkumarUjjawal
authored
fix(spark): align parse_url empty FILE path (apache#21969)
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#21943. ## Rationale for this change `parse_url` in the Spark function library did not match Spark for some empty path URL cases. In particular, absolute URLs with no explicit path were treated as if they had `/` because the URL parser normalizes the path. Also handles - `parse_url(..., 'PATH')` now returns `/` for an explicit root path like `http://example.com/`. - `parse_url(..., 'QUERY', key)` now returns the raw query value, so percent-encoded values like `x%20y` are not decoded to `x y`. <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> ## What changes are included in this PR? This PR keeps the existing URL parser, but adjusts `FILE` extraction so parser-normalized `/` is treated as empty only when the original URL had no path after the authority. It also adds regression coverage for `parse_url` and `try_parse_url`, including the boundary between a missing path and an explicit root path. <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? Yes <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? `parse_url` and `try_parse_url` now match Spark more closely for empty path FILE results. There is no public API change. <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
1 parent cae03c9 commit 52f23ae

3 files changed

Lines changed: 270 additions & 19 deletions

File tree

‎datafusion/spark/src/function/url/parse_url.rs‎

Lines changed: 110 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ impl ParseUrl {
7777
/// # Returns
7878
///
7979
/// * `Ok(Some(String))` - The extracted URL component as a string
80-
/// * `Ok(None)` - If the requested component doesn't exist or is empty
80+
/// * `Ok(None)` - If the requested component doesn't exist
8181
/// * `Err(DataFusionError)` - If the URL is malformed and cannot be parsed
8282
fn parse(value: &str, part: &str, key: Option<&str>) -> Result<Option<String>> {
8383
let url: std::result::Result<Url, ParseError> = Url::parse(value);
@@ -97,12 +97,7 @@ impl ParseUrl {
9797
"PATH" => Some(path.to_string()),
9898
"QUERY" => match key {
9999
None => query.map(String::from),
100-
Some(key) => query.and_then(|q| {
101-
q.split('&')
102-
.filter_map(|pair| pair.split_once('='))
103-
.find(|(k, _)| *k == key)
104-
.map(|(_, v)| v.to_string())
105-
}),
100+
Some(key) => Self::query_value(query, key).map(String::from),
106101
},
107102
"REF" => fragment.map(String::from),
108103
"FILE" => {
@@ -122,21 +117,17 @@ impl ParseUrl {
122117
.map(|url| match part {
123118
"HOST" => url.host_str().map(String::from),
124119
"PATH" => {
125-
let path: String = url.path().to_string();
126-
let path: String = if path == "/" { "".to_string() } else { path };
127-
Some(path)
120+
let path = Self::path(value, &url);
121+
Some(path.to_string())
128122
}
129123
"QUERY" => match key {
130124
None => url.query().map(String::from),
131-
Some(key) => url
132-
.query_pairs()
133-
.find(|(k, _)| k == key)
134-
.map(|(_, v)| v.into_owned()),
125+
Some(key) => Self::query_value(url.query(), key).map(String::from),
135126
},
136127
"REF" => url.fragment().map(String::from),
137128
"PROTOCOL" => Some(url.scheme().to_string()),
138129
"FILE" => {
139-
let path = url.path();
130+
let path = Self::path(value, &url);
140131
match url.query() {
141132
Some(query) => Some(format!("{path}?{query}")),
142133
None => Some(path.to_string()),
@@ -156,6 +147,36 @@ impl ParseUrl {
156147
_ => None,
157148
})
158149
}
150+
151+
fn path<'a>(value: &str, url: &'a Url) -> &'a str {
152+
let path = url.path();
153+
if path == "/" && Self::absolute_url_has_empty_path(value) {
154+
""
155+
} else {
156+
path
157+
}
158+
}
159+
160+
fn absolute_url_has_empty_path(value: &str) -> bool {
161+
let Some(authority_start) = value.find("://").map(|index| index + 3) else {
162+
return false;
163+
};
164+
let after_authority = &value[authority_start..];
165+
match after_authority.find(['/', '?', '#']) {
166+
None => true,
167+
Some(index) => matches!(after_authority.as_bytes()[index], b'?' | b'#'),
168+
}
169+
}
170+
171+
fn query_value<'a>(query: Option<&'a str>, key: &str) -> Option<&'a str> {
172+
query.and_then(|query| {
173+
query
174+
.split('&')
175+
.filter_map(|pair| pair.split_once('='))
176+
.find(|(query_key, _)| *query_key == key)
177+
.map(|(_, value)| value)
178+
})
179+
}
159180
}
160181

161182
impl ScalarUDFImpl for ParseUrl {
@@ -382,9 +403,79 @@ mod tests {
382403
}
383404

384405
#[test]
385-
fn test_parse_path_root_is_empty_string() -> Result<()> {
386-
let got = ParseUrl::parse("https://example.com/", "PATH", None)?;
387-
assert_eq!(got, Some("".to_string()));
406+
fn test_parse_path_empty_vs_root() -> Result<()> {
407+
assert_eq!(
408+
ParseUrl::parse("https://example.com", "PATH", None)?,
409+
Some("".to_string())
410+
);
411+
assert_eq!(
412+
ParseUrl::parse("https://example.com/", "PATH", None)?,
413+
Some("/".to_string())
414+
);
415+
assert_eq!(
416+
ParseUrl::parse("https://ex.com/dir%20/pa%20th.HTML", "PATH", None)?,
417+
Some("/dir%20/pa%20th.HTML".to_string())
418+
);
419+
Ok(())
420+
}
421+
422+
#[test]
423+
fn test_parse_query_key_is_raw() -> Result<()> {
424+
let url = "https://use%20r:pas%20s@example.com/dir%20/pa%20th.HTML?query=x%20y&q2=2#Ref%20two";
425+
assert_eq!(
426+
ParseUrl::parse(url, "QUERY", None)?,
427+
Some("query=x%20y&q2=2".to_string())
428+
);
429+
assert_eq!(
430+
ParseUrl::parse(url, "QUERY", Some("query"))?,
431+
Some("x%20y".to_string())
432+
);
433+
assert_eq!(
434+
ParseUrl::parse("http://ex.com?key=", "QUERY", Some("key"))?,
435+
Some("".to_string())
436+
);
437+
assert_eq!(
438+
ParseUrl::parse("http://ex.com?keyonly", "QUERY", Some("keyonly"))?,
439+
None
440+
);
441+
assert_eq!(
442+
ParseUrl::parse("http://ex.com?a=1&a=2", "QUERY", Some("a"))?,
443+
Some("1".to_string())
444+
);
445+
assert_eq!(
446+
ParseUrl::parse("http://ex.com?a%20b=1", "QUERY", Some("a b"))?,
447+
None
448+
);
449+
Ok(())
450+
}
451+
452+
#[test]
453+
fn test_parse_empty_path_file() -> Result<()> {
454+
assert_eq!(ParseUrl::parse("", "PATH", None)?, Some("".to_string()));
455+
assert_eq!(
456+
ParseUrl::parse("http://example.com", "FILE", None)?,
457+
Some("".to_string())
458+
);
459+
assert_eq!(
460+
ParseUrl::parse("http://example.com?foo=bar", "FILE", None)?,
461+
Some("?foo=bar".to_string())
462+
);
463+
assert_eq!(
464+
ParseUrl::parse("http://example.com#fragment", "FILE", None)?,
465+
Some("".to_string())
466+
);
467+
assert_eq!(
468+
ParseUrl::parse("http://example.com/?foo=bar", "FILE", None)?,
469+
Some("/?foo=bar".to_string())
470+
);
471+
assert_eq!(
472+
ParseUrl::parse("http://ex.com/?", "FILE", None)?,
473+
Some("/?".to_string())
474+
);
475+
assert_eq!(
476+
ParseUrl::parse("http://ex.com?", "FILE", None)?,
477+
Some("?".to_string())
478+
);
388479
Ok(())
389480
}
390481

@@ -482,7 +573,7 @@ mod tests {
482573

483574
assert_eq!(out_sa.len(), 2);
484575
assert_eq!(out_sa.value(0), "example.com");
485-
assert_eq!(out_sa.value(1), "");
576+
assert_eq!(out_sa.value(1), "/");
486577
Ok(())
487578
}
488579

‎datafusion/sqllogictest/test_files/spark/url/parse_url.slt‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,86 @@ SELECT parse_url('https://example.com', 'PATH');
235235
----
236236
(empty)
237237

238+
query T
239+
SELECT parse_url('https://example.com/', 'PATH');
240+
----
241+
/
242+
243+
query T
244+
SELECT parse_url('https://ex.com/dir%20/pa%20th.HTML', 'PATH');
245+
----
246+
/dir%20/pa%20th.HTML
247+
248+
query T
249+
SELECT parse_url('', 'PATH');
250+
----
251+
(empty)
252+
253+
query T
254+
SELECT parse_url('http://example.com', 'FILE');
255+
----
256+
(empty)
257+
258+
query T
259+
SELECT parse_url('http://example.com/', 'FILE');
260+
----
261+
/
262+
263+
query T
264+
SELECT parse_url('http://example.com?foo=bar', 'FILE');
265+
----
266+
?foo=bar
267+
268+
query T
269+
SELECT parse_url('http://example.com#fragment', 'FILE');
270+
----
271+
(empty)
272+
273+
query T
274+
SELECT parse_url('http://example.com/?foo=bar', 'FILE');
275+
----
276+
/?foo=bar
277+
278+
query T
279+
SELECT parse_url('http://ex.com/?', 'FILE');
280+
----
281+
/?
282+
283+
query T
284+
SELECT parse_url('http://ex.com?', 'FILE');
285+
----
286+
?
287+
288+
query T
289+
SELECT parse_url('https://use%20r:pas%20s@example.com/dir%20/pa%20th.HTML?query=x%20y&q2=2#Ref%20two', 'QUERY');
290+
----
291+
query=x%20y&q2=2
292+
293+
query T
294+
SELECT parse_url('https://use%20r:pas%20s@example.com/dir%20/pa%20th.HTML?query=x%20y&q2=2#Ref%20two', 'QUERY', 'query');
295+
----
296+
x%20y
297+
298+
query T
299+
SELECT parse_url('http://ex.com?key=', 'QUERY', 'key');
300+
----
301+
(empty)
302+
303+
query T
304+
SELECT parse_url('http://ex.com?keyonly', 'QUERY', 'keyonly');
305+
----
306+
NULL
307+
308+
query T
309+
SELECT parse_url('http://ex.com?a=1&a=2', 'QUERY', 'a');
310+
----
311+
1
312+
313+
query T
314+
SELECT parse_url('http://ex.com?a%20b=1', 'QUERY', 'a b');
315+
----
316+
NULL
317+
238318
query T
239319
SELECT parse_url('https://example.com', 'path');
240320
----

‎datafusion/sqllogictest/test_files/spark/url/try_parse_url.slt‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,86 @@ SELECT try_parse_url('https://example.com', 'PATH');
186186
----
187187
(empty)
188188

189+
query T
190+
SELECT try_parse_url('https://example.com/', 'PATH');
191+
----
192+
/
193+
194+
query T
195+
SELECT try_parse_url('https://ex.com/dir%20/pa%20th.HTML', 'PATH');
196+
----
197+
/dir%20/pa%20th.HTML
198+
199+
query T
200+
SELECT try_parse_url('', 'PATH');
201+
----
202+
(empty)
203+
204+
query T
205+
SELECT try_parse_url('http://example.com', 'FILE');
206+
----
207+
(empty)
208+
209+
query T
210+
SELECT try_parse_url('http://example.com/', 'FILE');
211+
----
212+
/
213+
214+
query T
215+
SELECT try_parse_url('http://example.com?foo=bar', 'FILE');
216+
----
217+
?foo=bar
218+
219+
query T
220+
SELECT try_parse_url('http://example.com#fragment', 'FILE');
221+
----
222+
(empty)
223+
224+
query T
225+
SELECT try_parse_url('http://example.com/?foo=bar', 'FILE');
226+
----
227+
/?foo=bar
228+
229+
query T
230+
SELECT try_parse_url('http://ex.com/?', 'FILE');
231+
----
232+
/?
233+
234+
query T
235+
SELECT try_parse_url('http://ex.com?', 'FILE');
236+
----
237+
?
238+
239+
query T
240+
SELECT try_parse_url('https://use%20r:pas%20s@example.com/dir%20/pa%20th.HTML?query=x%20y&q2=2#Ref%20two', 'QUERY');
241+
----
242+
query=x%20y&q2=2
243+
244+
query T
245+
SELECT try_parse_url('https://use%20r:pas%20s@example.com/dir%20/pa%20th.HTML?query=x%20y&q2=2#Ref%20two', 'QUERY', 'query');
246+
----
247+
x%20y
248+
249+
query T
250+
SELECT try_parse_url('http://ex.com?key=', 'QUERY', 'key');
251+
----
252+
(empty)
253+
254+
query T
255+
SELECT try_parse_url('http://ex.com?keyonly', 'QUERY', 'keyonly');
256+
----
257+
NULL
258+
259+
query T
260+
SELECT try_parse_url('http://ex.com?a=1&a=2', 'QUERY', 'a');
261+
----
262+
1
263+
264+
query T
265+
SELECT try_parse_url('http://ex.com?a%20b=1', 'QUERY', 'a b');
266+
----
267+
NULL
268+
189269
query T
190270
SELECT try_parse_url('https://example.com', 'path');
191271
----

0 commit comments

Comments
 (0)