github-actions[bot] commented on code in PR #66159:
URL: https://github.com/apache/doris/pull/66159#discussion_r3671104156
##########
be/src/exprs/function/array/function_array_map.h:
##########
@@ -191,6 +198,70 @@ struct ArrayMapImpl {
return true;
}
+ static bool _execute_nested_array(ColumnArrayMutableData& dst,
+ const ColumnArrayExecutionDatas& datas,
+ const std::vector<bool>& col_const,
size_t start_row,
+ size_t end_row) {
+ for (const auto& data : datas) {
+ if (!is_column<ColumnArray>(*data.nested_col)) {
+ return false;
+ }
+ }
+
+ size_t result_offset = 0;
+ phmap::flat_hash_set<StringRef, StringRefHash> seen;
Review Comment:
Please use Doris's allocator-aware containers for this new cardinality-sized
state. `phmap::flat_hash_set` and `std::vector` use their default allocators
here, while `doris::flat_hash_set` and `DorisVector` route allocations through
the thread query tracker. A legal one-million-element row needs about 16 MB for
the `StringRef` vector alone, plus the hash table, none of which is charged to
the query or checked against its memory limit. This is separate from the Arena
payload; switch both containers to tracked allocators.
##########
be/src/exprs/function/array/function_array_map.h:
##########
@@ -167,7 +168,13 @@ struct ArrayMapImpl {
std::vector<bool>& col_const, size_t start_row,
size_t end_row) {
ColumnArrayMutableData dst =
create_mutable_data(datas[0].nested_col.get(),
datas[0].nested_nullmap_data);
- if (_execute_internal<ALL_COLUMNS_SIMPLE>(dst, datas, col_const,
start_row, end_row)) {
+
+ bool executed =
+ _execute_internal<ALL_COLUMNS_SIMPLE>(dst, datas, col_const,
start_row, end_row);
+ if constexpr (operation == MapOperation::UNION) {
+ executed = executed || _execute_nested_array(dst, datas,
col_const, start_row, end_row);
Review Comment:
This new success path invalidates the existing p0 test at
`regression-test/suites/doc/sql-manual/sql-function/test_array_function.groovy:406-409`,
which still requires `ARRAY<ARRAY<INT>>` to be rejected. With the FE exemption
in this PR, that query reaches this branch and no longer throws, so the suite
will fail. If support is intended, update the PR contract, replace that
negative case with an auto-generated positive oracle, and add the issue's
empty-left reproduction plus nullable/duplicate/variadic cases.
##########
be/src/exprs/function/array/function_array_map.h:
##########
@@ -191,6 +198,70 @@ struct ArrayMapImpl {
return true;
}
+ static bool _execute_nested_array(ColumnArrayMutableData& dst,
+ const ColumnArrayExecutionDatas& datas,
+ const std::vector<bool>& col_const,
size_t start_row,
+ size_t end_row) {
+ for (const auto& data : datas) {
+ if (!is_column<ColumnArray>(*data.nested_col)) {
+ return false;
+ }
+ }
+
+ size_t result_offset = 0;
+ phmap::flat_hash_set<StringRef, StringRefHash> seen;
+ std::vector<StringRef> distinct_elements;
+ Arena arena;
+
+ for (size_t row = start_row; row < end_row; ++row) {
+ seen.clear();
+ distinct_elements.clear();
+ arena.clear();
+
+ bool has_null = false;
+ for (size_t arg_idx = 0; arg_idx < datas.size(); ++arg_idx) {
+ const auto& data = datas[arg_idx];
+ const size_t input_row = index_check_const(row,
col_const[arg_idx]);
+ const size_t begin = (*data.offsets_ptr)[input_row - 1];
+ const size_t end = (*data.offsets_ptr)[input_row];
+
+ for (size_t off = begin; off < end; ++off) {
+ if (data.nested_nullmap_data &&
data.nested_nullmap_data[off]) {
+ has_null = true;
+ continue;
+ }
+
+ const char* serialized_begin = nullptr;
+ StringRef key =
data.nested_col->serialize_value_into_arena(off, arena,
Review Comment:
Each occurrence is serialized before deduplication, but a failed
`seen.emplace` leaves that allocation in the Arena until the next row.
Duplicate-heavy input such as 1,000 copies of the same large inner array
therefore retains serialized bytes proportional to all occurrences even though
the result has one value, adding input-sized avoidable peak memory.
Reclaim/reuse the last serialization on duplicates (or hash/compare row
references) and cover this with a duplicate-heavy test.
##########
be/src/exprs/function/array/function_array_map.h:
##########
@@ -167,7 +168,13 @@ struct ArrayMapImpl {
std::vector<bool>& col_const, size_t start_row,
size_t end_row) {
ColumnArrayMutableData dst =
create_mutable_data(datas[0].nested_col.get(),
datas[0].nested_nullmap_data);
- if (_execute_internal<ALL_COLUMNS_SIMPLE>(dst, datas, col_const,
start_row, end_row)) {
+
Review Comment:
Please remove the spaces from this otherwise blank line and run the required
BE formatter. The authoritative patch adds a six-space-only line here;
clang-format v16 removes it, so `build-support/check-format.sh` reports a diff
and fails this file.
##########
be/src/exprs/function/array/function_array_map.h:
##########
@@ -191,6 +198,70 @@ struct ArrayMapImpl {
return true;
}
+ static bool _execute_nested_array(ColumnArrayMutableData& dst,
+ const ColumnArrayExecutionDatas& datas,
+ const std::vector<bool>& col_const,
size_t start_row,
+ size_t end_row) {
+ for (const auto& data : datas) {
+ if (!is_column<ColumnArray>(*data.nested_col)) {
+ return false;
+ }
+ }
+
+ size_t result_offset = 0;
+ phmap::flat_hash_set<StringRef, StringRefHash> seen;
+ std::vector<StringRef> distinct_elements;
+ Arena arena;
+
+ for (size_t row = start_row; row < end_row; ++row) {
+ seen.clear();
+ distinct_elements.clear();
+ arena.clear();
+
+ bool has_null = false;
+ for (size_t arg_idx = 0; arg_idx < datas.size(); ++arg_idx) {
+ const auto& data = datas[arg_idx];
+ const size_t input_row = index_check_const(row,
col_const[arg_idx]);
+ const size_t begin = (*data.offsets_ptr)[input_row - 1];
+ const size_t end = (*data.offsets_ptr)[input_row];
+
+ for (size_t off = begin; off < end; ++off) {
+ if (data.nested_nullmap_data &&
data.nested_nullmap_data[off]) {
+ has_null = true;
+ continue;
+ }
+
+ const char* serialized_begin = nullptr;
+ StringRef key =
data.nested_col->serialize_value_into_arena(off, arena,
+
serialized_begin);
+
+ if (seen.emplace(key).second) {
Review Comment:
Please do not use raw serialized bytes as the equality key.
`array_union([[CAST('0.0' AS DOUBLE)]], [[CAST('-0.0' AS DOUBLE)]])` reaches
this path; `+0.0` and `-0.0` are equal under Doris/scalar-union and
`ColumnArray::compare_at` semantics, but `ColumnVector::serialize_impl`
preserves the sign bit and `StringRef` compares with `memcmp`, so both inner
arrays are emitted. Use value-semantic hashing/equality (also canonicalizing
NaNs), and add focused regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/ArrayUnion.java:
##########
@@ -66,13 +68,24 @@ public ArrayUnion withChildren(List<Expression> children) {
return new ArrayUnion(getFunctionParams(children));
}
+
@Override
public void checkLegalityBeforeTypeCoercion() {
- DataType argType = getArgument(0).getDataType();
- if (argType.isArrayType() && (((ArrayType)
argType).getItemType().isComplexType()
- || ((ArrayType) argType).getItemType().isVariantType()
- || ((ArrayType) argType).getItemType().isJsonType())) {
- throw new AnalysisException("array_union does not support types: "
+ argType.toSql());
+ for (Expression arg : getArguments()) {
+ DataType argType = arg.getDataType();
+ if (!argType.isArrayType()) {
+ continue;
+ }
+
+ DataType itemType = ((ArrayType) argType).getItemType();
+ boolean unsupported = (itemType.isComplexType() &&
!itemType.isArrayType())
Review Comment:
Please validate nested array leaves recursively here. For a non-null VARIANT
expression `v`, `array_repeat(v, 1)` produces `ARRAY<VARIANT>` and wrapping it
again produces `ARRAY<ARRAY<VARIANT>>`; this immediate-array exemption accepts
that argument. BE then serializes the inner `ColumnArray`, reaches
`ColumnVariant::serialize_size_at`/`serialize_impl` (not implemented), and
throws `NOT_IMPLEMENTED` instead of the FE error this guard is meant to
provide. Recurse through nested `ArrayType`s and reject unsupported leaves,
with a regression for the nested `array_repeat` trigger.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/ArrayUnion.java:
##########
@@ -38,21 +38,23 @@
* ScalarFunction 'array_union'. This class is generated by GenerateFunction.
*/
public class ArrayUnion extends ScalarFunction implements
ExplicitlyCastableSignature,
- BinaryExpression, PropagateNullable {
+ BinaryExpression, PropagateNullable {
Review Comment:
Please restore the generated-function continuation indentation. Nereids
Checkstyle requires `lineWrappingIndentation=8`, but this continuation (and the
signature/condition continuations below) was shifted to four spaces; the two
blank lines before `@Override` also violate the Nereids `EmptyLineSeparator`
rule. FE Maven validation will fail before reaching the functional tests.
--
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]
