Skip to content

[Enhancement] Split cast/agg template-cartesian TUs to cut compile time - #76458

Merged
kangkaisen merged 2 commits into
StarRocks:mainfrom
Hyper-FF:castagg
Jul 17, 2026
Merged

[Enhancement] Split cast/agg template-cartesian TUs to cut compile time#76458
kangkaisen merged 2 commits into
StarRocks:mainfrom
Hyper-FF:castagg

Conversation

@Hyper-FF

@Hyper-FF Hyper-FF commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Why I'm doing:

Several expression/aggregate factory TUs were dominated by large 2-D template cartesians (arg-type x ret-type / from-type x to-type), making them among the slowest single translation units in the BE build. Split each, behavior-preservingly, across parallel TUs: move the classes/templates/macros into a shared _tpl.hpp and distribute the cartesian over group .cpp files, with the original file routing by one type dimension.

  • aggregate_resolver_avg: split out sum_map -> aggregate_resolver_sum_map{1,2,3}.cpp (641s -> 201 + 150/154/117s)
  • case_expr: result-type cartesian -> case_expr_g{1,2,3}.cpp (536s -> 14 + 203/176/157s)
  • aggregate_resolver_minmaxany: split out max_by/min_by ->
    aggregate_resolver_maxminby{1,2,3}.cpp (346s -> 187 + 90/90/76s)
  • cast_expr: primitive from-type x to-type cartesian -> cast_expr_g{1,2,3}.cpp (258s -> 79 + 87/88/72s)

Behavior-preserving (identical type coverage). The wall-clock pole of each TU drops ~60%; total CPU rises slightly from per-group header re-parse. Isolated compile times measured without PCH.

What I'm doing:

Fixes #issue

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function
    • This pr needs auto generate documentation
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 4.1
    • 4.0
    • 3.5

Several expression/aggregate factory TUs were dominated by large 2-D template
cartesians (arg-type x ret-type / from-type x to-type), making them among the
slowest single translation units in the BE build. Split each, behavior-preservingly,
across parallel TUs: move the classes/templates/macros into a shared _tpl.hpp and
distribute the cartesian over group .cpp files, with the original file routing by
one type dimension.

- aggregate_resolver_avg: split out sum_map -> aggregate_resolver_sum_map{1,2,3}.cpp
  (641s -> 201 + 150/154/117s)
- case_expr: result-type cartesian -> case_expr_g{1,2,3}.cpp
  (536s -> 14 + 203/176/157s)
- aggregate_resolver_minmaxany: split out max_by/min_by ->
  aggregate_resolver_maxminby{1,2,3}.cpp (346s -> 187 + 90/90/76s)
- cast_expr: primitive from-type x to-type cartesian -> cast_expr_g{1,2,3}.cpp
  (258s -> 79 + 87/88/72s)

Behavior-preserving (identical type coverage). The wall-clock pole of each TU drops
~60%; total CPU rises slightly from per-group header re-parse. Isolated compile times
measured without PCH.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@CLAassistant

CLAassistant commented Jul 16, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ wanpengfei-git
❌ Hyper-FF
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions
github-actions Bot requested review from stdpain and trueeyu July 16, 2026 07:22
@github-actions

Copy link
Copy Markdown
Contributor

Module-risk briefing for Codex review

This is review focus, not a finding list. Validate against the diff.

