[Opt](lambda) Aviod redundant copies during lambda execute - #66590
[Opt](lambda) Aviod redundant copies during lambda execute#66590linrrzqqq wants to merge 2 commits into
Conversation
3d14794 to
86e0aea
Compare
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
@linrrzqqq review |
linrrzqqq
left a comment
linrrzqqq
left a comment
There was a problem hiding this comment.
Rarion independent review completed and converged.
No findings.
This result is based on an independent static code review. No builds or tests were run; refer to CI for test status.
|
run buildall |
1 similar comment
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 28625 ms |
TPC-DS: Total hot run time: 158326 ms |
ClickBench: Total hot run time: 23.91 s |
|
|
||
| if (std::ranges::any_of( | ||
| data_types, | ||
| [](const auto& type) { return !type->have_maximum_size_of_value(); }) || |
There was a problem hiding this comment.
这个函数完全没必要传data_type和加这个判断，用_has_variable_length_column(lambda_expr)就足够了啊
|
/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: two distinct batching-budget issues remain.
Critical checkpoint conclusions:
- Goal and coverage: The direct-reuse, selector mapping, const/sparse capture, first-result ownership, and nested-scope changes are generally coherent, and the new unit/regression cases cover those paths. However, the adaptive sizing goal is not met for a large variable-width capture, and the fixed-width estimator can defeat direct reuse. Missing boundary tests correspond to the two inline issues.
- Correctness and lifecycle: Selector-to-capture mapping is consistent in direct and multi-batch paths, including empty/NULL rows; nested frame visibility/shadowing and COW/nullable result assembly are sound. LambdaFunction objects are factory-created per expression and budgets are prepared once, so no new concurrency, locking, or lifecycle issue was found.
- Compatibility and parallel paths: Removing the unused base batch_size member does not affect array_filter/array_sort; legacy lambda binding remains handled. No protocol, storage-format, persistence, transaction, configuration, or observability change applies.
- Performance and memory: Blocking issues are inline: variable-width captures can still overflow ColumnString despite the byte budget, and alias-only VColumnRefs are over-counted into unnecessary batches.
- Tests: Static review only; no builds/tests were run per runner instructions. Existing SQL results are ordered and consistent, but add the two focused adaptive-budget regressions described inline.
- User focus: No additional user-provided review focus.
There was a problem hiding this comment.
Request changes: two distinct batching-budget issues remain.
Critical checkpoint conclusions:
- Goal and coverage: The direct-reuse, selector mapping, const/sparse capture, first-result ownership, and nested-scope changes are generally coherent, and the new unit/regression cases cover those paths. However, the adaptive sizing goal is not met for a large variable-width capture, and the fixed-width estimator can defeat direct reuse. Missing boundary tests correspond to the two inline issues.
- Correctness and lifecycle: Selector-to-capture mapping is consistent in direct and multi-batch paths, including empty/NULL rows; nested frame visibility/shadowing and COW/nullable result assembly are sound. LambdaFunction objects are factory-created per expression and budgets are prepared once, so no new concurrency, locking, or lifecycle issue was found.
- Compatibility and parallel paths: Removing the unused base batch_size member does not affect array_filter/array_sort; legacy lambda binding remains handled. No protocol, storage-format, persistence, transaction, configuration, or observability change applies.
- Performance and memory: Blocking issues are inline: variable-width captures can still overflow ColumnString despite the byte budget, and alias-only VColumnRefs are over-counted into unnecessary batches.
- Tests: Static review only; no builds/tests were run per runner instructions. Existing SQL results are ordered and consistent, but add the two focused adaptive-budget regressions described inline.
- User focus: No additional user-provided review focus.
| : current_bytes + additional_bytes; | ||
| }; | ||
|
|
||
| if (_has_variable_length_column(lambda_expr)) { |
There was a problem hiding this comment.
[P1] Keep expanded variable-width captures within the byte budget
This early return skips preferred_block_size_bytes exactly when a row-dependent STRING/ARRAY capture can be most expensive. For example, with batch_size = 65535, one non-const 70,000-byte string capture and a 65,535-element input array take the direct path, which repeats that string into at least 4,587,450,000 bytes before evaluating an otherwise fixed-width lambda. That exceeds ColumnString's 4,294,967,295-byte offset limit and fails a valid query even though the normal block budget is much smaller. Please include the measured get_max_row_byte_size() of expanded non-const captures (and other known variable inputs) in effective_max_rows, while retaining a conservative policy for unknown intermediates, and add a large variable-capture test that proves the work is split below the byte/offset limit.
| return _lambda_block_budget.max_rows; | ||
| } | ||
|
|
||
| size_t estimated_lambda_bytes_per_row = lambda_expr->estimate_memory(1); |
There was a problem hiding this comment.
[P2] Exclude alias-only lambda references from the byte estimate
lambda_expr->estimate_memory(1) charges every VColumnRef occurrence as a newly allocated value, and the loop below then charges the backing lambda_data again. Inside array_map the lambda is executed with a null selector, so those refs only return the existing lambda-block column. For example, 64 references to x in a fixed-width greatest(x, ... x) over 20,000 nullable INTs with a 1 MiB budget are estimated at roughly 320 bytes/row and split into about seven batches, although the single input plus result is only about 200 KiB and fits the direct path. Please make this lambda-local estimate treat forwarding refs (and the forwarding lambda wrapper) as aliases rather than materialized intermediates, without changing selector-aware estimates globally, and add an adaptive-budget test with repeated argument refs.
3 participants
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
array_mapflattens array elements into an internal block before executing its lambda expression. The previous implementation introduced several redundant copies and materializations on this path:These operations increased CPU usage and peak memory consumption, especially for large arrays, wide captured inputs, and complex lambda expressions.
This change optimizes the
array_mapexecution path by:ColumnConst.ColumnNothing.The optimization preserves array element order, nullable semantics, captured-column mapping, nested lambda behavior, and SQL result compatibility.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)