Repository navigation
TTL GROUP BY passes min_chunk_bytes_for_parallel_parsing as min_hit_rate_to_use_consecutive_keys_optimization #112075
Description
ClickHouse/src/Processors/TTL/TTLAggregationAlgorithm.cpp
Lines 117 to 120 in b849a6c
/*only_merge=*/false,
settings[Setting::optimize_group_by_constant_keys],
static_cast<float>(settings[Setting::min_chunk_bytes_for_parallel_parsing]),
/*stats_collecting_params_=*/{},That argument of Aggregator::Params is min_hit_rate_to_use_consecutive_keys_optimization_, a hit rate in [0, 1], default 0.5. min_chunk_bytes_for_parallel_parsing is a byte threshold, default 10 MiB. So the threshold becomes 10485760.0, cache_hit_rate in Aggregator::executeImpl is at most 1.0, and the consecutive keys optimization is always disabled in TTL GROUP BY aggregation. Since 23.12. Perf only, results are not affected. There is no parallel parsing anywhere in this file, the extern for min_chunk_bytes_for_parallel_parsing exists only to feed this argument.
Copy-paste error from #57872, which introduced the parameter and updated every other Aggregator::Params call site with the correct setting. #94643 later wrapped it in static_cast<float> and silenced the exact UInt64 to float conversion warning that would have caught it. I checked all Aggregator::Params construction sites in src/ (both ctors), this is the only wrong one.
Fix: declare extern const SettingsFloat min_hit_rate_to_use_consecutive_keys_optimization in the Setting namespace of the file, pass it instead, and drop the now unused min_chunk_bytes_for_parallel_parsing extern.
Activity
harikrishnan94 commentedon Jul 27, 2026
@groeneai fix
groeneai commentedon Jul 27, 2026
Acknowledged -- I reproduced the analysis on
origin/masterand it holds. I'll follow up here with a PR.Re-derived, so the record is complete:
Argument position confirmed.
Aggregator::Params' full ctor (src/Interpreters/Aggregator.h:152-174) takesfloat min_hit_rate_to_use_consecutive_keys_optimization_right afteroptimize_group_by_constant_keys_, andTTLAggregationAlgorithm::TTLAggregationAlgorithmpassesmin_chunk_bytes_for_parallel_parsingthere.min_chunk_bytes_for_parallel_parsingisNonZeroUInt64default10 * 1024 * 1024(Settings.cpp:1550);min_hit_rate_to_use_consecutive_keys_optimizationisFloatdefault0.5(Settings.cpp:6079). InAggregator::executeImplthe comparison iscache_hit_rate >= params.min_hit_rate_to_use_consecutive_keys_optimizationwithcache_hit_rateat most1.0, souse_cacheis unconditionally false on this path.Your completeness claim checks out. I swept every
Aggregator::Paramsconstruction site insrc/against both ctors:InterpreterSelectQuery.cpp:2401and:2990,Planner.cpp:653and:785,AggregatingStep.cpp:1113,MergingAggregatedStep.cpp:311,LazyReadReplacingFinalSource.cpp:240,MergingAggregatedTransform.cpp:88(forwardsparams.min_hit_rate_...), plus the gtest atgtest_aggregating_step_clears_reduced_flag.cpp:47(0.5f). All correct.TTLAggregationAlgorithm.cppis the only wrong one, and itsmin_chunk_bytes_for_parallel_parsingextern has exactly the two occurrences you describe -- the declaration and this argument.Blame confirmed:
10bc2db56d2425(Better optimization of consecutive keys in aggregation #57872) added the argument and set the correct setting at every other site it touched in the same commit;dc3d47d6899a0b(Enable implicit-int-float-conversion check #94643) added thestatic_cast<float>. First release containing it isv23.12.1.1368-stable.One thing worth adding: disabling the cache costs more than the last-element lookup.
use_cache == falseselectsMethod::StateNoCache, whosehasOnlyOneValueSinceLastReset()returnsfalseunconditionally (ColumnsHashingImpl.h:341-346), andAggregator::executeAggregateInstructionsgates the equal-key-range batching on exactly that value -- soaddBatchSinglePlaceover a whole run of equal keys is never taken either, and theAggregationOptimizedEqualRangesOfKeysprofile event stays at zero for TTL GROUP BY. That makes the TTL path the worst place to lose it:TTLAggregationAlgorithm::executewalks the block detecting group-key changes and callscalculateAggregatesonce per run of equal keys, i.e. everyexecuteOnBlockcall is a single-key run -- the exact shape the optimization exists for.I'll keep the PR to the one concern: declare
extern const SettingsFloat min_hit_rate_to_use_consecutive_keys_optimization, pass it, drop the now unusedmin_chunk_bytes_for_parallel_parsingextern. Changelog categoryPerformance Improvement, since results are unaffected as you note.groeneai commentedon Jul 28, 2026
PR: #112314
It is the two-line change you described: the argument now passes
min_hit_rate_to_use_consecutive_keys_optimization, and themin_chunk_bytes_for_parallel_parsingextern (which existed only to feed that slot) is removed.Two things worth flagging beyond the fix itself:
04648_ttl_group_by_consecutive_keys_optimization) assertsProfileEvents['AggregationOptimizedEqualRangesOfKeys'] > 0on theMergePartsrow insystem.part_logafter aTTL ... GROUP BYmerge. That event reads0before the fix and non-zero after, and reverting only the argument reproduces the failure as a one-line reference diff whilecount()/sum(v)stay byte-identical, which is the "perf only, results are not affected" half of your report measured rather than asserted.Performance Comparisonjobs, for which the PR addstests/performance/ttl_group_by_consecutive_keys.xmltiming the aggregating TTL merge.One note on reachability that may matter for anyone hitting this in production: the call site reads the global context (
MergeTreeDataholdscontext_->getGlobalContext()), so this setting cannot be influenced byOPTIMIZE ... SETTINGS, a sessionSET, or a client option. Only the server's default profile reaches it.Pass min_hit_rate_to_use_consecutive_keys_optimization in TTL GROUP BY