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

Use NonZeroU64 for num_distinct_values to avoid div-by-zero #1991

Open
wants to merge 4 commits into
base: release/v0.11.1-beta
Choose a base branch
from

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Nov 12, 2024

Description of Changes

Hotfix for a div-by-zero panic encountered by the BitCraft Alpha.

What was happening was, num_distinct_values on an empty table returned 0, which index_row_est then used as a divisor, leading to a panic. It seems reasonably clear to me that the correct estimate of distinct values in an empty index is 0, not bottom, so adjusting num_distinct_values to return Option<NonZeroU64>, and then making index_row_est return 0 in that case, seems an obvious fix.

API and ABI breaking changes

N/a.

Expected complexity level and risk

1 - Hard to be more broken.

How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

Testing

  • I am unsure how to test this. Perhaps we can deploy to staging, then do whatever the BitCraft team did that was broken, and verify that it no longer panics?

@bfops bfops added release-any To be landed in any release window bugfix Fixes something that was expected to work differently labels Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes something that was expected to work differently release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants