Repository navigation
Conversation
|
@linrrzqqq PTAL |
1 similar comment
|
@linrrzqqq PTAL |
linrrzqqq
force-pushed
the
opt-array-map
branch
from
August 7, 2026 10:35
9f5c11a to
3c64273
Compare
|
@linrrzqqq PTAL |
linrrzqqq
commented
Aug 7, 2026
linrrzqqq
left a comment
Owner
Author
There was a problem hiding this comment.
Rarion 独立 Review 已完成并收敛。
发现 3 个可证实问题,详见行级评论:
- [HIGH]
be/src/exprs/lambda_function/varray_map_function.cpp:449为固定宽度外部函数保留运行时行数上限 - [MEDIUM]
be/src/exprs/lambda_function/varray_map_function.cpp:456计入缓存常量 lambda 的全量结果物化 - [MEDIUM]
be/src/exprs/lambda_function/varray_map_function.cpp:474按实际 POD 容量限制 32 MiB 批次
本次结果来自独立的静态代码审查;未运行构建或测试,测试状态请以 CI 为准。
linrrzqqq
force-pushed
the
opt-array-map
branch
from
August 7, 2026 13:20
3c64273 to
e22fed5
Compare
linrrzqqq
commented
Aug 7, 2026
linrrzqqq
left a comment
Owner
Author
There was a problem hiding this comment.
Rarion independent review completed and converged.
Found 1 substantiated issue(s). See the inline comments:
- [P1]
be/src/exprs/lambda_function/varray_map_function.cpp:271Restore bounded capture expansion
This result is based on an independent static code review. No builds or tests were run; refer to CI for test status.
linrrzqqq
force-pushed
the
opt-array-map
branch
from
August 7, 2026 14:21
e22fed5 to
350d1ba
Compare
|
@linrrzqqq review |
linrrzqqq
commented
Aug 7, 2026
linrrzqqq
left a comment
Owner
Author
There was a problem hiding this comment.
Rarion independent review completed and converged.
Found 1 substantiated issue(s). See the inline comments:
- [P2]
be/src/exprs/lambda_function/varray_map_function.cpp:348Include capture-index memory in the batch budget
This result is based on an independent static code review. No builds or tests were run; refer to CI for test status.
linrrzqqq
force-pushed
the
opt-array-map
branch
from
August 7, 2026 20:55
350d1ba to
3244e20
Compare
linrrzqqq
pushed a commit
that referenced
this pull request
Aug 21, 2026
…66841) ### What problem does this PR solve? support two phase in distributed execution for lance vector_search. 1. lance_scan_node will query nearest with query_vector and prefilter in lance rust, will return _row_id, _distance and some postfilter column from lance scan result, and execute the where postfilter. 2. then local topn and global merge topn will get top_k rows. 3. MaterializeNode will use _row_id to fetch the payload project column from lance rust. ``` mysql> desc SELECT row_id, category, label, _distance FROM vector_search( "table"="test_lance_vector_search_two_phase.doris.vs_ivf_pq_f32", "column"="embedding", "query_vector"="[0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15]", "top_k"="3", "metric"="l2", "nprobes"="4", "refine_factor"="10", "use_index"="true") WHERE category = 'odd' ORDER BY _distance; +-----------------------------------------------------------------------------------------------------------------------------------------------------------+ | Explain String(Nereids Planner) | +-----------------------------------------------------------------------------------------------------------------------------------------------------------+ | PLAN FRAGMENT 0 | | OUTPUT EXPRS: | | row_id[apache#17] | | category[apache#18] | | label[apache#19] | | _distance[apache#20] | | PARTITION: UNPARTITIONED | | | | HAS_COLO_PLAN_NODE: false | | | | VRESULT SINK | | MYSQL_PROTOCOL | | | | 4:VSORT(193) | | | order by: _distance[apache#20] ASC | | | algorithm: full sort | | | offset: 0 | | | distribute expr lists: | | | | | 3:VMaterializeNode | | | materialize tuple id:3 | | | output tuple id:4 | | | projectList:[row_id, category, label, _distance] | | | column_descs_lists[[`row_id` bigint NOT NULL, `label` text NOT NULL]] | | | locations: [[2, 3]] | | | column_idxs_lists: [[0, 2]] | | | row_ids: [__DORIS_GLOBAL_ROWID_COL__vector_search] | | | isTopMaterializeNode: true | | | final projections: row_id[apache#11], category[apache#9], label[apache#12], _distance[apache#10] | | | final project output tuple id: 4 | | | | | 2:VMERGING-EXCHANGE | | offset: 0 | | limit: 3 | | distribute expr lists: | | | | PLAN FRAGMENT 1 | | | | PARTITION: RANDOM | | | | HAS_COLO_PLAN_NODE: false | | | | STREAM DATA SINK | | EXCHANGE ID: 02 | | UNPARTITIONED | | IS_MERGE: true | | | | 1:VTOP-N(182) | | | order by: _distance[apache#7] ASC | | | algorithm: heap sort | | | local merge sort | | | merge by exchange | | | offset: 0 | | | limit: 3 | | | distribute expr lists: | | | | | 0:VLANCE_SCAN_NODE(174) | | table: null.null.VectorSearchTableValuedFunction<test_lance_vector_search_two_phase.doris.vs_ivf_pq_f32> | | predicates: (_tvf_vector_search.category[#0] = 'odd') | | inputSplitNum=2, totalFileSize=0, scanRanges=2 | | partition=0/0 | | numNodes=1 | | pushdown agg=NONE | | externalSearchType=VECTOR | | lanceVectorColumn=embedding | | lanceTopK=3 | | lanceOffset=0 | | lanceMetric=l2 | | lanceVersion=3 | | lanceSearchFragments=2 | | final projections: _tvf_vector_search.category[#0], _tvf_vector_search._distance[#1], _tvf_vector_search.__DORIS_GLOBAL_ROWID_COL__vector_search[apache#2] | | final project output tuple id: 1 | | | | | | | | ========== STATISTICS ========== | +-----------------------------------------------------------------------------------------------------------------------------------------------------------+ 76 rows in set (0.04 sec) mysql> ``` ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [x] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
linrrzqqq
added a commit
that referenced
this pull request
Aug 26, 2026
…ExprId (apache#67047) ### What problem does this PR solve? Related PR: apache#57204 Problem Summary: #### 1. Captured arguments from outer Lambdas could not be resolved Lambda arguments were associated with their bound arrays by argument name, and `visitArrayItemSlot` only searched the innermost Lambda scope. For example: ```sql SELECT array_map( m -> array_map( x -> element_at(map_values(m)[1], 'a') + x, [1]), maps) FROM t; ``` The inner Lambda captures m from the outer Lambda. The collector should produce:`s.data.*.VALUES.a` #### 2. `element_at` did not collect payload access for Lambda index/key arguments For example: ```sql SELECT id, array_map( (nested, i) -> element_at(nested, i), nested_arrays, indexes ) AS result FROM repro_lambda_access_path WHERE indexes IS NOT NULL ORDER BY id; -- ERROR 1105 (HY000): errCode = 2, detailMessage = (127.0.0.1)[INVALID_ARGUMENT]in array map function, the input column size are not equal completely, nested column data rows 1st size is 5, 2th size is 0. ``` In `EXPLAIN VERBOSE`: ```text | 0:VOlapScanNode(20) | | TABLE: repro_lambda_access_path.repro_lambda_access_path(repro_lambda_access_path), PREAGGREGATION: ON | | PREDICATES: indexes[apache#2] IS NOT NULL | | partitions=1/1 (repro_lambda_access_path) | | tablets=1/1, tabletList=1787714324292 | | cardinality=1, avgRowSize=0.0, numNodes=1 | | pushAggOp=NONE | | nested columns: | | nested_arrays: | | origin type: array<array<int>> | | all access paths: [nested_arrays.*.*] | | indexes: | | origin type: array<bigint> | | all access paths: [indexes.NULL] | | predicate access paths: [indexes.NULL] | | final projections: array_map((nested,i) -> element_at(nested, i), nested_arrays[#1], indexes[apache#2]) | | final project output tuple id: 1 | | tuple ids: 0 ``` The old visitElementAt handled secondary arguments through a direct generic visitor call:`visit(arguments.get(i), context);` When `i` is an `ArrayItemSlot` leaf, this call does not dispatch to `visitArrayItemSlot`. The collector therefore did not register payload access for the array bound to `i`. #### 3. Comparator-form array_sort did not preserve its returned payload ```sql SELECT id, array_sort( (x, y) -> IF( cardinality(x) < cardinality(y), -1, IF(cardinality(x) = cardinality(y), 0, 1) ), nested_arrays ) AS result FROM repro_lambda_access_path ORDER BY id; +------+--------------------------------------------------------------+ | id | result | +------+--------------------------------------------------------------+ | 1 | [[null, null], [null, null, null], [null, null, null, null]] | | 2 | [[null], [null, null]] | | 3 | [] | | 4 | NULL | +------+--------------------------------------------------------------+ ``` In `EXPLAIN VERBOSE`: ```text | 0:VOlapScanNode(15) | | TABLE: repro_lambda_access_path.repro_lambda_access_path(repro_lambda_access_path), PREAGGREGATION: ON | | partitions=1/1 (repro_lambda_access_path) | | tablets=1/1, tabletList=1787714324292 | | cardinality=4, avgRowSize=1592.5, numNodes=1 | | pushAggOp=NONE | | nested columns: | | nested_arrays: | | origin type: array<array<int>> | | all access paths: [nested_arrays.*.OFFSET] | | final projections: array_sort((x,y) -> if((cardinality(x) < cardinality(x)), -1, if((cardinality(x) = cardinality(x)), 0, 1)), nested_arrays[#1]) | | final project output tuple id: 1 | | tuple ids: 0 ``` For arr `ARRAY<ARRAY<INT>>`, the comparator only needs the inner-array offsets: `arr.*.OFFSET`. However, `array_sort` also returns the original inner arrays after reordering them. The old collector returned immediately after collecting the comparator paths and did not propagate the consumer's result path to `arr`. #### What is changed 1. Resolve Lambda bindings by `ExprId` - Associate each `ArrayItemReference` with its bound array by `ExprId`. - When visiting an `ArrayItemSlot`, search Lambda scopes from the innermost scope to the outermost scope. 2. Dynamically dispatch `element_at` index/key expressions 3. Preserve comparator paths and result paths for `array_sort` #### After fix: ```text Doris> SELECT id, -> array_map( -> (nested, i) -> element_at(nested, i), -> nested_arrays, -> indexes -> ) AS result -> FROM repro_lambda_access_path -> WHERE indexes IS NOT NULL -> ORDER BY id; +------+-----------+ | id | result | +------+-----------+ | 1 | [3, 4, 2] | | 2 | [9, 7] | | 3 | [] | +------+-----------+ Doris> SELECT id, -> array_sort( -> (x, y) -> -> IF( -> cardinality(x) < cardinality(y), -> -1, -> IF(cardinality(x) = cardinality(y), 0, 1) -> ), -> nested_arrays -> ) AS result -> FROM repro_lambda_access_path -> ORDER BY id; +------+-----------------------------------+ | id | result | +------+-----------------------------------+ | 1 | [[1, 2], [2, 3, 1], [4, 2, 1, 4]] | | 2 | [[9], [8, 7]] | | 3 | [] | | 4 | NULL | +------+-----------------------------------+ ```
linrrzqqq
pushed a commit
that referenced
this pull request
Sep 1, 2026
Problem Summary: Streaming `ROWS` window aggregates retain state across
frame evaluations. The eviction path previously considered only whether
buffered blocks had been emitted, so it could erase either the outgoing
row needed by a bounded sliding frame or the next unread row needed by
an `UNBOUNDED PRECEDING ... N PRECEDING` frame. After rebasing, negative
partition and outgoing positions could allow a nullable aggregate to
access its null map out of bounds. Evicting either kind of required row
could also produce incorrect aggregate results.
Root cause: `_remove_unused_rows()` did not account for the earliest row
required by the next ROWS frame evaluation, and
`BoundaryPose::remove_unused_rows()` allowed retained-column coordinates
to become negative.
This change defers block-aligned eviction when the candidate prefix
contains either `frame_start - 1`, the outgoing row required by a
bounded sliding update, or the next unread row required by an `UNBOUNDED
PRECEDING ... N PRECEDING` frame. It also rebases partition and order
boundaries to nonnegative physical-column coordinates. The BE unit
coverage exercises both ROWS executors across eviction boundaries and
verifies boundary rebasing.
Observed ASAN failure before this change (`output/be/log/be.out`):
```text
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 1
#0 doris::AggregateFunctionNullUnaryInlineV2<...>::execute_function_with_incremental(...)
be/src/exprs/aggregate/aggregate_function_null_v2.h:595
#1 doris::AggFnEvaluator::execute_function_with_incremental(...)
be/src/exprs/vectorized_agg_fn.cpp:334
apache#2 doris::AnalyticSinkLocalState::_execute_for_function<true>(...)
be/src/exec/operator/analytic_sink_operator.cpp:385
apache#3 doris::AnalyticSinkLocalState::_get_next_for_sliding_rows(...)
be/src/exec/operator/analytic_sink_operator.cpp:203
apache#4 doris::AnalyticSinkLocalState::_execute_impl(...)
be/src/exec/operator/analytic_sink_operator.cpp:358
apache#5 doris::AnalyticSinkOperatorX::sink_impl(...)
be/src/exec/operator/analytic_sink_operator.cpp:757
SUMMARY: AddressSanitizer: heap-buffer-overflow in
doris::AggregateFunctionNullUnaryInlineV2<...>::execute_function_with_incremental(...)
```
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
linrrzqqq
pushed a commit
that referenced
this pull request
Sep 10, 2026
…angled in the apache#64093 backport (apache#67564) ### What problem does this PR solve? Problem Summary: Every branch-4.1 PR that touches `thirdparty/` fails `Build Third Party Libraries (macOS-arm64)` after two to three minutes, inside `download-thirdparty.sh`, long before anything is compiled: ``` patching file 'src/dwarf/Gfind_proc_info-lsb.c' 2 out of 2 hunks failed--saving rejects to 'src/dwarf/Gfind_proc_info-lsb.c.rej' ##[error]Process completed with exit code 1. ``` | PR | base | `Build Third Party Libraries (macOS-arm64)` | |---|---|---| | apache#67378 | branch-4.1 | fail, 5m18s — these hunks | | apache#67356 | branch-4.1 | fail, 3m11s — these hunks | | apache#67523 | branch-4.1 | fail, 2m43s — these hunks | | apache#67554 | branch-4.1 | fail, 2m43s — these hunks | | **apache#67535** — same change as apache#67523 | **master** | **pass, 2h26m full build** | The last row is the point: the same content passes on master and dies in the patch step on branch-4.1. **Root cause.** `thirdparty/patches/libunwind-1.6.2-doris-phdr-cache.patch` is corrupt on this branch. It arrived that way in `4439e6f6b46` (the backport of apache#64093) and does not match the file master carries (`dc70115a432`) — even though both branches pin the same libunwind 1.6.2 with the same md5 and run byte-identical patching code in `download-thirdparty.sh`. In master's copy the blank lines around the change are **context** lines, each a line holding a single space. In this branch's copy they are **deletion** lines (`-` with nothing after it), with the hunk headers renumbered to match that reading (`@@ -46,3 +46,8 @@` instead of `@@ -47,6 +47,14 @@`, `@@ -805,5 +810,6 @@` instead of `@@ -804,7 +812,10 @@`) and the trailing context dropped. That is what a trailing-whitespace strip followed by a hand repair leaves behind. The patch now asks `patch(1)` to delete blank lines that the real source does not have at those offsets. **Why only macOS.** GNU patch on the Linux runner recovers from it — `Hunk #1 succeeded at 12 with fuzz 1` — and the Linux job passes, which is why this went unnoticed for a week. The macOS runner's `patch` rejects both hunks and `download-thirdparty.sh` exits 1. **Fix.** Take master's copy verbatim. One file, nothing else. ### Release note None ### Check List (For Author) - Test - [ ] Regression test - [ ] Unit Test - [x] Manual test (add detailed scripts or steps below) Verified against the pinned tarball (`libunwind-1.6.2.tar.gz`, md5 `f625b6a98ac1976116c71708a73dc44a`) with the same patch implementation the runner uses (`patch 2.0-12u11-Apple`), on two pristine copies of the extracted source: | patch file | exit | result | |---|---|---| | this branch's current file | **1** | `2 out of 2 hunks failed` — byte-identical to the CI message | | this PR's file | **0** | applies cleanly, no `.rej` | After applying, the weak declaration and the fork land where `be/src/common/phdr_cache.cpp` expects them: ``` 54:extern int doris_unwind_iterate_phdr (int (*callback) (struct dl_phdr_info *, size_t, void *), 55: void *data, unw_word_t ip) __attribute__ ((weak)); ... 815: if (doris_unwind_iterate_phdr) 816: ret = doris_unwind_iterate_phdr (dwarf_callback, &cb_data, ip); 817: else 818: ret = dl_iterate_phdr (dwarf_callback, &cb_data); ``` This branch does define that symbol (`be/src/common/phdr_cache.cpp:249`), so the fix restores the intended behaviour rather than being a no-op. Linux builds were already getting it via fuzz, so no behaviour changes there. - Behavior changed: - [x] No. Linux already applied this patch (with fuzz); this only makes the macOS thirdparty build stop failing. - Does this need documentation? - [x] No. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DzUYFcGHQH3bnLGCjpncVj Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
linrrzqqq
pushed a commit
that referenced
this pull request
Oct 12, 2026
…pache#68880) Issue Number: N/A Related PR: N/A Problem Summary: EXCEPT DISTINCT can introduce a streaming distinct aggregation whose grouping expressions repeat an input column, for example (s, d, d). When preaggregation switches to pass-through, both output construction and column reuse call assert_mutable() even though they only transfer column references. Retaining output references can make a repeated key shared, so a subsequent assertion fails with COW::assert_mutable: use_count() > 1: ``` W 2026-10-09 06:31:59,596 13803 exception.cpp:34] meet exception, error code: 6, message: COW::assert_mutable: use_count() > 1 0# doris::Exception::Exception(int, std::basic_string_view<char, std::char_traits<char> > const&) at /usr/local/ldb-toolchain-v0.26/bin/../lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/bits/basic_string.h:239 1# doris::DistinctStreamingAggLocalState::_distinct_pre_agg_with_serialized_key(doris::Block*, doris::Block*) at ../src/core/cow.h:325 2# doris::DistinctStreamingAggOperatorX::push(doris::RuntimeState*, doris::Block*, bool) const at ../src/common/status.h:519 3# doris::StatefulOperatorX<doris::DistinctStreamingAggLocalState>::get_block_impl(doris::RuntimeState*, doris::Block*, bool*) at ../src/common/status.h:519 4# doris::OperatorXBase::get_block(doris::RuntimeState*, doris::Block*, bool*) at ../src/common/status.h:519 5# doris::OperatorXBase::get_block_after_projects(doris::RuntimeState*, doris::Block*, bool*) at ./be/build_RELEASE/../src/exec/operator/operator.cpp:0 6# doris::PipelineTask::execute(bool*) at ./be/src/exec/pipeline/pipeline_task.cpp:0 7# doris::TaskScheduler::_do_work(int) at ./be/src/exec/pipeline/task_scheduler.cpp:155 ``` Use immutable get_ptr() references in both pass-through paths. The evaluated key pointers preserve the original column when a repeated input position has already been replaced during column reuse, and downstream mutations continue to use copy-on-write. No column data is copied during pass-through. Add unit coverage with three references to one input slot for fresh and reused output blocks, transition from deduplication, retained output ownership, and nullable output. Add regression coverage for repeated keys in EXCEPT DISTINCT, nullable EXCEPT output, INTERSECT DISTINCT, and UNION DISTINCT. Fix queries with repeated projected columns failing with a COW assertion during distinct streaming aggregation. 根因是:**Distinct Streaming Aggregation 在透传列时调用了要求“列对象独占”的 `assert_mutable()`,而重复分组键会让多个输出位置共享同一个列对象,导致后续调用违反这个要求。** 下面结合修复前的代码,分别说明两条透传路径。 **1\. `assert_mutable()` 检查的是列对象的引用计数** COW::assert\_mutable() (be/src/core/cow.h:323) 的实现是: ``` MutablePtr assert_mutable() const { if (this->use_count() > 1) { throw Exception(ErrorCode::INTERNAL_ERROR, "COW::assert_mutable: use_count() > 1"); } return const_cast<COW*>(this)->get_ptr(); } ``` 它要求调用时 `use_count() <= 1`,直接返回可变指针,**不会自动复制共享的列对象**。 如果确实需要修改可能共享的列,应使用 IColumn::mutate() (be/src/core/column/column.h:618):它通过 `shallow_mutate()` 在引用计数大于 1 时复制列,使修改发生在独立对象上。 这里的引用计数统计的是持有列对象的智能指针数量。例如: ``` in_block.column[1] ─┐ ├── 同一个日期列对象 C output.column[2] ───┘ C.use_count() = 2 ``` **2\. 重复投影如何变成重复输入列引用** 例如: ``` SELECT s, d, d FROM t1 EXCEPT DISTINCT SELECT s, d, d FROM t2; ``` 当优化器为集合操作的输入推导局部去重时,InferSetOperatorDistinct (fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/InferSetOperatorDistinct.java:60) 使用子节点输出构造分组键: ``` new LogicalAggregate<>( ImmutableList.copyOf(child.getOutput()), true, child) ``` 因此,生成的局部聚合可能保留重复槽位,例如: ``` group by: s[#1], d[apache#2], d[apache#2] ``` BE 在准备分组列 (be/src/exec/operator/distinct\_streaming\_aggregation\_operator.cpp:177)时,记录表达式执行结果: ``` RETURN_IF_ERROR(_probe_expr_ctxs[i]->execute(in_block, &result_column_id)); // 准备列…… key_columns[i] = in_block->get_by_position(result_column_id).column.get(); result_idxs[i] = result_column_id; ``` 假设输入 Block 只有 `s`、`d` 两列,最终得到: ``` in_block: [Cs, Cd] result_idxs: [ 0, 1, 1] key_columns: [Cs*, Cd*, Cd*] ``` 两个日期键指向同一个列对象。`key_columns` 保存的是裸指针,本身不增加引用计数。 当预聚合因内存限制或去重收益不足设置 `_stop_emplace_flag = true` 后,算子开始透传输入,进入下面的问题路径。 **3\. 首次构造输出:两个重复引用即可报错** 修复前,不复用输出列的透传代码是: ``` for (int i = 0; i < key_size; ++i) { if (_stop_emplace_flag) { columns_with_schema.emplace_back( key_columns[i]->assert_mutable(), /* type, name */); } } ``` 对应当前代码的输出构造位置 (be/src/exec/operator/distinct\_streaming\_aggregation\_operator.cpp:267)。 以 `[s, d, d]` 为例,假设日期列 `Cd` 开始只有输入 Block 持有: | 时刻 | 持有 `Cd` 的位置 | 稳定引用计数 | |---|---|---:| | 处理第一个 `d` 前 | 输入列 | 1 | | 第一个 `d` 加入输出后 | 输入列、第一个日期输出列 | 2 | | 处理第二个 `d` 时 | 再次调用 `Cd->assert_mutable()` | 2,抛异常 | 第一个 `d` 输出时增加了一个列引用,输入引用仍然存在。第二个 `d` 再要求独占,就会失败。 后面的 `in_block->clear()` 原本可以释放输入引用,但异常已经在循环内发生,执行不到那里。 **4\. 复用输出 Block:三个重复引用能展示同样的问题** 修复前,复用输出列的透传位置 (be/src/exec/operator/distinct\_streaming\_aggregation\_operator.cpp:235) 是: ``` for (int i = 0; i < key_size; ++i) { auto output_column = out_block->get_by_position(i).column; out_block->replace_by_position(i, key_columns[i]->assert_mutable()); in_block->replace_by_position(result_idxs[i], output_column); } ``` 这个分支将旧输出列放回输入 Block,释放输入对原数据列的引用。 以 `[s, d, d, d]` 为例: ``` result_idxs = [0, 1, 1, 1] ``` 假设输出列最初为空,且没有其他所有者: | 时刻 | 原日期列 `Cd` 的所有者 | 稳定引用计数 | |---|---|---:| | 第一个 `d` 处理结束 | 第一个日期输出列;输入位置已换成旧输出空列 | 1 | | 第二个 `d` 处理结束 | 第一个、第二个日期输出列 | 2 | | 第三个 `d` 开始处理 | 调用 `Cd->assert_mutable()` | 2,抛异常 | 第二次替换的输入位置已经是空列,无法再释放原日期列的引用。于是两个输出列共同持有 `Cd`,第三次独占断言失败。 因此,**复用分支中只有两个相同槽位不一定立即报错**;已有单测使用 `[0, 1, 0, 0]`,让同一输入槽位出现三次,覆盖这一过程。 **5\. 为什么改为 `get_ptr()` 能解决** 两条透传路径现在都使用: ``` key_columns[i]->get_ptr() ``` `key_columns` 的类型是 `std::vector<const IColumn*>`,因此这里调用的是 const 版本的 `get_ptr()` (be/src/core/cow.h:304),返回不可变列指针。 透传只需要让输出引用已有列数据,允许多个输出位置共享该列。后续需要修改数据时,仍通过 `IColumn::mutate()` 执行 COW。 复用分支继续从预先记录的 `key_columns` 取指针也很关键:第一次替换后,`in_block[result_idxs[i]]` 已经变成旧输出列,而 `key_columns[i]` 仍指向原数据列,并由前面的输出位置保持其生命周期。 关于 GROUP BY,直接的 `GROUP BY s, d, d` 会在 NormalizeAggregate (fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeAggregate.java:167) 中去重。静态分析发现,`GROUP BY + LIMIT` 下推到 `UNION ALL` 分支时,槽位映射可能重新引入重复键;已在回归用例 (regression-test/suites/query\_p0/set\_operations/distinct\_streaming\_repeated\_keys.groovy:101)中补充直接分组对照和该候选场景,并检查计划中同一槽位是否出现三次。提交为 `c8c1f67d3e9`,按你的要求未执行新增用例,因此尚未实测确认该 GROUP BY 场景。 - Test: Unit Test / Regression test / Manual test - ASAN build with ./build.sh --be -j 12 - 11 BE unit tests covering distinct streaming aggregation - Four regression queries; results also match non-streaming execution - Original reproducing SQL on the supplied table data - clang-format 16, format check, build hygiene, and clang-tidy on changed lines - Behavior changed: Yes; repeated grouping keys pass through without an incorrect ownership assertion - Does this need documentation: No ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
linrrzqqq
pushed a commit
that referenced
this pull request
Oct 21, 2026
…fresh exprId (apache#69111) ### What problem does this PR solve? Problem Summary: When ConstantPropagation replaces an aggregate output by a constant, it materializes the constant in a project above the aggregate, and the project item reused the exprId of the replaced output: LogicalProject ( projects=[..., 1 AS `id`#1, ...] ) +-- LogicalAggregate ( groupByExpr=[id#1], outputExpr=[id#1] ) So one exprId had two definitions, the aggregate output slot and the constant, whose nullable and data type may differ. Rules which compare exprIds only then take them as the same column. For RQG case 11000258898 EliminateUnnecessaryProject removed such a project after the aggregate had been rewritten into `Limit 1`, the plan above fell back to the nullable output slot of the set operation, and the last AdjustNullable pass reported AdjustNullable convert slot 2004-04-28#55 from not-nullable to nullable Fix: Give the constant a fresh exprId only when it really shadows a column, i.e. when the exprId is still produced by the plan below; otherwise keep it: * the constant gets a fresh exprId, and the references above are refreshed with ExprIdRewriter, when the aggregate still outputs the replaced column (its grouping key is kept because all grouping keys are constant) or when a project item re-defines a column which its child still outputs. That is the shadow the old code created, e.g. the RQG case 11000258898 of DORIS-28416. * the constant keeps the exprId when the aggregate removed the constant grouping key from its outputs: the project item is then the only definition of the column, nothing below references the exprId, and consumers which rely on the identity of the column, e.g. the materialized view matching, keep working. The plan of such a query is byte identical to the plan before this change, including the exprIds (verified with the query of `agg_optimize_when_uniform/mv2_0`, for the query side and for the MV side which is generated with constant propagation disabled). The sink also has to follow the fresh exprIds, otherwise it would reference a slot which its child does not output any more; only exprIds are remapped there, the name and the data type declared by the sink are kept (that is why the rule does not replace the slots of the sink by literals). A CTE is rewritten in separate subtree contexts (producer body and consumers) and the consumers, which map their slots to the producer outputs by exprId, are rewritten before the producer. The renamed exprIds are published per CTE (`StatementContext#producerExprIdRemap`), and `RewriteCteChildren` refreshes both maps of every consumer with the last exprId of the chain (`ExprIdRewriter#lastExprId`). Reproduction: ConstantPropagationFreshExprIdTest runs this rule alone, so the project above the aggregate is still visible in the plan, and asserts that it does not reuse the exprId of the aggregate output and that the plan above references the fresh exprIds. ConstantPropagationTest used to pin the reused exprIds in its constant project expectations; it now asserts fresh exprIds. Both fail before this change. ### Release note None ### Check List (For Author) - Test: Unit Test - ConstantPropagationFreshExprIdTest (new): fails before with "the constant project reuses the exprId of the aggregate output". - ConstantPropagationTest: expectations updated, fails before with "the constant project item must not reuse the exprId of the replaced output slot". - 31 related FE unit test classes (constant propagation, constant cte, set operation, column pruning, fd, uniform, sql plan suite, runtime filter, ...) rerun: 385 tests, 0 failures. - Behavior changed: Yes. A constant materialized above an aggregate has its own exprId and the plan above references it, so one exprId no longer describes two different columns. - Does this need documentation: No
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
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: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)