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

Avoid div-by-zero in row count estimations for empty tables #1992

Closed
wants to merge 2 commits into from

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Nov 12, 2024

Description of Changes

Forward-port of #1991 , an 0.11.1 hotfix for the BitCraft alpha, into master.

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

None

Expected complexity level and risk

1

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • Check that it works for the BitCraft alpha.
  • Maybe write a smoketest?

Forward-port of an 0.11.1 hotfix for 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.
@kim
Copy link
Contributor

kim commented Nov 13, 2024

#1985 and #1986 addressed the same, you may want to pick #1986 as a regression test

@gefjon
Copy link
Contributor Author

gefjon commented Nov 13, 2024

#1985 and #1986 addressed the same, you may want to pick #1986 as a regression test

Ah, I didn't realize we already had patches against master for this. Will close.

@gefjon gefjon closed this Nov 13, 2024
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.

2 participants