github-actions[bot] commented on code in PR #68407:
URL: https://github.com/apache/doris/pull/68407#discussion_r4091319880
##########
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:
**[P2] Preserve the all-NULL block short circuit**
Before these overrides, `default_implementation_for_nulls` detected
`NullableColumnInfo::only_null` and returned a NULL `ColumnConst` before
entering the function. Now an all-NULL nullable input block (or a const-NULL
input with a varying peer) reaches `execute_impl`: IPv4 allocates both result
vectors plus a null map, IPv6 allocates roughly 33 bytes per row, and
`cut_ipv6` preallocates roughly 45 bytes per row, then each loops over every
row just to mark NULL. Please retain an `only_null` early return before
allocating the output buffers, shared across the three functions if possible,
while keeping the new per-row skip for partially NULL blocks.
##########
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;
}
+
Status execute_impl(FunctionContext* context, Block& block, const
ColumnNumbers& arguments,
uint32_t result, size_t input_rows_count) const
override {
- ColumnWithTypeAndName& ip_column = block.get_by_position(arguments[0]);
- ColumnWithTypeAndName& cidr_column =
block.get_by_position(arguments[1]);
-
- const auto& [ip_column_ptr, ip_col_const] =
unpack_if_const(ip_column.column);
- const auto& [cidr_column_ptr, cidr_col_const] =
unpack_if_const(cidr_column.column);
+ const auto& ip_argument = block.get_by_position(arguments[0]);
+ const auto& cidr_argument = block.get_by_position(arguments[1]);
+ const auto ip = ColumnView<TYPE_IPV4>::create(ip_argument.column);
+ const auto cidr =
ColumnView<TYPE_SMALLINT>::create(cidr_argument.column);
+ const bool result_nullable =
block.get_by_position(result).type->is_nullable();
- const auto* col_ip_column = assert_cast<const
ColumnIPv4*>(ip_column_ptr.get());
- const auto* col_cidr_column = assert_cast<const
ColumnInt16*>(cidr_column_ptr.get());
-
- const typename ColumnIPv4::Container& vec_ip_input =
col_ip_column->get_data();
- const ColumnInt16::Container& vec_cidr_input =
col_cidr_column->get_data();
auto col_lower_range_output = ColumnIPv4::create(input_rows_count, 0);
auto col_upper_range_output = ColumnIPv4::create(input_rows_count, 0);
+ auto& lower = col_lower_range_output->get_data();
+ auto& upper = col_upper_range_output->get_data();
+ ColumnUInt8::MutablePtr null_map;
+ if (result_nullable) {
+ null_map = ColumnUInt8::create(input_rows_count, 0);
+ execute_impl<true>(ip, cidr, input_rows_count, lower, upper,
&null_map->get_data());
+ } else {
+ execute_impl<false>(ip, cidr, input_rows_count, lower, upper,
nullptr);
+ }
- ColumnIPv4::Container& vec_lower_range_output =
col_lower_range_output->get_data();
- ColumnIPv4::Container& vec_upper_range_output =
col_upper_range_output->get_data();
+ ColumnPtr result_column = ColumnStruct::create(
+ Columns {std::move(col_lower_range_output),
std::move(col_upper_range_output)});
+ if (result_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();
+ }
+private:
+ template <bool ResultNullable>
+ static void execute_impl(const ColumnView<TYPE_IPV4>& ip, const
ColumnView<TYPE_SMALLINT>& cidr,
+ size_t input_rows_count, ColumnIPv4::Container&
lower,
+ ColumnIPv4::Container& upper,
ColumnUInt8::Container* nulls) {
static constexpr UInt8 max_cidr_mask = IPV4_BINARY_LENGTH * 8;
-
- if (ip_col_const) {
- auto ip = vec_ip_input[0];
- for (size_t i = 0; i < input_rows_count; ++i) {
- auto cidr = vec_cidr_input[i];
- if (cidr < 0 || cidr > max_cidr_mask) {
- throw Exception(ErrorCode::INVALID_ARGUMENT, "Illegal cidr
value '{}'",
- std::to_string(cidr));
+ for (size_t i = 0; i < input_rows_count; ++i) {
Review Comment:
**[P2] Avoid the documented slow nullable-binary ColumnView loop**
For nullable IPv4/CIDR columns this loop performs two `is_null_at` and two
`value_at` shape selections on every row before only a bounds check and a few
integer mask operations. The performance contract in `column_execute_util.h`
calls out exactly this cheap nullable multi-column pattern as inhibiting
vectorization (about 1.4x in its loop benchmarks) and recommends hand-written
access. Please keep the skip-before-read semantics, but hoist the
physical/const shapes and read the typed data/null maps directly so ordinary
nullable IPv4 range evaluation does not take this known hot-loop penalty.
--
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]
