[feat](array) support trim_array function - #67397
Merged
Merged
Conversation
Implement trim_array in BE and register its Nereids signature. Add constant folding, BE tests for const-column combinations, and regression coverage for nulls, nested arrays, boundary sizes, and types. Related to apache#48203
vajaw
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
September 1, 2026 12:52
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
vajaw
changed the title
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run vault_p0 |
Contributor
Author
|
/review |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
linrrzqqq
reviewed
Sep 3, 2026
| if (UNLIKELY(size < 0)) { | ||
| return Status::InvalidArgument("size must not be negative: {}", size); | ||
| } | ||
| if (UNLIKELY(static_cast<size_t>(size) > cardinality)) { |
Collaborator
There was a problem hiding this comment.
这里估计得重写 use_default_implementation_for_nulls
外层默认的 null 处理，对于 array 列来说会替换为[], which cardinality == 0, 所以如果使用默认处理框架，L156应该会报错，得手动处理 null
linrrzqqq
reviewed
Sep 3, 2026
| Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, | ||
| uint32_t result, size_t input_rows_count) const override { | ||
| auto array_column = | ||
| block.get_by_position(arguments[0]).column->convert_to_full_column_if_const(); |
Collaborator
There was a problem hiding this comment.
use unpack_if_const and index_check_const better， you can refer to: #64175
linrrzqqq
reviewed
Sep 3, 2026
Comment on lines
+165
to
+166
| auto offset_column = ColumnInt64::create(input_rows_count, 1); | ||
| slice_array(dst, src, *offset_column, length_column.get()); |
Collaborator
There was a problem hiding this comment.
slice_array 里面看起来 array 的每个元素都调用了一次虚函数insert_from/insert_default, 这里就别用了，每行调用一次insert_range_from应该就行, 参考：
Suggested change
| auto offset_column = ColumnInt64::create(input_rows_count, 1); | |
| slice_array(dst, src, *offset_column, length_column.get()); | |
| size_t keep = cardinality - size; | |
| if (keep > 0) { | |
| result->insert_range_from(src.array_col->get_data(), offset, keep); | |
| } | |
| res_offset += keep; | |
| res_offset_data.push_back(res_offset); |
linrrzqqq
reviewed
Sep 3, 2026
Collaborator
There was a problem hiding this comment.
作为一个支持新函数的 pr，我认为这个 pr 里面非必要尽量别改动原有的test_util结构
linrrzqqq
reviewed
Sep 3, 2026
Collaborator
There was a problem hiding this comment.
需要加强一波测试，这里面 null 只和 size = 0 配对，还有需要支持(const, col), (col, const) 这种行为的测试等等
linrrzqqq
reviewed
Sep 3, 2026
| // under the License. | ||
|
|
||
| suite("test_trim_array") { | ||
| qt_trim_two "select trim_array([1, 2, 3, 4], 2)" |
Collaborator
There was a problem hiding this comment.
also need testFoldConst to double check
Collaborator
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review status: complete (converged after two rounds)
I found one material BE allocation issue and left it inline.
Checkpoint conclusions:
- Goal and proof: the new
trim_array(array, n)behavior is implemented across BE execution, Nereids binding/constant folding, visitor/registration paths, and regression/unit coverage; valid, NULL, nested, boundary, and mixed const/vector cases are represented. - Scope: all 10 changed files and the relevant framework, column, registration, and evaluator paths were reviewed. The change stays focused on the new function and its tests.
- Concurrency, lifecycle, and configuration: this stateless scalar function adds no shared mutable state, threads, lifecycle hook, or configuration surface.
- Compatibility and parallel paths: FE signature/nullability/folding and BE factory/runtime routes are aligned. No persisted format, RPC, mixed-version field, storage layout, or alternate execution registration is affected.
- Conditions and error handling: NULL precedence and negative/out-of-range validation are consistent between FE and BE; the const/nullable physical-shape and offset paths are valid on this head.
- Tests and results: the BE, FE, and regression cases cover core valid/error/null/type combinations and their expected results are consistent with the implementation. I did not run builds or tests because the review instructions prohibit builds; the PR reports the targeted BE ASAN, BE/FE unit, and regression runs passed.
- Observability, persistence, transactions, and data writes: not applicable; this function has no distributed operation, durable state, catalog mutation, transaction visibility, or data-write path.
- Performance and memory: the prior per-element virtual insertion concern is fixed by bulk range copying, but the output reserve still uses the physical source size and creates the material peak-allocation/reallocation issue described inline.
- Other: all six existing live review threads were duplicate-fenced, and no second distinct issue survived the final sweep.
User-provided focus: none beyond the full PR review.
Reviewed head: 89106bc9371f9ac6d0d6942885a6d61d20b9dfd0.
| ColumnArray::ColumnOffsets::create()); | ||
| auto& result_data = result_array->get_data(); | ||
| auto& result_offsets = result_array->get_offsets(); | ||
| result_data.reserve(src.array_col->get_data().size()); |
Contributor
There was a problem hiding this comment.
result_data.reserve uses the physical source element count before any trim size is examined. For a block that trims large numeric/nullable arrays completely, this eagerly allocates almost another full input buffer (and nullable null map) while the input is still live, even though the result data is empty; near the memory limit that can fail an otherwise tiny-result query. A const array has the opposite problem because its physical source contains one array while the logical result may repeat it for every row, so this estimate under-reserves and grows repeatedly. This is separate from the earlier per-element insertion thread. Please validate sizes/build offsets in a first pass, compute the overflow-checked total retained element count, reserve that exact total, and then bulk-copy the retained ranges.
linrrzqqq
reviewed
Sep 6, 2026
Comment on lines
+22
to
+37
| qt_trim_zero "select trim_array([1, 2, 3, 4], 0)" | ||
| qt_trim_one "select trim_array([1, 2, 3, 4], 1)" | ||
| qt_trim_all "select trim_array([1, 2, 3, 4], 4)" | ||
| qt_trim_string "select trim_array(['a', 'b', 'c', 'd'], 1)" | ||
| qt_trim_null_element "select trim_array(['a', 'b', null, 'd'], 1)" | ||
| qt_trim_nested "select trim_array([[1, 2, 3], [4, 5, 6]], 1)" | ||
| qt_trim_empty "select trim_array(cast([] as array<int>), 0)" | ||
| qt_trim_boolean "select trim_array(cast([true, false, true] as array<boolean>), 1)" | ||
| qt_trim_tinyint "select trim_array(cast([-128, 0, 127] as array<tinyint>), 1)" | ||
| qt_trim_bigint "select trim_array(cast([-9223372036854775808, 0, 9223372036854775807] as array<bigint>), 1)" | ||
| qt_trim_double "select trim_array(cast([-1.7976931348623157E308, 0.0, 1.7976931348623157E308] as array<double>), 1)" | ||
| qt_trim_decimal "select trim_array(cast([-99999999.99, 0.00, 99999999.99] as array<decimal(10, 2)>), 1)" | ||
| qt_trim_date "select trim_array(cast(['0000-01-01', '2024-02-29', '9999-12-31'] as array<date>), 1)" | ||
| qt_trim_null_array "select trim_array(cast(null as array<int>), 0)" | ||
| qt_trim_null_array_invalid_size "select trim_array(cast(null as array<int>), 9223372036854775807)" | ||
| qt_trim_null_size "select trim_array([1, 2, 3], cast(null as bigint))" |
Collaborator
There was a problem hiding this comment.
这些常量的，最好都再测一遍testFoldConst 保证 be 和 fe 常量折叠 行为一致
Contributor
Author
There was a problem hiding this comment.
感谢您的审查！两个评论已在11f8aa5c中得到处理，能否请您再看一遍？谢谢。
1、BE实现目前会在第一遍遍历中验证修剪大小并计算输出偏移量，同时进行溢出检查。随后在第二遍遍历中预留总保留元素数量，并批量复制保留的范围。
2、目前，所有17个成功的常量输入案例均采用testFoldConst来验证前端常量折叠与后端执行的一致性，涵盖NULL值、可空元素、嵌套数组、空数组及不同元素类型。
Contributor
Author
|
/review |
1 similar comment
Collaborator
|
/review |
Contributor
There was a problem hiding this comment.
Review status: complete (converged after two rounds)
No blocking findings remain on the current head. From a code-review perspective, this looks ready; I am submitting a comment-only review with no inline comments.
Checkpoint conclusions:
- Goal and proof:
trim_array(array, size)is implemented end to end across BE execution, Nereids registration, typing, constant folding, visitor/lowering paths, and focused BE/FE/regression coverage. The implementation preserves the array type and retained-element order, propagates outer NULLs, preserves nullable elements, and rejects negative or over-cardinality sizes. - Scope and reuse: all 10 changed files and the relevant function framework, column-copy, evaluator, registration, and test-harness paths were reviewed. Existing abstractions are reused and the change stays focused on the new scalar function.
- Conditions and error handling: all const/vector argument shapes, outer-NULL precedence, empty/full trims, cumulative offsets, overflow checking, nested range copying, and invalid-size errors were traced. Invalid literal fold attempts return the original expression and reach BE, so the regression error cases cover the BE branches.
- Compatibility and parallel paths: FE signatures/nullability/folding and the registered BE symbol/runtime behavior align. No persisted format, RPC field, storage layout, mixed-version protocol, catalog state, or alternate execution path is changed.
- Concurrency, lifecycle, configuration, observability, transactions, and writes: not applicable; this stateless scalar function introduces no shared mutable state, lifecycle hook, configuration surface, distributed operation, durable state, transaction boundary, or data-write path.
- Performance and memory: the implementation bulk-copies retained ranges and reserves the exact overflow-checked retained element count; the earlier per-element-copy and source-sized-reservation concerns are resolved on this head.
- Tests and results: the added tests cover value/type preservation, invalid sizes, visitor/child rewriting, all const/vector shapes, outer NULLs, nullable and nested elements, representative element types, fold-versus-runtime parity, ordering, and exact error text. I inspected the test code and expected results but did not run builds or tests because the review instructions prohibit them.
- Other: the existing live threads were treated as duplicate fences. One Round 1 test-routing candidate was independently disproved in Round 2, and no distinct issue survived the final missed-area sweep.
User-provided focus: none beyond the full PR review.
Reviewed head: 11f8aa5ca2aeb304ebe958d4be219902325a9bf3.
Collaborator
|
run buildall |
Contributor
TPC-H: Total hot run time: 16717 ms |
Contributor
TPC-DS: Total hot run time: 81782 ms |
Contributor
ClickBench: Total hot run time: 14.65 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Collaborator
|
run beut |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
924060929
approved these changes
Sep 10, 2026
HappenLee
dismissed
github-actions[bot]’s stale review
September 21, 2026 01:53
This automated review was superseded by the subsequent automated review of the current head 11f8aa5: #67397 (review) . That review explicitly confirms the earlier source-sized reservation issue is resolved and no blocking findings remain. Dismissing the stale changes-requested status so the PR can proceed through normal merge checks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: Related to #48203
Related PR:
apache/doris-website#4106
Problem Summary:
Add the
trim_array(array, n)scalar function. The BE implementationtrims trailing elements directly from array storage and avoids virtual
function calls in the hot loop.
Register the function and its signature in the Nereids planner. Add FE
constant folding so constant invocations can be evaluated during
planning.
Cover normal and constant columns, NULL values, nullable elements,
nested arrays, multiple element types, zero, negative and out-of-range
sizes, and the maximum BIGINT value.
Release note
Add the TRIM_ARRAY scalar function.
Check List (For Author)
Test results:
BE ASAN build passed.
FunctionArrayTrimTest.all_argument_combinationspassed.FE unit tests passed: 2 tests, 0 failures and 0 errors.
The trim_array regression suite passed with no mismatches.
run-be-ut.shandrun-regression-test.shcompletedsuccessfully for the relevant tests.
Behavior changed:
trim_arrayscalar function.Does this need documentation?
[doc] Add trim_array function documentation doris-website#4106
Check List (For Reviewer who merge this PR)