High-confidence risks

  • Module: exprs/agg
    Changed files: be/src/exprs/agg/factory/aggregate_factory.cpp, aggregate_resolver.hpp, aggregate_resolver_avg.cpp, aggregate_resolver_maxminby.hpp, aggregate_resolver_maxminby{1,2,3}.cpp, aggregate_resolver_minmaxany.cpp, aggregate_resolver_sum_map.hpp, aggregate_resolver_sum_map{1,2,3}.cpp
    Historical failure pattern: registration/lifecycle and type-coverage gaps introduced when aggregate resolver code is split, moved, or ported across TUs (missing track_function() wrapper, cache-key/arg-count mismatches, template-parameter mismatches breaking builds).
    Review focus: this PR splits register_avg's sum_map cartesian into register_sum_map{1,2,3}() and register_minmaxany's max_by/min_by cartesian into register_maxminby{1,2,3}(). Confirm the three key-type/outer-type partitions in each split are an exact, non-overlapping, non-omitting partition of the original combined type list (sum_map key types across sum_map1/2/3; max_by/min_by outer types across maxminby1/2/3), and that all six new register_* functions are actually invoked from AggregateFuncResolver::AggregateFuncResolver() (they are, in aggregate_factory.cpp) — a silently-dropped partition or unregistered function would be indistinguishable from working code until a specific type combination is queried.
    Evidence: [BugFix] fix clang build failure in template parameter #70932, [BugFix] fix asan crash #67678, [BugFix] Fix memory leak in count aggregate function registration #67490, [BugFix] vararg Udaf's cache key should contains num_args #72102 (from 19 merged bugfixes)
  • Module: exprs
    Changed files: be/src/exprs/case_expr.cpp, case_expr.h, case_expr_g{1,2,3}.cpp, case_expr_tpl.hpp, cast_expr.cpp, cast_expr_g{1,2,3}.cpp, cast_expr_tpl.hpp
    Historical failure pattern: VectorizedCaseExpr::generate_ir_impl previously selected the LLVM compare instruction by ResultType instead of WhenType, producing malformed JIT IR when the two types diverge in float-ness (PR [BugFix] Fix invalid JIT IR for CASE WHEN with mixed float/int when and result types #74382).
    Review focus: case_expr_tpl.hpp moves generate_ir_impl (and the WhenType/ResultType-dependent JIT codegen branch) verbatim out of case_expr.cpp — verify the WhenType-based branch selection from the [BugFix] Fix invalid JIT IR for CASE WHEN with mixed float/int when and result types #74382 fix survived the move intact and wasn't accidentally reverted to ResultType. Also verify the three-way result-type partitions in case_expr_g{1,2,3}.cpp and cast_expr_g{1,2,3}.cpp cover every LogicalType the original single-file switch covered exactly once (no type silently dropped, causing create_primitive_cast/case_from_thrift to return nullptr for a previously-supported type), and that the routing in case_expr.cpp/cast_expr.cpp correctly chains to all three groups.
    Evidence: [BugFix] Fix invalid JIT IR for CASE WHEN with mixed float/int when and result types #74382 (from 20 merged bugfixes)

@CelerData-Reviewer

Copy link
Copy Markdown

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 530ad6a726

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@github-actions

Copy link
Copy Markdown
Contributor

[FE Incremental Coverage Report]

pass : 0 / 0 (0%)

@github-actions

Copy link
Copy Markdown
Contributor

[Java-Extensions Incremental Coverage Report]

pass : 0 / 0 (0%)

@github-actions

Copy link
Copy Markdown
Contributor

[BE Incremental Coverage Report]

fail : 1059 / 1358 (77.98%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 be/src/exprs/case_expr_g3.cpp 3 11 27.27% [23, 24, 25, 26, 27, 29, 30, 31]
🔵 be/src/exprs/case_expr_g2.cpp 6 11 54.55% [23, 25, 27, 28, 30]
🔵 be/src/exprs/case_expr_g1.cpp 6 11 54.55% [24, 27, 28, 29, 30]
🔵 be/src/exprs/cast_expr_tpl.hpp 614 843 72.84% [144, 152, 153, 197, 202, 208, 209, 210, 213, 215, 217, 232, 234, 235, 236, 241, 242, 243, 244, 269, 270, 271, 293, 294, 295, 296, 299, 300, 306, 307, 308, 326, 327, 332, 333, 338, 340, 341, 342, 344, 356, 357, 359, 362, 382, 383, 384, 404, 405, 406, 425, 426, 432, 433, 434, 452, 453, 463, 464, 474, 488, 491, 492, 493, 517, 525, 526, 545, 546, 547, 564, 565, 566, 596, 597, 612, 615, 616, 617, 618, 629, 632, 633, 634, 635, 637, 646, 656, 658, 676, 680, 681, 683, 689, 700, 704, 705, 706, 707, 709, 710, 714, 715, 724, 728, 729, 730, 731, 738, 752, 753, 754, 755, 764, 765, 768, 769, 770, 774, 775, 776, 784, 785, 787, 788, 791, 792, 793, 794, 795, 796, 797, 798, 799, 800, 802, 816, 817, 818, 834, 835, 836, 854, 855, 863, 864, 865, 879, 880, 881, 883, 884, 885, 886, 887, 903, 904, 905, 921, 922, 923, 955, 956, 970, 972, 973, 976, 977, 987, 999, 1000, 1024, 1025, 1026, 1043, 1044, 1045, 1068, 1076, 1077, 1079, 1080, 1082, 1083, 1084, 1085, 1120, 1121, 1132, 1133, 1134, 1135, 1144, 1145, 1146, 1147, 1154, 1155, 1165, 1166, 1171, 1172, 1215, 1216, 1222, 1228, 1229, 1249, 1250, 1291, 1293, 1295, 1353, 1355, 1358, 1360, 1405, 1406, 1407, 1408, 1506, 1508, 1514, 1520, 1541, 1542, 1586, 1602, 1603]
🔵 be/src/exprs/case_expr_tpl.hpp 286 338 84.62% [60, 72, 100, 119, 124, 125, 135, 159, 169, 173, 174, 188, 189, 191, 193, 194, 195, 196, 197, 199, 201, 203, 204, 205, 206, 207, 216, 217, 218, 228, 240, 258, 259, 260, 261, 262, 263, 264, 265, 267, 269, 270, 272, 273, 275, 278, 279, 281, 282, 283, 387, 555]
🔵 be/src/exprs/case_expr.cpp 10 10 100.00% []
🔵 be/src/exprs/cast_expr_g3.cpp 6 6 100.00% []
🔵 be/src/exprs/agg/factory/aggregate_resolver_maxminby.hpp 13 13 100.00% []
🔵 be/src/exprs/cast_expr_g2.cpp 8 8 100.00% []
🔵 be/src/exprs/agg/factory/aggregate_resolver_sum_map1.cpp 3 3 100.00% []
🔵 be/src/exprs/agg/factory/aggregate_resolver_maxminby3.cpp 14 14 100.00% []
🔵 be/src/exprs/agg/factory/aggregate_resolver_maxminby2.cpp 15 15 100.00% []
🔵 be/src/exprs/cast_expr_g1.cpp 8 8 100.00% []
🔵 be/src/exprs/cast_expr.cpp 19 19 100.00% []
🔵 be/src/exprs/agg/factory/aggregate_resolver_sum_map2.cpp 3 3 100.00% []
🔵 be/src/exprs/agg/factory/aggregate_resolver_maxminby1.cpp 15 15 100.00% []
🔵 be/src/exprs/agg/factory/aggregate_resolver_sum_map.hpp 21 21 100.00% []
🔵 be/src/exprs/agg/factory/aggregate_resolver_sum_map3.cpp 3 3 100.00% []
🔵 be/src/exprs/agg/factory/aggregate_factory.cpp 6 6 100.00% []

@kangkaisen
kangkaisen merged commit cc85672 into StarRocks:main Jul 17, 2026
96 of 118 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants