github-actions[bot] commented on code in PR #68490:
URL: https://github.com/apache/doris/pull/68490#discussion_r4092209592
##########
be/src/util/url_parser.cpp:
##########
@@ -111,10 +111,12 @@ bool UrlParser::parse_url(const StringRef& url, UrlPart
part, StringRef* result)
case FILE:
case PATH: {
- // Find first '/'.
int32_t start_pos = _s_slash_search.search(&protocol_end);
- if (start_pos < 0) {
+ int32_t question_pos = _s_question_search.search(&protocol_end);
+ int32_t hash_pos = _s_hash_search.search(&protocol_end);
+ if (start_pos < 0 || (question_pos >= 0 && question_pos < start_pos) ||
Review Comment:
[P1] Preserve query-only FILE components
This early return is correct for `PATH`, but not for `FILE`. For example,
`parse_url('http://h?k=/v#frag', 'FILE')` now returns `""` because `?` precedes
the first slash, even though FILE is path plus query and should be `?k=/v`
(Hive delegates this to Java `URL.getFile()`). The FE `parseUrlFile` mirrors
the same mistake, so folding and runtime agree on the wrong value. Please split
FILE's start handling from PATH and add the same query-only case to both
implementations' tests.
##########
be/src/exprs/function/function_string_url.cpp:
##########
@@ -111,101 +111,67 @@ class FunctionStringParseUrl : public IFunction {
size_t argument_size = arguments.size();
const bool has_key = argument_size == 3;
- std::vector<ColumnPtr> argument_columns(argument_size);
- std::vector<UInt8> col_const(argument_size);
- for (size_t i = 0; i < argument_size; ++i) {
- std::tie(argument_columns[i], col_const[i]) =
-
unpack_if_const(block.get_by_position(arguments[i]).column);
- }
-
- const auto* url_col = assert_cast<const
ColumnString*>(argument_columns[0].get());
- const auto* part_col = assert_cast<const
ColumnString*>(argument_columns[1].get());
- const bool part_const = col_const[1];
- std::vector<UrlParser::UrlPart> url_parts;
- const int part_nums = part_const ? 1 : input_rows_count;
-
- url_parts.resize(part_nums);
- for (int i = 0; i < part_nums; i++) {
- StringRef part = part_col->get_data_at(i);
- UrlParser::UrlPart url_part = UrlParser::get_url_part(part);
- if (url_part == UrlParser::INVALID) {
- return Status::RuntimeError("Invalid URL part: {}\n{}",
- std::string(part.data, part.size),
- "(Valid URL parts are 'PROTOCOL',
'HOST', "
- "'PATH', 'REF', 'AUTHORITY', "
- "'FILE', 'USERINFO', 'PORT' and
'QUERY')");
- }
- url_parts[i] = url_part;
- }
-
+ const auto url_col =
+
ColumnView<TYPE_STRING>::create(block.get_by_position(arguments[0]).column);
+ const auto part_col =
+
ColumnView<TYPE_STRING>::create(block.get_by_position(arguments[1]).column);
if (has_key) {
- const bool url_const = col_const[0];
- const bool key_const = col_const[2];
- const auto* key_col = assert_cast<const
ColumnString*>(argument_columns[2].get());
- RETURN_IF_ERROR(std::visit(
- [&](auto url_const, auto part_const, auto key_const) {
- return vector_parse_key<url_const, part_const,
key_const>(
- url_col, url_parts, key_col, input_rows_count,
null_map_data,
- res_chars, res_offsets);
- },
- make_bool_variant(url_const),
make_bool_variant(part_const),
- make_bool_variant(key_const)));
- } else {
- const bool url_const = col_const[0];
- RETURN_IF_ERROR(std::visit(
- [&](auto url_const, auto part_const) {
- return vector_parse<url_const, part_const>(url_col,
url_parts,
-
input_rows_count, null_map_data,
- res_chars,
res_offsets);
- },
- make_bool_variant(url_const),
make_bool_variant(part_const)));
- }
- block.get_by_position(result).column =
- ColumnNullable::create(std::move(res), std::move(null_map));
- return Status::OK();
- }
- template <bool url_const, bool part_const>
- static Status vector_parse(const ColumnString* url_col,
- std::vector<UrlParser::UrlPart>& url_parts,
const int size,
- ColumnUInt8::Container& null_map_data,
- ColumnString::Chars& res_chars,
ColumnString::Offsets& res_offsets) {
- for (size_t i = 0; i < size; ++i) {
- UrlParser::UrlPart& url_part =
url_parts[index_check_const<part_const>(i)];
- StringRef url_val =
url_col->get_data_at(index_check_const<url_const>(i));
- StringRef parse_res;
- if (UrlParser::parse_url(url_val, url_part, &parse_res)) {
- if (parse_res.empty()) [[unlikely]] {
- StringOP::push_empty_string(i, res_chars, res_offsets);
+ const auto key_col =
+
ColumnView<TYPE_STRING>::create(block.get_by_position(arguments[2]).column);
+ for (size_t i = 0; i < input_rows_count; ++i) {
+ if (url_col.is_null_at(i) || part_col.is_null_at(i) ||
key_col.is_null_at(i)) {
+ StringOP::push_null_string(i, res_chars, res_offsets,
null_map_data);
continue;
}
- StringOP::push_value_string(std::string_view(parse_res.data,
parse_res.size), i,
- res_chars, res_offsets);
- } else {
- StringOP::push_null_string(i, res_chars, res_offsets,
null_map_data);
+ const auto part = part_col.value_at(i);
+ const auto url_part = UrlParser::get_url_part(part);
Review Comment:
[P2] Preserve the constant-part fast path
For the usual `parse_url(url_column, 'HOST')` shape, `part_col` is a
`ColumnConst`, but both new loops now call `get_url_part` for every row. That
helper copies and uppercases the same string each time; the removed code
converted a constant part once before the loop. This turns one component lookup
per block into `input_rows_count` redundant lookups on large scans. Please
cache the enum for a non-NULL constant part (lazily if needed to preserve the
new NULL short-circuit) and keep per-row conversion only for vector parts; the
keyed branch has the same issue.
--
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]
