github-actions[bot] commented on code in PR #68011:
URL: https://github.com/apache/doris/pull/68011#discussion_r4013966399
##########
be/src/util/url_parser.cpp:
##########
@@ -226,61 +227,81 @@ bool UrlParser::parse_url_key(const StringRef& url,
UrlPart part, const StringRe
// Remove leading and trailing spaces.
StringRef trimmed_url = url.trim();
- // Search for the key in the url, ignoring malformed URLs for now.
+ // The key can only be found in the query component, which starts at the
first '?' and ends
+ // before the '#' that starts the fragment (if any).
+ int32_t query_pos = _s_question_search.search(&trimmed_url);
+ if (query_pos < 0) {
+ // Query component is missing, the whole url is the path plus the
fragment.
+ return false;
+ }
Review 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).
##########
regression-test/suites/function_p0/test_parse_url_key.groovy:
##########
@@ -0,0 +1,63 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_parse_url_key") {
+
+ sql """
+ drop table if exists test_parse_url_key;
+ """
+
+ sql """
+ CREATE TABLE `test_parse_url_key` (
+ `id` int NULL,
+ `url` text NULL
+ ) ENGINE=OLAP
+ DUPLICATE KEY(`id`)
+ DISTRIBUTED BY RANDOM BUCKETS AUTO
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1"
+ );
+ """
+
+ sql """
+ insert into test_parse_url_key values
+ (1, 'http://h/p#f?k=v'),
+ (2, 'http://h/p&k=v?x=1'),
+ (3, 'http://h/p#f/?#k=v'),
+ (4, 'http://h/p?x=1#f&k=v'),
+ (5, 'http://h/p&k=v?k=1#f&k=2'),
+ (6, 'http://h/p#f&k=v?k=2'),
+ (7, 'http://h/p'),
+ (8, 'http://h/p?a=1&k=2')
+ """
+
+ // The query key must only be looked up inside the query component, which
is located
+ // between the first '?' and the following '#'. Keys appearing in the path
or in the
+ // fragment must not be returned.
+ qt_sql """
+ select id, parse_url(url, 'QUERY', 'k') as query_k from
test_parse_url_key order by id
+ """
+
+ qt_sql """
+ select id, parse_url(url, 'QUERY', 'x') as query_x from
test_parse_url_key order by id
+ """
+
+ sql """
Review 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.
##########
be/src/util/url_parser.cpp:
##########
@@ -226,61 +227,81 @@ bool UrlParser::parse_url_key(const StringRef& url,
UrlPart part, const StringRe
// Remove leading and trailing spaces.
StringRef trimmed_url = url.trim();
- // Search for the key in the url, ignoring malformed URLs for now.
+ // The key can only be found in the query component, which starts at the
first '?' and ends
+ // before the '#' that starts the fragment (if any).
+ int32_t query_pos = _s_question_search.search(&trimmed_url);
+ if (query_pos < 0) {
+ // Query component is missing, the whole url is the path plus the
fragment.
+ return false;
+ }
+ int32_t fragment_pos = _s_hash_search.search(&trimmed_url);
+ if (fragment_pos >= 0 && fragment_pos < query_pos) {
+ // The '#' comes before the '?', so the text after the '?' is a
fragment, not a query.
+ return false;
+ }
+
+ StringRef query = trimmed_url.substring(query_pos + _s_question.size);
+ int32_t query_end_pos = _s_hash_search.search(&query);
+ if (query_end_pos >= 0) {
+ // The fragment starts at the '#', so the query component ends right
before it.
+ query = query.substring(0, query_end_pos);
+ }
+
+ // Search for the key inside the query component, ignoring malformed URLs
for now.
StringSearch key_search(&key);
+ // Offset of the next search inside the query component. The query
component starts right
+ // after the '?', so a key at offset 0 is a query key as well.
+ int32_t offset = 0;
+ bool found = false;
- while (trimmed_url.size > 0) {
- // Search for the key in the current substring.
- int32_t key_pos = key_search.search(&trimmed_url);
- bool match = true;
+ while (offset < query.size) {
+ // Search for the key in the remaining part of the query component.
+ StringRef rest = query.substring(offset);
+ int32_t key_pos = key_search.search(&rest);
if (key_pos < 0) {
- return false;
- }
-
- // Key pos must be != 0 because it must be preceded by a '?' or a '&'.
- // Check that the char before key_pos is either '?' or '&'.
- if (key_pos == 0 ||
- (trimmed_url.data[key_pos - 1] != '?' && trimmed_url.data[key_pos
- 1] != '&')) {
- match = false;
+ // No (more) key in the query component.
+ break;
}
- // Advance substring beyond matching key.
- trimmed_url = trimmed_url.substring(key_pos + key.size);
-
- if (!match) {
+ offset += key_pos;
+ // The key must start the query component or be preceded by a '&'.
+ if (offset != 0 && query.data[offset - 1] != '&') {
+ // The matched text is not a key, step over it and keep searching.
+ offset += cast_set<int32_t>(key.size);
continue;
}
- if (trimmed_url.size <= 0) {
- break;
- }
+ // Positioned to the char right after the key.
+ int32_t value_pos = offset + cast_set<int32_t>(key.size);
- // Next character must be '=', otherwise the match cannot be a key in
the query part.
- if (trimmed_url.data[0] != '=') {
+ // The key must be followed by a '=' and a value, otherwise the match
cannot be a key.
+ if (value_pos >= cast_set<int32_t>(query.size) ||
query.data[value_pos] != '=') {
+ // Step over the matched text and keep searching.
+ offset += cast_set<int32_t>(key.size);
continue;
}
- int32_t pos = 1;
-
- // Find ending position of key's value by matching '#' or '&'.
- while (pos < trimmed_url.size) {
- switch (trimmed_url.data[pos]) {
- case '#':
- case '&':
- *result = trimmed_url.substring(1, pos - 1);
- return true;
- }
+ ++value_pos;
- ++pos;
+ // Find the ending position of the key's value by matching '&'.
+ StringRef value_rest = query.substring(value_pos);
+ size_t value_end_rel_pos = value_rest.find_first_of('&');
+ int32_t value_end_pos;
+ if (value_end_rel_pos == std::numeric_limits<size_t>::max()) {
+ // 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);
Review 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.
##########
be/src/util/url_parser.cpp:
##########
@@ -226,61 +227,81 @@ bool UrlParser::parse_url_key(const StringRef& url,
UrlPart part, const StringRe
// Remove leading and trailing spaces.
StringRef trimmed_url = url.trim();
- // Search for the key in the url, ignoring malformed URLs for now.
Review 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.
--
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]
