HappenLee commented on code in PR #68407:
URL: https://github.com/apache/doris/pull/68407#discussion_r4119122541
##########
be/src/exprs/function/function_ip.h:
##########
@@ -878,91 +859,113 @@ class FunctionIPv6CIDRToRange : public IFunction {
DataTypePtr get_return_type_impl(const DataTypes& arguments) const
override {
DataTypePtr element = std::make_shared<DataTypeIPv6>();
- return std::make_shared<DataTypeStruct>(DataTypes {element, element},
- Strings {"min", "max"});
+ DataTypePtr result = std::make_shared<DataTypeStruct>(DataTypes
{element, element},
+ Strings {"min",
"max"});
+ if (arguments[0]->is_nullable() || arguments[1]->is_nullable()) {
+ return make_nullable(result);
+ }
+ return result;
}
+ bool use_default_implementation_for_nulls() const override { return false;
}
+
Status execute_impl(FunctionContext* context, Block& block, const
ColumnNumbers& arguments,
uint32_t result, size_t input_rows_count) const
override {
const auto& addr_column_with_type_and_name =
block.get_by_position(arguments[0]);
const auto& cidr_column_with_type_and_name =
block.get_by_position(arguments[1]);
- const auto& [addr_column, add_col_const] =
- unpack_if_const(addr_column_with_type_and_name.column);
- const auto& [cidr_column, col_const] =
- unpack_if_const(cidr_column_with_type_and_name.column);
+ const auto cidr =
ColumnView<TYPE_SMALLINT>::create(cidr_column_with_type_and_name.column);
+ const auto addr_type =
addr_column_with_type_and_name.type->get_primitive_type();
- const auto* cidr_col = assert_cast<const
ColumnInt16*>(cidr_column.get());
- ColumnPtr col_res = nullptr;
-
- if (addr_column_with_type_and_name.type->get_primitive_type() ==
TYPE_IPV6) {
- const auto* ipv6_addr_column = assert_cast<const
ColumnIPv6*>(addr_column.get());
- col_res = execute_impl(*ipv6_addr_column, *cidr_col,
input_rows_count, add_col_const,
- col_const);
- } else if
(is_string_type(addr_column_with_type_and_name.type->get_primitive_type())) {
- ColumnPtr col_ipv6 =
-
convert_to_ipv6<IPConvertExceptionMode::Throw>(addr_column, nullptr);
- const auto* ipv6_addr_column = assert_cast<const
ColumnIPv6*>(col_ipv6.get());
- col_res = execute_impl(*ipv6_addr_column, *cidr_col,
input_rows_count, add_col_const,
- col_const);
+ auto col_res_lower_range = ColumnIPv6::create(input_rows_count, 0);
+ auto col_res_upper_range = ColumnIPv6::create(input_rows_count, 0);
+ auto& vec_res_lower_range = col_res_lower_range->get_data();
+ auto& vec_res_upper_range = col_res_upper_range->get_data();
+ auto null_map = ColumnUInt8::create(input_rows_count, 0);
+ auto& nulls = null_map->get_data();
+ if (addr_type == TYPE_IPV6) {
+ const auto addr =
ColumnView<TYPE_IPV6>::create(addr_column_with_type_and_name.column);
+ execute_impl(addr, cidr, input_rows_count, vec_res_lower_range,
vec_res_upper_range,
+ nulls);
+ } else if (is_string_type(addr_type)) {
+ const auto addr =
+
ColumnView<TYPE_STRING>::create(addr_column_with_type_and_name.column);
+ execute_impl(addr, cidr, input_rows_count, vec_res_lower_range,
vec_res_upper_range,
+ nulls);
} else {
return Status::RuntimeError(
"Illegal column {} of argument of function {}, Expected
IPv6 or String",
- addr_column->get_name(), get_name());
+ addr_column_with_type_and_name.column->get_name(),
get_name());
}
- block.replace_by_position(result, std::move(col_res));
+ ColumnPtr result_column = ColumnStruct::create(
+ Columns {std::move(col_res_lower_range),
std::move(col_res_upper_range)});
+ if (block.get_by_position(result).type->is_nullable()) {
+ result_column = ColumnNullable::create(std::move(result_column),
std::move(null_map));
+ }
+ block.replace_by_position(result, std::move(result_column));
return Status::OK();
}
- static ColumnPtr execute_impl(const ColumnIPv6& from_column, const
ColumnInt16& cidr_column,
- size_t input_rows_count, bool is_addr_const
= false,
- bool is_cidr_const = false) {
- auto col_res_lower_range = ColumnIPv6::create(input_rows_count, 0);
- auto col_res_upper_range = ColumnIPv6::create(input_rows_count, 0);
- auto& vec_res_lower_range = col_res_lower_range->get_data();
- auto& vec_res_upper_range = col_res_upper_range->get_data();
-
+private:
+ static void execute_impl(const ColumnView<TYPE_IPV6>& addr,
+ const ColumnView<TYPE_SMALLINT>& cidr, size_t
input_rows_count,
+ ColumnIPv6::Container& lower,
ColumnIPv6::Container& upper,
+ ColumnUInt8::Container& nulls) {
static constexpr UInt8 max_cidr_mask = IPV6_BINARY_LENGTH * 8;
+ for (size_t i = 0; i < input_rows_count; ++i) {
+ if (addr.is_null_at(i) || cidr.is_null_at(i)) {
+ nulls[i] = 1;
+ continue;
+ }
+ const auto prefix = cidr.value_at(i);
+ if (prefix < 0 || prefix > max_cidr_mask) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT, "Illegal cidr
value '{}'",
+ std::to_string(prefix));
+ }
+ const auto address = addr.value_at(i);
+ apply_cidr_mask(reinterpret_cast<const char*>(&address),
+ reinterpret_cast<char*>(&lower[i]),
reinterpret_cast<char*>(&upper[i]),
+ cast_set<UInt8>(prefix));
+ }
+ }
- if (is_addr_const) {
- for (size_t i = 0; i < input_rows_count; ++i) {
- auto cidr = cidr_column.get_int(i);
- if (cidr < 0 || cidr > max_cidr_mask) {
- throw Exception(ErrorCode::INVALID_ARGUMENT, "Illegal cidr
value '{}'",
- std::to_string(cidr));
- }
- apply_cidr_mask(from_column.get_data_at(0).data,
-
reinterpret_cast<char*>(&vec_res_lower_range[i]),
-
reinterpret_cast<char*>(&vec_res_upper_range[i]),
- cast_set<UInt8>(cidr));
+ static void execute_impl(const ColumnView<TYPE_STRING>& addr,
+ const ColumnView<TYPE_SMALLINT>& cidr, size_t
input_rows_count,
+ ColumnIPv6::Container& lower,
ColumnIPv6::Container& upper,
+ ColumnUInt8::Container& nulls) {
+ static constexpr UInt8 max_cidr_mask = IPV6_BINARY_LENGTH * 8;
+ for (size_t i = 0; i < input_rows_count; ++i) {
+ if (addr.is_null_at(i) || cidr.is_null_at(i)) {
+ nulls[i] = 1;
+ continue;
}
- } else if (is_cidr_const) {
- auto cidr = cidr_column.get_int(0);
- if (cidr < 0 || cidr > max_cidr_mask) {
+ const auto prefix = cidr.value_at(i);
+ if (prefix < 0 || prefix > max_cidr_mask) {
throw Exception(ErrorCode::INVALID_ARGUMENT, "Illegal cidr
value '{}'",
- std::to_string(cidr));
+ std::to_string(prefix));
}
- for (size_t i = 0; i < input_rows_count; ++i) {
- apply_cidr_mask(from_column.get_data_at(i).data,
-
reinterpret_cast<char*>(&vec_res_lower_range[i]),
-
reinterpret_cast<char*>(&vec_res_upper_range[i]),
- cast_set<UInt8>(cidr));
+ const auto value = addr.value_at(i);
+ if (value.size == 0) {
+ throw Exception(ErrorCode::INVALID_ARGUMENT, "Invalid IPv6
value");
}
- } else {
- for (size_t i = 0; i < input_rows_count; ++i) {
- auto cidr = cidr_column.get_int(i);
- if (cidr < 0 || cidr > max_cidr_mask) {
- throw Exception(ErrorCode::INVALID_ARGUMENT, "Illegal cidr
value '{}'",
- std::to_string(cidr));
- }
- apply_cidr_mask(from_column.get_data_at(i).data,
-
reinterpret_cast<char*>(&vec_res_lower_range[i]),
-
reinterpret_cast<char*>(&vec_res_upper_range[i]),
- cast_set<UInt8>(cidr));
+ IPv6 address = 0;
+ Int64 parsed_ipv4 = 0;
+ bool parsed = false;
+ if (try_parse_ipv4(value.begin(), value.end(), parsed_ipv4)) {
Review Comment:
Confirmed against da63e8d6a8c. For `ipv6_cidr_to_range('2001:db8::1',
cidr_column)`, the address remains a constant string, but the new loop parses
it once per non-NULL row. The previous implementation parsed the one-row nested
string once per block and reused the address. Please preserve that reuse with a
block-local cached parse, initialized only when the first non-NULL row with a
valid CIDR needs it. Parsing eagerly would reintroduce errors for invalid
constant strings when every result should be NULL. A focused
constant-address/varying-CIDR benchmark would also help quantify the
regression; I have confirmed the repeated work statically, without measuring
its wall-clock impact.
##########
be/src/exprs/function/function_ip.h:
##########
@@ -792,73 +793,67 @@ class FunctionIPv4CIDRToRange : public IFunction {
DataTypePtr get_return_type_impl(const DataTypes& arguments) const
override {
DataTypePtr element = std::make_shared<DataTypeIPv4>();
- return std::make_shared<DataTypeStruct>(DataTypes {element, element},
- Strings {"min", "max"});
+ DataTypePtr result = std::make_shared<DataTypeStruct>(DataTypes
{element, element},
+ Strings {"min",
"max"});
+ if (arguments[0]->is_nullable() || arguments[1]->is_nullable()) {
+ return make_nullable(result);
+ }
+ return result;
}
+ bool use_default_implementation_for_nulls() const override { return false;
}
Review Comment:
Confirmed against da63e8d6a8c for all three functions. Disabling the default
NULL implementation also removes its `only_null` shortcut. With one all-NULL
input and a varying peer, the new implementation allocates full output buffers
and visits every row merely to mark NULL; the old framework returned a NULL
ColumnConst. For IPv6 ranges alone, the two result arrays and null map require
approximately 33 bytes per row, excluding container overhead. Please retain an
early all-NULL return before output allocation, reusing the existing
nullable-column metadata helpers where practical, and keep the new per-row skip
for partially NULL inputs. Add coverage through the function factory/framework
path, since the new tests call execute_impl directly and do not exercise the
shortcut being replaced.
--
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]
