Repository navigation
Enable implicit-int-float-conversion check - #94643
Merged
Merged
Conversation
Contributor
|
Workflow [PR], commit [a520cfd] Summary: ❌
|
clickhouse-gh
Bot
added
the
pr-not-for-changelog
antonio2368
force-pushed
the
enable-implicit-int-float-conversion
branch
4 times, most recently
from
January 21, 2026 09:48
9a702f2 to
c22808e
Compare
antonio2368
force-pushed
the
enable-implicit-int-float-conversion
branch
from
January 21, 2026 14:36
c22808e to
d27229d
Compare
antonio2368
marked this pull request as ready for review
January 23, 2026 12:10
robot-ch-test-poll3
added
the
pr-synced-to-cloud
pull Bot
pushed a commit
to Lobinson1/ClickHouse
that referenced
this pull request
Jul 30, 2026
TTLAggregationAlgorithm built its Aggregator::Params with min_chunk_bytes_for_parallel_parsing in the argument slot that expects min_hit_rate_to_use_consecutive_keys_optimization. The former is a byte threshold defaulting to 10 MiB, the latter a hit rate in [0, 1] defaulting to 0.5, so the threshold arrived as 10485760.0. Aggregator::executeImpl compares cache_hit_rate (at most 1.0) against that threshold, so use_cache was always false, Method::StateNoCache was selected, and StateImpl<false>::hasOnlyOneValueSinceLastReset() returns false unconditionally. That value gates the equal-key-range batching in executeAggregateInstructions, so addBatchSinglePlace was never taken and the consecutive keys optimization was permanently disabled for TTL GROUP BY aggregation. This is the path that benefits most from it: execute() calls calculateAggregates() once per run of equal keys, so every executeOnBlock receives a single-key chunk. Introduced by ClickHouse#57872, which added the parameter and used the correct setting at every other call site; the narrowing conversion that would have flagged it was later silenced by the static_cast<float> from ClickHouse#94643. Present since v23.12.1.1368-stable. Results are not affected, only performance. There is no parallel parsing in this file, so the now-unused min_chunk_bytes_for_parallel_parsing extern is removed. Measured on the added test (200000 rows, 100 keys, one aggregating TTL merge): ProfileEvents['AggregationOptimizedEqualRangesOfKeys'] goes from 0 to 588 while count() and sum(v) stay identical. Closes: ClickHouse#112075
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.
Changelog category (leave one):
This one is still acceptable when it comes to amount of changes, but it does force you to cast all parts of some expression to floating point.
So you cannot just cast one part and rely on implicit casts to floating point to handle the rest, something which is often done.
Documentation entry for user-facing changes
Version info
26.1.1.883