airborne12 opened a new pull request, #66262:
URL: https://github.com/apache/doris/pull/66262
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
`runtime/exec_env.h` included
`storage/index/inverted/inverted_index_writer.h`, which nothing in that header
actually needed: the two types it names, `InvertedIndexSearcherCache` and
`InvertedIndexQueryCache`, are already forward declared a few lines below. The
include was dead, but not free — `exec_env.h` is reached by most of the
backend, so it put the whole inverted index writer stack (and through it
CLucene) in front of ~1750 translation units. Editing any index or index-writer
header therefore rebuilt essentially all of BE.
Measured on this tree from the reverse include graph, before → after:
| header | TUs rebuilt before | after |
| --- | ---: | ---: |
| `storage/index/inverted/inverted_index_writer.h` | 1752 | **142** |
| `storage/index/index_file_writer.h` | 1754 | **254** |
Deleting the include alone does not compile, which is why it had survived.
The `#ifdef BE_TEST` accessor `set_tmp_file_dir` was defined inline in the
header:
```cpp
void set_tmp_file_dir(std::unique_ptr<segment_v2::TmpFileDirs>
tmp_file_dirs) {
this->_tmp_file_dirs = std::move(tmp_file_dirs);
}
```
Assigning the `unique_ptr` destroys the old pointee, so `TmpFileDirs` had to
be **complete** in every translation unit that includes `exec_env.h` — and that
completeness was being supplied, by accident, through the dead include.
Removing the include without this fix breaks every translation unit that
instantiates the setter (29 of them on the tree where this was first tried),
all with the same `invalid application of 'sizeof' to an incomplete type
'doris::segment_v2::TmpFileDirs'`.
Worth noting: the inline body only exists under `BE_TEST`, so production
builds were always clean and only the unit-test build exposed the requirement.
That is part of why the dead include went unnoticed.
The fix keeps the header free of the dependency — the accessor is declared
in the header and defined in `exec_env.cpp`, which includes
`storage/index/index_writer.h` for the complete type. The member
`std::unique_ptr<TmpFileDirs>` needs nothing further, because `~ExecEnv()` is
already out of line.
**Guard against regression.** Explaining the rule does not hold the line;
the include compiled fine for as long as it sat there. So
`build-support/check-header-deps.py` turns it into a build error. Rules are
declarative — per hub header, a subtree that must not be reachable through any
chain of includes, plus explicit exceptions and the reason for each:
```python
RULES = [(
"runtime/exec_env.h",
"storage/index/",
{"storage/index/inverted/inverted_index_stats.h"}, # leaf POD struct
via storage/olap_common.h
"ExecEnv only names index types as pointers and already forward-declares
them; ...",
)]
```
On a violation it prints the offending include chain and the fix, so the
next person can see how they got there:
```
error: runtime/exec_env.h must not reach storage/index/*
chain: runtime/exec_env.h
-> storage/index/inverted/inverted_index_writer.h
-> storage/index/index_file_writer.h
fix: forward-declare the type in the header and include the real header
in the
.cpp, or route it through a *_fwd.h
```
`--report` ranks headers by how many translation units they can force a
rebuild of, for finding the next one of these. Exceptions are listed explicitly
rather than inferred, so widening the barrier stays a deliberate, reviewable
act.
### Release note
None
### Check List (For Author)
- Test: Manual test
- Built the BE unit-test target with this change applied and confirmed
the translation units that previously failed on the incomplete `TmpFileDirs`
(among them `ann_index_result_cache.cpp`, `faiss_ann_index.cpp`,
`column_with_type_and_name.cpp`) all compile. That build ran on a tree 55
commits behind this base; upstream has not touched `exec_env.h` or
`exec_env.cpp` in between, so CI on this PR is the check for the current base.
- Verified `build-support/check-header-deps.py` passes on this tree and
exits 1 when the removed include is put back.
- No functional test is added: this change removes a dependency without
altering behavior, and the layering check is itself the regression guard.
- Behavior changed: No
- Does this need documentation: No
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_015y8zuYVey5DybRbUvwDnof
--
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]
