github-actions[bot] commented on code in PR #68022:
URL: https://github.com/apache/doris/pull/68022#discussion_r4015811539
##########
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;
Review Comment:
[P1] This skip is confined to `ArrayMapFunction`, but the parallel
lambda-form `array_sort` path still evaluates hidden payload from an outer-NULL
array. `ArraySortFunction` saves `outside_null_map`, then unconditionally
`std::sort`s every original row range and invokes the comparator. With
`short_circuit_evaluation=false`, a row such as `array_sort((x,y) -> if(cast(x
as int) < cast(y as int), -1, 1), if(id=1, cast(NULL as ARRAY<STRING>),
values))` still raises on two hidden non-numeric strings even though the result
row must be NULL. Please skip comparator execution for null rows too and add
the corresponding strict-cast regression.
##########
be/test/exprs/lambda_function/array_map_function_test.cpp:
##########
@@ -1015,6 +1025,102 @@ TEST(ArrayMapFunctionTest,
MultiBatchPreservesCaptureMappingAcrossSelectedArrayR
EXPECT_EQ(values.get_element(total_nested_rows - 1), 1349);
}
+TEST(ArrayMapFunctionTest,
HiddenPayloadAfterValidRowUsesOwnOffsetsAcrossBatchesAndSelector) {
+ constexpr int lambda_batch_size = 3;
+ auto int_type = std::make_shared<DataTypeInt32>();
+ auto array_int_type = std::make_shared<DataTypeArray>(int_type);
+ auto nullable_array_int_type =
std::make_shared<DataTypeNullable>(array_int_type);
+ std::vector<size_t> observed_batch_sizes;
+
+ auto root =
Review Comment:
[P1] The required Clang Formatter check is failing on this new test block;
the local clang-format 16 dry run reports violations from here through both
added tests. Please run `build-support/clang-format.sh` on the changed C++
files and push the formatted result so the mandatory style gate can pass.
##########
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:
[P1] Compacting an outer-NULL row to length 0 breaks callers that combine
this result with another logically aligned array. In a mixed two-row block this
result can have offsets `[0,2]` and null map `[1,0]` while the other array has
`[2,4]`; nullable execution still exposes those physical offsets to the nested
function. High-order `array_split`/`array_reverse_split`,
`array_zip(array_map(...), a)`, multi-argument `array_enumerate_uniq`, and
multi-array `_foreach` aggregates then reject the unequal or shifted offsets
instead of producing the required NULL row. Please make the nullable
execution/affected consumers ignore or normalize outer-NULL rows, and add mixed
NULL/non-NULL composition regressions.
--
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]
