[opt](build) Take olap_common.h off the dependency base and slim the PCH - #66826
Open
morningman wants to merge 4 commits into
Open
[opt](build) Take olap_common.h off the dependency base and slim the PCH#66826morningman wants to merge 4 commits into
morningman wants to merge 4 commits into
Conversation
… olap_common.h Three foundational entities lived in storage/olap_common.h, which drags gen_cpp/Types_types.h, io_common, uid_util (boost/uuid) and friends into every closure that needs any of them: - int128_t/uint128_t -> core/extended_types.h. core/types.h reaches olap_common through binary_cast.hpp -> packed_int128.h purely for these two aliases, which put the storage layer underneath every TU in the codebase; util/coding.h had the same dependency for uint128_t. - RowsetId -> storage/rowset_id.h (new), a dependency-free header (cstdint/functional/iosfwd/string) safe for core/column/column.h. Method bodies, the hex helpers dependency and MAX_ROWSET_ID/LOW_56_BITS move to storage/rowset_id.cpp; std::hash<RowsetId>::operator() is defined out of line so the header does not need hash_util.hpp. - enum FieldType (+ the three field_is_*_type helpers) -> storage/field_type.h (new), so data-type headers can name storage cell types without the rest of olap_common.h. olap_common.h includes the three light headers, so its own users are unaffected. Includer fixes: packed_int128.h / util/coding.h / data_type_number_serde.h now use extended_types.h; column.h / column_nullable.h use rowset_id.h; data_type_decimal.h uses field_type.h; data_type_ipv4.h and ipv4/ipv6 serde headers dropped a dead olap_common include; decimal12.h carries its own power table instead of pulling storage/utils.h. The closure-sweep natural-closure gate (300 first-party TUs, PCH stripped) surfaced every TU that only compiled through the now-cut transitive paths; fixed at the owning header: metadata_adder.h (rowset_fwd), runtime_profile.h (cast_set), column_reader_cache.h (io_common + olap_common), condition_cache.h (rowset_id), python_client.h (olap_define for DISALLOW_COPY_AND_ASSIGN), format_v2/file_reader.h and tracing_file_reader.h (io_common), four data-type headers (field_type). function_encryption.cpp: the const-args dispatch is now `if constexpr` so the arg_num==4 instantiation no longer indexes col_const[4]/argument_columns[4] out of bounds (-Warray-bounds fired once the include graph changed). Verification: closure-sweep 300/300 clean vs p3-pre baseline; compile_bench full rebuild + doris_be link green at -j6; BE UT Debug build green, 31 tests / 6 suites passed (RowsetId behavior: IdManagerTest, IdFileMapTest, StorageResourceTest, TestRowIdConversion, TxnManagerTest; registration contract: BinaryArithmeticRegistrationTest). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND7L1ZVTJf91TBpLwYSqct
exprs/function/function.h is included by every scalar-function TU, and three of its includes pulled the storage/index domain (zone-map -> thrift + S3 SDK; inverted iterator -> reader -> tablet_schema + CLucene; inverted parser) into all of them. All heavy types appear only as pointers/references in the interface, so forward declarations suffice: DictionaryEvalContext/BloomFilterEvalContext are declared with their real doris::expr_zonemap:: home plus the same doris-level aliases expr_zonemap_filter.h defines; segment_v2::IndexIterator and InvertedIndexResultBitmap as classes. zonemap_filter_result.h (456 preprocessed lines) stays: ZoneMapFilterResult is returned by value. multiply.cpp natural closure: 432,112 -> 308,140 lines (-28.7%) together with the previous commit; the three edges are redundant with each other, so cutting them individually moves almost nothing -- they only pay off as a set. doris_be also shrinks 328KB from fewer transitively visible inline weak symbols in function TUs. Dependents that really use the cut headers now include them directly (closure-sweep verified, 300/300 clean): functions_comparison.h and function_ip.h materialize InvertedIndexParam (inverted_index_iterator.h); is_null.h/is_not_null.h call IndexIterator methods (index_iterator.h); cast_to_string.h/cast_to_timestamptz.h dereference RuntimeState (runtime/runtime_state.h). index_file_reader.h wraps its CLucene includes in the codebase's established -Wconversion suppression: whether CLucene's first expansion lands inside someone's suppressed region depends on include order, and this TU became the first expansion point once the function.h edge disappeared. Verification: closure-sweep 300/300 clean vs p3-pre; compile_bench rebuild (200 targets) + doris_be link green at -j6; BE UT green -- 44 tests passed (ExprZonemapFilterTest, SegmentIteratorExprZonemapTest, SegmentIteratorApplyIndexExprTest, BinaryArithmeticRegistrationTest). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND7L1ZVTJf91TBpLwYSqct
…ype base Four small usages were each dragging a heavy header into the closure of nearly every TU: - core/data_type/primitive_type.h included util/json/path_in_data.h (-> gen_cpp/segment_v2.pb.h, ~11.6k preprocessed lines) for one `using VariantMap = std::map<PathInData, FieldWithDataType>` alias. core/field.h used to carry the same alias and include until apache#65561 moved both into core/value/variant/variant_field.h, which field.h now includes -- the same edge, one hop longer. A forward declaration is enough for the alias in both headers. VariantField's std::unique_ptr<VariantMap> member needs every special member out-of-line for that; the only inline one, the class-body defaulted default constructor, would instantiate the VariantMap destructor through the _legacy deleter, so it moves to the .cpp (declared noexcept, defaulted there). TUs that instantiate the map include the real header. - common/logging.h included util/uid_util.h (-> Types_types.h + boost/uuid -> boost/tti -> boost/function_types) so that TaggableLogger::tag could name TUniqueId/PUniqueId in an `if constexpr` and call print_id. std::is_same_v works on incomplete types, so declaring the two classes plus the two print_id overloads suffices; callers that actually log an id already have the definitions. - util/pretty_printer.h included boost/algorithm/string.hpp (~60k lines through boost/function) for one boost::algorithm::join and two boost::enable_if_c. Replaced by direct streaming (which also drops the intermediate vector<string> materialization) and std::enable_if_t. runtime_profile.h includes pretty_printer.h, so every TU with a profile was paying for it. Dependents that really use the cut headers now include them directly (closure-sweep verified, 300/300 clean): segment_iterator.h, field.cpp, variant_field.cpp, variant_field_test.cpp and column_variant_v2_test.cpp (path_in_data.h), vmatch_predicate.cpp and schema_scan_operator.cpp (boost/algorithm/string.hpp for split/iequals). ColumnPredicateInfo::debug_string also used boost::join and now streams its std::set directly. multiply.cpp natural closure with the whole Phase 3 series so far: 432,112 -> 242,764 lines (-43.8%); aws, CLucene and segment_v2.pb.h are gone from it entirely. Verification: closure-sweep 300/300 clean vs p3-pre; compile_bench rebuild (318 targets, PCH-wide) + doris_be link green at -j6; BE UT build green, 43 tests passed across the affected suites plus 462 in the variant/profile sweep. The two SegmentFlusherFormatTest "...KeepTheirSegmentBytes" cases fail both with and without this change (verified by rebuilding the UT binary at the parent commit), so they are pre-existing and unrelated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND7L1ZVTJf91TBpLwYSqct
pch.h is a dependency of every first-party object, so each doris header it pulls in becomes a tripwire: touching that header invalidates the PCH and rebuilds the whole BE, however few TUs actually name a symbol from it. storage/olap_common.h dragged in 22 more doris headers this way (io_common, uid_util, hash_util, time, olap_define, rowset_fwd, inverted_index_stats, ...). With the preceding commits every TU compiles against its natural include closure (closure-sweep --no-pch is clean for all 300 first-party TUs), so the entry can go. Third-party headers stay: absorbing those is what a PCH is for. One TU grew a bare dependency upstream since that sweep: data_type_array_serde.cpp now names FieldType (apache#66413 series) and only compiled through the PCH's olap_common.h; it gets the direct storage/field_type.h include. Effect on the incremental rebuild radius (ninja -t deps, first-party objects): storage/olap_common.h 318 (all, via PCH) -> 182 real dependents util/uid_util.h 318 (all, via PCH) -> 196 real dependents and the world-rebuild tripwire set shrinks from 31 doris headers to 9 (config/status/check/compiler_util/expected/version_internal/stack_util and pch.h itself). multiply.cpp preprocessed size, PCH-inclusive (the size the front end actually sees in a real build): 806k -> 717,285 lines. Verification: compile_bench full rebuild (318 targets) + doris_be link green at -j6. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ND7L1ZVTJf91TBpLwYSqct
morningman
requested review from
Gabriel39,
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
August 17, 2026 04:00
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
morningman
added a commit
that referenced
this pull request
Aug 17, 2026
…count probe (FE) (#66831) ### What Two independent breakages that each make **current master fail to build** — one in BE configure, one in FE compile. They share a shape: a pair of PRs that never conflict textually, merge cleanly, and only break once combined, so each PR's own pipeline was green. | | Breakage | Colliding PRs | |---|---|---| | BE | `cmake` configure aborts | #66052 moved a file, #66789 made a dangling unity-skip entry fail loud | | FE | `fe-connector-iceberg` does not compile | #66778 deleted `getCountFromSnapshot()`, #66413 added a caller for it | CI merges each PR into the latest master before building, so **every PR pipeline that picks up current master is red** on one or both. --- ## 1. BE — stale `STORAGE_UNITY_SKIP` entry for a moved file Remove the stale `STORAGE_UNITY_SKIP` entry (and its comment block) for `compaction/collection_statistics.cpp`, which no longer exists. ### Why — master configure is currently broken #66052 moved `storage/compaction/collection_statistics.{cpp,h}` to `storage/index/inverted/similarity/` (rewritten), but left behind the unity-skip entry that #66789 had added for the old path. The fail-loud validation introduced by #66789 turns a dangling skip entry into a configure-time error — which is exactly what it is designed to catch (a skip list rotting after a file move), so BE configure on current master fails immediately: ``` CMake Error at CMakeLists.txt:1002 (message): unity skip entry does not exist (renamed or moved?): .../be/src/storage/compaction/collection_statistics.cpp ``` #66826, #66824, #66819, #66820 were the first hits — same error on multiple independent agents. ### Why deletion (not a path update) is correct The old entry existed because the old `collection_statistics_test` `#include`d the `.cpp` into a second TU (unity batching would then produce a duplicate definition at link time). The rewritten file at the new location is not `#include`d by any test (`grep -rn 'collection_statistics.cpp' be/test/` is empty on master), so the new path needs no skip entry. ### Verification - Full BE build from a clean tree at master + this change (clang20 / macOS arm64, unity=ON, PCH=ON): configure passes the skip-list validation and the build compiles. (The same tree without this change fails configure with the error above.) - Timeline note: #66052's last green CI round presumably predates #66789's validation landing (2026-08-16), which is how the dangling entry slipped through. --- ## 2. FE — the metadata-only COUNT(\*) probe calls a deleted method #66778 replaced the snapshot-summary COUNT(\*) pushdown with a manifest-derived count and deleted `getCountFromSnapshot()`. `canServeMetadataOnlyCount()`, added by #66413, still calls it, so FE compilation fails: ``` [ERROR] .../connector/iceberg/IcebergScanPlanProvider.java:[505,16] cannot find symbol [ERROR] symbol: method getCountFromSnapshot(org.apache.iceberg.TableScan,org.apache.doris.connector.spi.ConnectorSession) [ERROR] location: class org.apache.doris.connector.iceberg.IcebergScanPlanProvider ``` ### Why re-express the probe instead of restoring the method Bringing `getCountFromSnapshot()` back would reintroduce precisely what #66778 removed: a query result derived from optional, writer-provided snapshot summary fields. The probe is rebuilt in #66778's own terms instead. It reuses that PR's delete gate and additionally requires the data manifests to carry aggregate row counters, so the answer is proved from the manifest list alone — `O(manifests)`, no data-file enumeration, which is what a probe running before planning can afford. Manifest lists that omit those aggregates now answer `false`, where count planning still serves them through its bounded per-file fallback. A capability probe should under-promise rather than over-promise; the planner itself is untouched. ### Verification - `mvn package` over the full FE reactor (74 modules): all green. This also confirms no second semantic break is hiding behind the first — CI's maven stops at `fe-connector-iceberg` and never reaches the rest. - `IcebergScanPlanProviderTest`: 153 tests, 0 failures, including a new case pinning that the probe follows the same delete gate as count planning. - FE checkstyle: 0 violations. ### Release note None (both fixes only restore a buildable master; no user-visible behavior change). ### Check List (For Author) - Test - [x] Regression test — not applicable: neither change alters query behavior - [x] Unit test: `IcebergScanPlanProviderTest` (153 tests, 0 failures) - [x] Manual test: full BE configure/build and full FE reactor build - Behavior changed: No - Does this need documentation: No --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17367 ms |
Contributor
TPC-DS: Total hot run time: 83753 ms |
Contributor
ClickBench: Total hot run time: 14.77 s |
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
2 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
Take
storage/olap_common.hoff the every-TU dependency base: move its three universally-consumed pieces (int128_t/uint128_ttypedefs,RowsetId,FieldType) into small dedicated headers, cut four side-door include edges under the column/type base, and dropstorage/olap_common.hfrompch.h.The main win is incremental rebuild radius, not cold-build wall clock. Cold build is measured neutral (673.3s vs 674.1s A/B on the PCH removal); what changes is how many TUs recompile when a storage-domain header is touched, and how many preprocessed lines every function/expr TU pays.
Why / mechanism
core/types.h -> binary_cast.hpp -> packed_int128.h -> olap_common.h: the deepest base header of the tree carried the whole storage domain because of twoint128typedefs. They move tocore/extended_types.h;RowsetIdmoves to a newstorage/rowset_id.h/.cpp(method bodies out-of-line),FieldTypeto a newstorage/field_type.h.olap_common.hre-exports all three, so the 260 direct users see zero API change.exprs/function/function.hincluded three storage-domain headers (zonemap condition, inverted index iterator, function parser). The three edges are redundantly meshed: cutting any single one is worth almost nothing (-406 / -15,455 / 0 preprocessed lines), cutting the group is -122,885 lines from every function TU.core/data_type/primitive_type.h(and formerlycore/field.h, see drift note) includedutil/json/path_in_data.h->gen_cpp/segment_v2.pb.h(~11.6k lines) for oneusing VariantMap = std::map<PathInData, FieldWithDataType>alias. A forward declaration suffices.common/logging.hincludedutil/uid_util.h(->Types_types.h+boost/uuid->boost/tti) soTaggableLogger::tagcould nameTUniqueId/PUniqueIdin anif constexpr.std::is_same_vworks on incomplete types.util/pretty_printer.hincludedboost/algorithm/string.hpp(~60k lines) for oneboost::algorithm::joinand twoboost::enable_if_c;runtime_profile.hincludespretty_printer.h, so every TU with a profile paid for it. Replaced by direct streaming andstd::enable_if_t.pch.hdropsstorage/olap_common.h(it alone pulled 22 doris headers into the PCH blast line).Measured effect (mother-branch pairing, clang20 / macOS arm64,
ninja -t deps-based radius)storage/olap_common.hutil/uid_util.hutil/json/path_in_data.hmultiply.cppnatural closure across the whole series: 432,112 -> 242,764 preprocessed lines (-43.8%); aws/S3 SDK, CLucene andsegment_v2.pb.hare gone from function-module closures entirely.Pre-existing defects fixed on the way
exprs/function/function_encryption.cpp: statically out-of-bounds index intobool[4]in thearg_num==4instantiation (indexed[4]); rewritten asif constexprdispatch.storage/index/index_file_reader.h: its CLucene warning suppression only worked by include-order luck.storage/segment/condition_cache.h: usesRowsetIdbut never included a header providing it (leaned on a neighbor's transitive include).Upstream drift absorbed during rebase
VariantMapalias plus itsutil/json/path_in_data.hinclude fromcore/field.hinto the newcore/value/variant/variant_field.h, whichfield.hnow includes — same heavy edge, one hop longer. The cut is applied at the new location:variant_field.hforward-declaresPathInData, and the TUs that really instantiate the map (variant_field.cpp,variant_field_test.cpp) include the real header directly. One subtlety:VariantField's class-body= defaultdefault constructor was an inline definition, and with the key type forward-declared it would instantiate theVariantMapdestructor through theunique_ptrdeleter — it moves to the .cpp (declarednoexcept, defaulted there); every other special member was already out-of-line.field.hends up with zero net change. A whole-tree audit of "namesPathInDatawithout directly including its header" (8 src + 19 test files) confirmed every one has an independent provider (column_variant.hand the variant reader/writer headers includepath_in_data.hthemselves); a second sweep for files that spell onlyVariantMap/legacy_mapcaught one more —column_variant_v2_test.cppvalue-constructsVariantMap {}and now includes the header directly.data_type_array_serde.cppgrew aFieldType::use upstream ([fix](variant) Forward-port Parquet and external Variant fixes to master #66413 series) after the mother-branch closure sweep, compiling only through the PCH'solap_common.h; with the PCH entry dropped it gets the directstorage/field_type.hinclude (folded into the pch commit).Verification
doris_belinks (319MiB).doris_be_testlinks (306MiB), zero duplicate/undefined symbols. The variant suites touched by the drift absorption ran green:VariantFieldTest.*+ColumnVariantV2*= 61/61 passed.-fsyntax-onlyagainst its real include closure, no PCH symbol leakage — tooling from [opt](build) Add build-timing and header-closure sweep tooling #66616); 4 rounds of incremental rebuild; 3 rounds of BE UT.SKIP_PRECOMPILE_HEADERS/ PCH interaction has a dedicated A/B: PCH-drop is wall-clock neutral (673.3s vs 674.1s), so the radius win is free.Deliberately disclosed
olap_common.hstill re-exports the three moved headers; nothing was migrated call-site-by-call-site. Peeling direct users off the re-export is possible follow-up, not needed for the win.rowset_id.h,field_type.h), which are plain moves.