HappenLee commented on code in PR #68022:
URL: https://github.com/apache/doris/pull/68022#discussion_r4044340294
##########
be/src/exprs/lambda_function/varray_map_function.cpp:
##########
@@ -144,35 +144,90 @@ class ArrayMapFunction : public LambdaFunction {
// here is the array column
const auto& col_array = assert_cast<const
ColumnArray&>(*column_array);
+ lambda_offsets[i] = col_array.get_offsets_ptr();
if (i == 0) {
nested_array_column_rows = col_array.get_data_ptr()->size();
- first_array_offsets = col_array.get_offsets_ptr();
const auto& off_data = col_array.get_offsets_column();
array_column_offset =
off_data.clone_resized(col_array.get_offsets_column().size());
args_info.offsets_ptr = &col_array.get_offsets();
- } else {
- // select array_map((x,y)->x+y,c_array1,[0,1,2,3]) from
array_test2;
- // c_array1: [0,1,2,3,4,5,6,7,8,9]
- const auto& array_offsets =
- assert_cast<const
ColumnArray::ColumnOffsets&>(*first_array_offsets)
- .get_data();
- if (nested_array_column_rows !=
col_array.get_data_ptr()->size() ||
- (!array_offsets.empty() &&
- memcmp(array_offsets.data(),
col_array.get_offsets().data(),
- sizeof(array_offsets[0]) * array_offsets.size())
!= 0)) {
- return Status::InvalidArgument(
- "in array map function, the input column size "
- "are "
- "not equal completely, nested column data rows 1st
size is {}, {}th "
- "size is {}.",
- nested_array_column_rows, i + 1,
col_array.get_data_ptr()->size());
- }
}
lambda_datas[i] = col_array.get_data_ptr();
const auto& col_type = assert_cast<const
DataTypeArray&>(*type_array);
lambda_argument_types[i] = col_type.get_nested_type();
}
+
+ const auto& first_array_offsets =
+ assert_cast<const
ColumnArray::ColumnOffsets&>(*lambda_offsets[0]).get_data();
+ const auto& outside_null_map_data = outside_null_map->get_data();
+ const bool has_outer_null =
+ std::ranges::any_of(outside_null_map_data, [](uint8_t is_null)
{ return is_null; });
+ bool has_hidden_nested_data = false;
+ if (!has_outer_null) {
+ // select array_map((x,y)->x+y,c_array1,[0,1,2,3]) from
array_test2;
+ // c_array1: [0,1,2,3,4,5,6,7,8,9]
+ for (int i = 1; i < arguments.size(); ++i) {
+ const auto& offsets =
+ assert_cast<const
ColumnArray::ColumnOffsets&>(*lambda_offsets[i])
+ .get_data();
+ if (nested_array_column_rows != lambda_datas[i]->size() ||
+ (!first_array_offsets.empty() &&
+ memcmp(first_array_offsets.data(), offsets.data(),
+ sizeof(first_array_offsets[0]) *
first_array_offsets.size()) != 0)) {
+ return Status::InvalidArgument(
+ "in array map function, the input column size are
not equal "
+ "completely, nested column data rows 1st size is
{}, {}th size is {}.",
+ nested_array_column_rows, i + 1,
lambda_datas[i]->size());
+ }
+ }
+ } else {
+ std::vector<size_t> previous_offsets(arguments.size(), 0);
+ for (size_t row = 0; row < count; ++row) {
+ const size_t first_row_size = first_array_offsets[row] -
previous_offsets[0];
+ has_hidden_nested_data |= outside_null_map_data[row] != 0 &&
first_row_size > 0;
+ for (int i = 1; i < arguments.size(); ++i) {
+ const auto& offsets =
+ assert_cast<const
ColumnArray::ColumnOffsets&>(*lambda_offsets[i])
+ .get_data();
+ const size_t row_size = offsets[row] - previous_offsets[i];
+ has_hidden_nested_data |= outside_null_map_data[row] != 0
&& row_size > 0;
+ if (outside_null_map_data[row] == 0 && first_row_size !=
row_size) {
+ return Status::InvalidArgument(
+ "in array map function, the input column size
are not equal "
+ "completely at row {}, 1st size is {}, {}th
size is {}.",
+ row, first_row_size, i + 1, row_size);
+ }
+ previous_offsets[i] = offsets[row];
+ }
+ previous_offsets[0] = first_array_offsets[row];
+ }
+ }
+
+ // NULL rows are skipped. If they retain hidden payload, rebuild only
result offsets;
+ // the bounded execution path reads each argument through its own
original offsets.
+ if (has_hidden_nested_data) {
+ auto res_offsets = ColumnArray::ColumnOffsets::create();
+ auto& res_offsets_data = res_offsets->get_data();
+ res_offsets_data.reserve(count);
+ size_t previous_offset = 0;
+ size_t compacted_rows = 0;
+ for (size_t row = 0; row < count; ++row) {
+ const size_t current_offset = first_array_offsets[row];
+ if (outside_null_map_data[row] == 0) {
+ const size_t row_size = current_offset - previous_offset;
+ compacted_rows += row_size;
+ }
+ res_offsets_data.push_back(compacted_rows);
Review Comment:
The `array_zip` and `array_split`/`array_reverse_split` composition cases
now pass on `0885d6b34062d8836ddd3d3c892a72c637ec2131`, but the
**multi-argument `array_enumerate_uniq` case still fails**.
For a two-row nullable array `a`, consider this valid physical
representation:
```text
a:
data = [1, 2, 3, 4]
offsets = [2, 4]
null_map = [1, 0]
array_map(x -> x, a):
data = [3, 4]
offsets = [0, 2]
null_map = [1, 0]
```
Both columns logically contain `NULL` and `[3,4]`. The first row's hidden
payload can arise from `IF(condition, NULL, a)`:
`VectorizedIfExpr::execute_for_null_then_else` can retain the array payload
while changing its outer null map.
However, this composition fails:
```sql
array_enumerate_uniq(array_map(x -> x, a), a)
```
Expected rows: `NULL`, then `[1,1]`. Actual error:
```text
[RUNTIME_ERROR]lengths of all arrays of function array_enumerate_uniq must
be equal.
```
The remaining assumption is in
[`FunctionArrayEnumerateUniq::execute_impl`](https://github.com/apache/doris/blob/0885d6b34062d8836ddd3d3c892a72c637ec2131/be/src/exprs/function/array/function_array_enumerate_uniq.cpp#L138):
it compares the complete physical offsets (`*offsets != cur_offsets`), so
`[0,2]` versus `[2,4]` is rejected even though the only non-NULL row has length
2 in both arguments. The [default nullable
wrapper](https://github.com/apache/doris/blob/0885d6b34062d8836ddd3d3c892a72c637ec2131/be/src/exprs/function/function.cpp#L234)
unwraps the nested columns without filtering out mixed NULL/non-NULL rows; the
function errors before the result can be wrapped with the null map.
I reproduced this in a local ASAN BE test using the actual `array_map`
lambda expression and `array_enumerate_uniq` through `SimpleFunctionFactory`.
The control `array_enumerate_uniq(a, a)` succeeds; replacing its first argument
with the actual mapped column triggers the error. This verifies the BE
composition, rather than an end-to-end SQL regression run.
Could we also handle this consumer before merging and add a mixed
NULL/non-NULL composition regression? With the compact-output approach, it
needs to skip outer-NULL rows, validate lengths on visible rows, and read each
argument using its own row offsets (or normalize the inputs equivalently).
Simply removing the offsets equality check is insufficient:
[`_execute_by_hash`](https://github.com/apache/doris/blob/0885d6b34062d8836ddd3d3c892a72c637ec2131/be/src/exprs/function/array/function_array_enumerate_uniq.cpp#L225)
also assumes a shared physical element index across the input columns.
--
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]
