Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VarInt coding: Always perform sanity check #48740

Merged
merged 2 commits into from
Apr 15, 2023
Merged

Conversation

rschu1ze
Copy link
Member

@rschu1ze rschu1ze commented Apr 13, 2023

This is follow-up for the discussion in #48628. The sanity check against VAR_UINT_MAX is now unconditional, i.e. independent of the build type. Let's see how that affects performance. I will probably add Unchecked overloads later on - for now they are omitted to avoid clutter.

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

This is follow-up for the discussion in #48628. The sanity check against
VAR_UINT_MAX is now unconditional (independent of the build type). Let's
see how that affects performance ... I'll probably add "Unchecked"
overloads later on (to avoid clutter they are avoided for now)
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 13, 2023
@rschu1ze rschu1ze marked this pull request as ready for review April 14, 2023 08:07
@rschu1ze rschu1ze added the pr-performance-tests Forces full performance tests in CI label Apr 14, 2023
@alexey-milovidov alexey-milovidov self-assigned this Apr 15, 2023
@alexey-milovidov alexey-milovidov merged commit 75887c3 into master Apr 15, 2023
139 checks passed
@alexey-milovidov alexey-milovidov deleted the rs/varint-sanity branch April 15, 2023 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-not-for-changelog This PR should not be mentioned in the changelog pr-performance-tests Forces full performance tests in CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants