Conversation
### What problem does this PR solve?
Problem Summary:
`parse_url(url, 'QUERY', key)` searched for the key in the whole (trimmed) url instead of
only in the query component, so text belonging to the path or to the fragment was accepted
as a query key.
Reproduction:
```sql
SELECT parse_url('http://h/p#f?k=v', 'QUERY', 'k'); -- returns 'v' (expected NULL)
SELECT parse_url('http://h/p&k=v?x=1', 'QUERY', 'k'); -- returns 'v?x=1' (expected NULL)
```
Root cause:
`UrlParser::parse_url_key` scanned `trimmed_url` from the beginning and treated any `?`/`&`
preceded match as a query key, so it never checked whether the match was inside the real
query component. Two further defects existed in the same loop: a key at offset 0 of the
search window (the first key of the query, or the first key after a `&`) was rejected, and a
key value was not bounded by the fragment, so `#` was reported as part of the value.
Fix:
Locate the real query component first - it starts at the first `?` and ends at the `#` that
starts the fragment - and reject urls whose `#` comes before the `?`. The key/value scan is
now bounded by that component, recognizes the first key and advances past each candidate so
that no text is visited twice.
After the fix both statements above return NULL, and the first query parameter as well as
duplicated keys (`?k=1&k=2` - the last one wins, as before) are handled correctly.
### Release note
Fix `parse_url(url, 'QUERY', key)` returning values from the path or the fragment for urls
that do not contain the requested key in the query.
### Check List (For Author)
- Test: Regression test / Unit Test
- New regression suite `regression-test/suites/function_p0/test_parse_url_key.groovy`
(output generated with `run-regression-test.sh --run -d function_p0 -s test_parse_url_key -forceGenOut`,
then re-run and passed).
- Extended `be/test/exprs/function/function_url_test.cpp` with `ParseUrlQueryKeyTest`;
`FunctionUrlTEST.*`, `function_string_test.function_parse_url_test` and
`function_string_test.function_extract_url_parameter_test` all pass.
- Behavior changed: Yes (see above, path/fragment text is no longer a query key)
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for four inline findings: two result-correctness/compatibility issues, one mandatory regression-test lifecycle violation, and one avoidable per-row linear scan.
Critical checkpoint conclusions
- Goal and proof: The keyed parser now confines lookup to the query component for the added path/fragment examples, but it is not correct as submitted: duplicate parameters regress from established first-wins behavior, and the two-argument
parse_url(..., 'QUERY')path still disagrees with the keyed form. - Scope, clarity, and reuse: The change is localized and its conditions are readable, but query-component discovery is duplicated instead of shared across both BE arities and Nereids folding; this causes the overload inconsistency and redundant delimiter scan reported inline.
- Concurrency: Calls use stack-local state and immutable same-TU searchers. No mutable shared state, locks, lock ordering, atomics, blocking work, or thread-context change is involved.
- Lifecycle/static initialization: No new ownership or shutdown lifecycle exists. The existing static constants/searchers are initialized in dependency order in one translation unit, with no new cross-TU dependency or cycle.
- Configuration: No configuration or dynamic-reload behavior is added.
- Compatibility: No symbol, RPC, thrift, storage/serialization, or rolling-upgrade format changes apply. SQL result compatibility does apply:
?k=1&k=2changes from1to2even though the PR describes duplicate behavior as unchanged. - Parallel paths: The BE two-argument overload and Nereids
StringArithmetic.parseUrlQuerystill treat?after#as a query; both need matching fixes/tests.extract_url_parameterhas analogous ordering and should be explicitly adjudicated. - Conditional logic: Apart from the accepted findings, the new start/
&,=, component-order, empty-value, and progress rules were replayed across invalid-then-valid and repeated candidates; no additional missed match, nontermination, or bounds defect was substantiated. - Test coverage and results: Test discovery and all eight existing const/vector dispatch shapes were verified statically. Both regression queries are ordered, and every new
.outrow is correct for the keyed cases. The duplicate-key unit oracle is wrong, the parallel two-argument/FE cases are missing, and the suite's terminal table drop violates the mandatory preserve-for-debugging rule. - Observability: This deterministic local scalar parser needs no new log, metric, or tracing field.
- Transactions, persistence, and writes: EditLog, failover, visible versions, MoW, production transactions, atomic writes, and crash recovery are not involved; regression DDL/DML is test setup only.
- FE-BE variables/protocol: No variable or payload is added. FE/BE semantic parity matters only for the existing Nereids two-argument folding path noted above.
- Performance, memory, errors, and nullability: One redundant full-suffix
#scan is reported inline. Otherwise there is no significant new allocation, MemTracker obligation, ignoredStatus, exception-boundary issue, nullable-column misuse, COW issue, or lifetime defect. - Other core invariants: Data visibility, version consistency, delete bitmaps, storage correctness, and security boundaries are untouched. Scalar result correctness and per-row CPU work are fully represented by the inline findings.
- User focus: No additional user-provided focus was supplied; the whole PR was reviewed.
This was a static-only review because the runner contract prohibits builds and tests. The author's reported unit/regression runs were not independently executed. Three review rounds completed; every final-round reviewer returned NO_NEW_VALUABLE_FINDINGS, all candidates were adjudicated, and the review converged cleanly at head 3140b5a5252dcaa61d2212a94c4f52667ce80c28.
| // Remove leading and trailing spaces. | ||
| StringRef trimmed_url = url.trim(); | ||
|
|
||
| // Search for the key in the url, ignoring malformed URLs for now. |
There was a problem hiding this comment.
[P1] Keep both parse_url forms consistent about query bounds
For the new reproducer http://h/p#f?k=v, this branch makes the three-argument form return NULL, but the unchanged UrlParser::parse_url(..., QUERY, ...) arm still finds the ? after # and returns k=v for the two-argument form. The patch therefore makes the same SQL function disagree about whether this URL has a query component, and the new regression suite cannot catch it because it calls only the keyed overload. Please centralize the ?/# bounds for both BE overloads and update Nereids StringArithmetic.parseUrlQuery plus literal/column test oracles as well; otherwise a BE-only correction will leave constant folding inconsistent. extract_url_parameter should also be explicitly checked for the analogous ordering.
| if (query_pos < 0) { | ||
| // Query component is missing, the whole url is the path plus the fragment. | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[P2] Reuse the fragment offset instead of rescanning the query
Line 237 has already searched the whole trimmed URL for the first #, so after rejecting the # < ? case that absolute offset (or its absence) completely determines the query end. This second search walks the query suffix again on every row; for a long URL with no fragment, the same suffix is scanned twice for # before key lookup, contrary to the PR's claim that no text is visited twice. Please construct the query view directly from query_pos and fragment_pos (ideally in the shared bounds helper needed by the overload fix).
| // Ending position is end of string. | ||
| value_end_pos = cast_set<int32_t>(query.size); | ||
| } else { | ||
| value_end_pos = value_pos + cast_set<int32_t>(value_end_rel_pos); |
There was a problem hiding this comment.
[P1] Preserve first-wins behavior for duplicate keys
The removed implementation returned immediately after extracting the first valid match, so parse_url('http://h/p?k=1&k=2#f', 'QUERY', 'k') returned 1. This loop now continues and overwrites result with 2, even though the PR and test comment say duplicate behavior is unchanged. Apache Hive's keyed parse_url also returns its first regex match. Please stop after the first valid query-component match and update the new duplicate-key oracle to expect 1; otherwise this focused fix introduces an unrelated SQL-result compatibility regression.
| select id, parse_url(url, 'QUERY', 'x') as query_x from test_parse_url_key order by id | ||
| """ | ||
|
|
||
| sql """ |
There was a problem hiding this comment.
[P2] Preserve the table after the regression assertions
The repository's regression-test contract requires dropping tables before use, not after the test, so the populated state remains available for failure investigation. This suite already performs the required pre-test drop at lines 20-22; please remove this terminal drop block.
2 participants
What problem does this PR solve?
Problem Summary:
parse_url(url, 'QUERY', key)searched for the key in the whole (trimmed) url instead of only in the query component, so text belonging to the path or to the fragment was accepted as a query key.Reproduction:
Root cause:
UrlParser::parse_url_keyscannedtrimmed_urlfrom the beginning and treated any?/&preceded match as a query key, so it never checked whether the match was inside the real query component. Two further defects existed in the same loop: a key at offset 0 of the search window (the first key of the query, or the first key after a&) was rejected, and a key value was not bounded by the fragment, so#was reported as part of the value.Fix:
Locate the real query component first - it starts at the first
?and ends at the#that starts the fragment - and reject urls whose#comes before the?. The key/value scan is now bounded by that component, recognizes the first key and advances past each candidate so that no text is visited twice.After the fix both statements above return NULL, and the first query parameter as well as duplicated keys (
?k=1&k=2- the last one wins, as before) are handled correctly.Release note
Fix
parse_url(url, 'QUERY', key)returning values from the path or the fragment for urls that do not contain the requested key in the query.Check List (For Author)
regression-test/suites/function_p0/test_parse_url_key.groovy(output generated withrun-regression-test.sh --run -d function_p0 -s test_parse_url_key -forceGenOut, then re-run and passed).be/test/exprs/function/function_url_test.cppwithParseUrlQueryKeyTest;FunctionUrlTEST.*,function_string_test.function_parse_url_testandfunction_string_test.function_extract_url_parameter_testall pass.What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)