[feature](function) Add array_except_all scalar function - #67132
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Add array_except_all for ARRAY<scalar> arguments. The function applies multiset difference semantics, preserves unmatched left-side duplicates and order, handles nullable elements and constant columns through ColumnArrayView, and rejects complex element types. Add BE unit coverage and regression coverage for scalar types, nulls, constants, duplicates, and unsupported nested types.
### Release note
Add array_except_all for arrays with scalar element types.
### Check List (For Author)
- Test: Regression test
- Built BE with ASAN
- Built FE
- Ran test_array_except_all regression suite
- Behavior changed: Yes, adds the array_except_all scalar function
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: the core loop implements saturating multiset subtraction in left-input order, and the registration/include wiring is coherent, but five distinct issues remain.
Goal and scope: the PR is focused on array_except_all; the generic supported-type path, per-row count reset, NULL multiplicity, result offsets, and string-key lifetime are otherwise sound.
Correctness and compatibility: FE admits ARRAY<VARBINARY> even though BE cannot dispatch it, and FLOAT/DOUBLE NaNs use equality semantics inconsistent with Doris. There is no storage-format, protocol, rolling-upgrade, transaction, persistence, or data-write change.
Concurrency, lifecycle, and configuration: the implementation is function-local and introduces no shared mutable state, special lifecycle, or new configuration. The existing constant-fold control exposes the all-constant execution path described inline.
Parallel/special paths: FE registration, scalar visitor fallback, BE factory registration, default outer-NULL handling, common-type coercion, one-constant execution, source discovery, and unity/direct-include hygiene were traced. The both-constant path unnecessarily bypasses expression/framework caching and expands the result once per block row.
Performance and memory: the user-proportional count map bypasses Doris query-memory accounting, and the all-constant override can multiply CPU and result memory by the block row count.
Tests and results: the added expected values cover duplicates, NULLs, row reset, constants, coercion, and several scalar families, but they do not prove unsorted left-order preservation, NaN cancellation, the VARBINARY boundary, or multi-row all-constant result shape. No build or test binary was run because the review bundle prohibits builds; the repository's pure-text build-hygiene check passed.
Observability: no additional logging or metrics are required once the execution failures and resource-accounting defects are fixed. User focus: no additional focus point was supplied.
Review completion: three independent review rounds converged with no new finding in the final round. All surviving candidates are represented by the four inline comments below (the two line-40 defects share one anchor).
| struct ArrayExceptAllCountMap { | ||
| using ElementType = typename ColumnElementView<PType>::ElementType; | ||
| using KeyType = typename NativeType<ElementType>::Type; | ||
| using Type = phmap::flat_hash_map<KeyType, size_t>; |
There was a problem hiding this comment.
Please replace both direct phmap aliases with Doris's allocator-aware map and use float hash/equality consistent with Doris. There are two independent defects here: (1) FLOAT/DOUBLE use ordinary equality, so a right-side NaN cannot be found by the left lookup and array_except_all(array(cast('NaN' as double)), array(cast('NaN' as double))) incorrectly retains it; use the normalized hash plus doris::EqualTo. (2) The default std::allocator leaves this user-sized scratch table, and the string specialization below, outside query MemTracker accounting; doris::flat_hash_map routes allocation through tracking Allocator::alloc. Please fix both aliases and add NaN/accounting coverage.
| String get_name() const override { return name; } | ||
| bool is_variadic() const override { return false; } | ||
| size_t get_number_of_arguments() const override { return 2; } | ||
| bool use_default_implementation_for_constants() const override { return false; } |
There was a problem hiding this comment.
Please keep the framework's default all-constant execution path. This opt-out also makes VectorizedFnCall::is_constant() false, disabling open-time constant-result caching. When FE folding is skipped, two ColumnConst arrays retain the live block's logical row count in ColumnArrayView, so the loop rebuilds the same map and appends the same potentially large result for every row instead of executing once and returning a ColumnConst. Removing the override still leaves one-constant calls on the normal implementation because the framework unwraps only when all arguments are constant; add a multi-row, both-constant BE test that checks the nontrivial value and constant result shape.
| continue; | ||
| } | ||
| DataType itemType = ((ArrayType) argumentType).getItemType(); | ||
| if (itemType.isComplexType() || itemType.isVariantType() || itemType.isJsonType()) { |
There was a problem hiding this comment.
Please keep the FE-accepted element families aligned with the BE implementation. VarBinaryType is primitive, so array_except_all(array(X'AB'), array(X'AB')) binds through this check and the ARRAY<AnyDataType(0)> signature, but BE's dispatch_switch_all has no TYPE_VARBINARY case and returns InvalidArgument only at execution. Either reject unsupported primitive/object families here during analysis or implement the missing BE family, and add a regression for the boundary.
|
|
||
| suite("test_array_except_all") { | ||
| order_qt_partial_cancel """ | ||
| select array_sort(array_except_all(['a', 'a', 'b'], ['a'])) |
There was a problem hiding this comment.
Please add one non-monotonic assertion without array_sort to cover the advertised left-order guarantee, for example array_except_all([3, 1, 2, 1], [1]) = [3, 2, 1]. As written, every nontrivial result is sorted (and the BE unit inputs are already ordered), so an implementation that emits hash/sorted order would pass the whole new suite.
|
run buildall |
TPC-H: Total hot run time: 16797 ms |
TPC-DS: Total hot run time: 81939 ms |
ClickBench: Total hot run time: 14.61 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
2 participants
Problem Summary: Add array_except_all for ARRAY arguments. The function applies multiset difference semantics, preserves unmatched left-side duplicates and order, handles nullable elements and constant columns through ColumnArrayView, and rejects complex element types. Add BE unit coverage and regression coverage for scalar types, nulls, constants, duplicates, and unsupported nested types.
doc apache/doris-website#4089