HappenLee commented on code in PR #68490:
URL: https://github.com/apache/doris/pull/68490#discussion_r4119074845
##########
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:
Confirmed on the current head (`f8139f5765be`). This also affects the
two-argument loop at line 151.
For `SELECT parse_url(url_column, 'HOST') FROM logs`, the old implementation
used `part_nums = part_const ? 1 : input_rows_count` and cached the parsed
enum. The new implementation calls `get_url_part(part)` for every non-NULL row;
that helper copies the part string, uppercases it, and compares it against the
supported component names. For a block of 4,096 non-NULL rows, this changes the
same constant-part conversion from 1 call to 4,096 calls. The three-argument
form with a constant `'QUERY'` has the same issue.
Please keep the `ColumnView` reuse, but cache the enum when
`part_col.is_const` is true. Initialize it lazily on the first row that
survives all argument NULL checks, so an invalid constant part does not
introduce an error for rows that should short-circuit to NULL. Dynamic parts
still need per-row conversion.
This is a confirmed increase in redundant work; I have not benchmarked the
end-to-end latency impact.
--
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]
