github-actions[bot] commented on code in PR #68022:
URL: https://github.com/apache/doris/pull/68022#discussion_r4056326757
##########
be/src/exprs/function/array/function_array_enumerate_uniq.cpp:
##########
@@ -116,36 +121,114 @@ class FunctionArrayEnumerateUniq : public IFunction {
Status execute_impl(FunctionContext* context, Block& block, const
ColumnNumbers& arguments,
uint32_t result, size_t input_rows_count) const
override {
- ColumnRawPtrs data_columns(arguments.size());
- const ColumnArray::Offsets64* offsets = nullptr;
- ColumnPtr src_offsets;
- Columns src_columns; // to keep ownership
+ for (const auto argument : arguments) {
+ if (block.get_by_position(argument).column->only_null()) {
+ auto& result_column = block.get_by_position(result);
+ result_column.column =
+
result_column.type->create_column_const(input_rows_count, Field());
+ return Status::OK();
+ }
+ }
- const ColumnArray* first_column_array = nullptr;
+ ColumnUInt8::MutablePtr result_null_map;
+ ColumnUInt8::Container* result_null_map_data = nullptr;
+ if (block.get_by_position(result).type->is_nullable()) {
+ result_null_map = ColumnUInt8::create(input_rows_count, 0);
+ result_null_map_data = &result_null_map->get_data();
+ }
- for (size_t i = 0; i < arguments.size(); i++) {
- src_columns.emplace_back(
-
block.get_by_position(arguments[i]).column->convert_to_full_column_if_const());
- ColumnPtr& cur_column = src_columns[i];
- const ColumnArray* array =
-
check_and_get_column<ColumnArray>(remove_nullable(cur_column->get_ptr()).get());
+ std::vector<const ColumnArray*> array_columns(arguments.size());
+ Columns src_columns;
+ src_columns.reserve(arguments.size());
+ for (size_t i = 0; i < arguments.size(); ++i) {
+ auto cur_column =
+
block.get_by_position(arguments[i]).column->convert_to_full_column_if_const();
+ if (const auto* nullable =
check_and_get_column<ColumnNullable>(cur_column.get())) {
+ VectorizedUtils::update_null_map(*result_null_map_data,
+
nullable->get_null_map_data());
+ cur_column = nullable->get_nested_column_ptr();
+ }
+ src_columns.emplace_back(std::move(cur_column));
+ const auto* array =
check_and_get_column<ColumnArray>(src_columns.back().get());
if (!array) {
return Status::RuntimeError(
fmt::format("Illegal column {}, of first argument of
function {}",
- cur_column->get_name(), get_name()));
+ src_columns.back()->get_name(),
get_name()));
}
+ array_columns[i] = array;
+ }
- const ColumnArray::Offsets64& cur_offsets = array->get_offsets();
- if (i == 0) {
- first_column_array = array;
- offsets = &cur_offsets;
- src_offsets = array->get_offsets_ptr();
- } else if (*offsets != cur_offsets) {
- return Status::RuntimeError(fmt::format(
- "lengths of all arrays of function {} must be equal.",
get_name()));
+ bool has_hidden_nested_data = false;
+ if (result_null_map_data != nullptr) {
+ for (size_t row = 0; row < input_rows_count &&
!has_hidden_nested_data; ++row) {
+ if (!(*result_null_map_data)[row]) {
+ continue;
+ }
+ for (const auto* array : array_columns) {
+ const auto& current_offsets = array->get_offsets();
+ if (current_offsets[row] != current_offsets[row - 1]) {
Review Comment:
[P2] Keep aligned hidden rows on the zero-copy path
This flag becomes true for any retained payload in a NULL row, even with one
argument or when every argument already has identical physical offsets. Those
shapes do not need alignment repair: shared indexing is already safe and the
outer result null map hides that row. Instead, lines 194-221 clone each nested
column and copy every visible element before hashing; the parallel `array_zip`
branch does the same before it could otherwise reference the input columns
directly. One small hidden NULL row can therefore add a whole-block copy (for
example, about 64 MiB per INT argument for 4096 rows of 4096 elements) and
breach the query memory limit. Please retain the shared-offset fast path for
single/equal-offset materialized inputs and compact only when ignored NULL rows
actually make the inputs diverge, with an allocation or pointer-reuse
regression.
##########
be/src/exprs/function/array/function_array_enumerate_uniq.cpp:
##########
@@ -116,36 +121,114 @@ class FunctionArrayEnumerateUniq : public IFunction {
Status execute_impl(FunctionContext* context, Block& block, const
ColumnNumbers& arguments,
uint32_t result, size_t input_rows_count) const
override {
- ColumnRawPtrs data_columns(arguments.size());
- const ColumnArray::Offsets64* offsets = nullptr;
- ColumnPtr src_offsets;
- Columns src_columns; // to keep ownership
+ for (const auto argument : arguments) {
+ if (block.get_by_position(argument).column->only_null()) {
+ auto& result_column = block.get_by_position(result);
+ result_column.column =
+
result_column.type->create_column_const(input_rows_count, Field());
+ return Status::OK();
+ }
+ }
- const ColumnArray* first_column_array = nullptr;
+ ColumnUInt8::MutablePtr result_null_map;
+ ColumnUInt8::Container* result_null_map_data = nullptr;
+ if (block.get_by_position(result).type->is_nullable()) {
+ result_null_map = ColumnUInt8::create(input_rows_count, 0);
+ result_null_map_data = &result_null_map->get_data();
+ }
- for (size_t i = 0; i < arguments.size(); i++) {
- src_columns.emplace_back(
-
block.get_by_position(arguments[i]).column->convert_to_full_column_if_const());
- ColumnPtr& cur_column = src_columns[i];
- const ColumnArray* array =
-
check_and_get_column<ColumnArray>(remove_nullable(cur_column->get_ptr()).get());
+ std::vector<const ColumnArray*> array_columns(arguments.size());
+ Columns src_columns;
+ src_columns.reserve(arguments.size());
+ for (size_t i = 0; i < arguments.size(); ++i) {
+ auto cur_column =
+
block.get_by_position(arguments[i]).column->convert_to_full_column_if_const();
+ if (const auto* nullable =
check_and_get_column<ColumnNullable>(cur_column.get())) {
+ VectorizedUtils::update_null_map(*result_null_map_data,
+
nullable->get_null_map_data());
+ cur_column = nullable->get_nested_column_ptr();
+ }
+ src_columns.emplace_back(std::move(cur_column));
+ const auto* array =
check_and_get_column<ColumnArray>(src_columns.back().get());
if (!array) {
return Status::RuntimeError(
fmt::format("Illegal column {}, of first argument of
function {}",
- cur_column->get_name(), get_name()));
+ src_columns.back()->get_name(),
get_name()));
}
+ array_columns[i] = array;
+ }
- const ColumnArray::Offsets64& cur_offsets = array->get_offsets();
- if (i == 0) {
- first_column_array = array;
- offsets = &cur_offsets;
- src_offsets = array->get_offsets_ptr();
- } else if (*offsets != cur_offsets) {
- return Status::RuntimeError(fmt::format(
- "lengths of all arrays of function {} must be equal.",
get_name()));
+ bool has_hidden_nested_data = false;
+ if (result_null_map_data != nullptr) {
+ for (size_t row = 0; row < input_rows_count &&
!has_hidden_nested_data; ++row) {
+ if (!(*result_null_map_data)[row]) {
+ continue;
+ }
+ for (const auto* array : array_columns) {
+ const auto& current_offsets = array->get_offsets();
+ if (current_offsets[row] != current_offsets[row - 1]) {
+ has_hidden_nested_data = true;
+ break;
+ }
+ }
}
- const auto* array_data = &array->get_data();
- data_columns[i] = array_data;
+ }
+
+ ColumnRawPtrs data_columns(arguments.size());
+ const ColumnArray::Offsets64* offsets = nullptr;
+ ColumnPtr result_offsets;
+ MutableColumns compacted_data_columns;
+ if (!has_hidden_nested_data) {
+ offsets = &array_columns[0]->get_offsets();
+ result_offsets = array_columns[0]->get_offsets_ptr();
+ for (size_t i = 0; i < arguments.size(); ++i) {
+ if (i > 0 && *offsets != array_columns[i]->get_offsets()) {
+ return Status::RuntimeError(fmt::format(
+ "lengths of all arrays of function {} must be
equal.", get_name()));
+ }
+ data_columns[i] = &array_columns[i]->get_data();
+ }
+ } else {
+ // Outer-NULL rows may retain payload and shift each input's
physical offsets.
+ // Compact visible rows so the hash key getter can keep using one
element index.
+ compacted_data_columns.resize(arguments.size());
+ for (size_t i = 0; i < arguments.size(); ++i) {
+ compacted_data_columns[i] =
array_columns[i]->get_data().clone_empty();
+ }
+
+ auto compacted_offsets = ColumnArray::ColumnOffsets::create();
+ auto& compacted_offsets_data = compacted_offsets->get_data();
+ compacted_offsets_data.reserve(input_rows_count);
+ size_t compacted_offset = 0;
+ for (size_t row = 0; row < input_rows_count; ++row) {
+ if ((*result_null_map_data)[row]) {
+ compacted_offsets_data.push_back(compacted_offset);
+ continue;
+ }
+
+ size_t row_size = 0;
+ for (size_t i = 0; i < arguments.size(); ++i) {
+ const auto& current_offsets =
array_columns[i]->get_offsets();
+ const size_t row_begin = current_offsets[row - 1];
+ const size_t current_row_size = current_offsets[row] -
row_begin;
+ if (i == 0) {
+ row_size = current_row_size;
+ } else if (current_row_size != row_size) {
+ return Status::RuntimeError(fmt::format(
+ "lengths of all arrays of function {} must be
equal.", get_name()));
+ }
+
compacted_data_columns[i]->insert_range_from(array_columns[i]->get_data(),
Review Comment:
[P2] Validate all visible lengths before compacting
This loop copies each argument's slice as soon as that argument is checked,
before the remaining inputs and rows have been validated. With divergent hidden
offsets and a mismatch in the last visible row, `array_enumerate_uniq` can
therefore clone nearly the entire block (and the first input's final slice)
before returning the required length error; `array_zip` has the same ordering.
For 4096 rows of 4096 INTs and two inputs, that is about 128 MiB copied for a
call that must fail, with strings or nested values costing more. Please make an
offset-only, const-aware validation pass over union-visible rows before
allocating or inserting into compacted columns, and add a late-row mismatch
regression.
##########
be/src/exprs/aggregate/aggregate_function_foreach.h:
##########
@@ -268,22 +268,41 @@ class AggregateFunctionForEach : public
AggregateFunctionNonFinalBase,
size_t begin = offsets[row_num - 1];
size_t end = offsets[row_num];
+ const size_t row_size = end - begin;
+ bool offsets_aligned = true;
/// Sanity check. NOTE We can implement specialization for a case with
single argument, if the check will hurt performance.
for (size_t i = 1; i < num_arguments; ++i) {
const auto& ith_column =
assert_cast<const ColumnArray&,
TypeCheckOnRelease::DISABLE>(*columns[i]);
const auto& ith_offsets = ith_column.get_offsets();
+ const size_t ith_begin = ith_offsets[row_num - 1];
+ const size_t ith_end = ith_offsets[row_num];
- if (ith_offsets[row_num] != end ||
- (row_num != 0 && ith_offsets[row_num - 1] != begin)) {
+ if (ith_end - ith_begin != row_size) {
throw Exception(ErrorCode::INTERNAL_ERROR,
"Arrays passed to {} aggregate function have
different sizes",
get_name());
}
+ offsets_aligned &= ith_begin == begin;
}
- AggregateFunctionForEachData& state = ensure_aggregate_data(place, end
- begin, arena);
+ std::vector<ColumnPtr> compacted_nested;
+ if (!offsets_aligned && row_size != 0) {
+ // The nested aggregate accepts one shared index, so align only
mismatched row slices.
+ compacted_nested.reserve(num_arguments);
+ for (size_t i = 0; i < num_arguments; ++i) {
+ const auto& ith_column =
+ assert_cast<const ColumnArray&,
TypeCheckOnRelease::DISABLE>(*columns[i]);
+ const size_t ith_begin = ith_column.get_offsets()[row_num - 1];
+ compacted_nested.emplace_back(nested[i]->cut(ith_begin,
row_size));
Review Comment:
[P2] Avoid deep-copying every misaligned row in the _foreach hot path
After a skipped outer-NULL row shifts the arguments' cumulative offsets,
`offsets_aligned` stays false for each later visible row until another
hidden-row delta happens to restore alignment. This branch then calls
`IColumn::cut` for every argument on every affected aggregate `add`; `cut`
always clones a column and `insert_range_from`s the whole slice. A block with
mapped offsets `[0,1,2,...]` beside original offsets `[M,M+1,M+2,...]`
therefore performs `num_arguments * rows` temporary allocations and copies
(including all string/nested payload bytes) before doing the actual
aggregation. Please normalize mismatched inputs once per block or use an
offset/slice-aware nested entry point that avoids owning per-row cuts, and add
a many-row allocation/performance regression.
--
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]
