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

feat: [sc-48511] Fix sparse array write strategy "too many local rejects" #107

Merged
merged 5 commits into from
May 30, 2024

Conversation

rroelke
Copy link
Member

@rroelke rroelke commented May 30, 2024

Story details: https://app.shortcut.com/tiledb-inc/story/48511

Generating a write input for a sparse array may require generating unique coordinates.

The current strategy generates coordinates which are unique for each dimension individually. If a single dimension is constrained to the domain 0.. 10, for example, then the strategy cannot possibly produce more than 10 records of input, no matter how many other dimensions there are.

This means that the test generator will never cover the whole space of a sparse dimension.

The implementation of choosing the unique values within a dimension also is not very smart. It uses proptest::collection::btree_set, which generates a bunch of values and then throws the whole thing away to try again if it is below the requested number of records. The probability of rejection increases a lot as the dimension gets more and more constrained, and we have seen in the occasional run that proptest eventually gives up if it cannot satisfy the minimum number of records.

For all of the above we must change our strategy to choose unique values across all dimensions instead.

This pull request fixes the issue by modifying the Cells strategy to first generate the maximum number of records for the dimensions, then deduplicate the coordinates. Then we generate the rest of the attributes to write using the new number of records.

@rroelke rroelke requested a review from davisp May 30, 2024 18:33
Copy link
Collaborator

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@rroelke rroelke merged commit 7b20182 into main May 30, 2024
2 checks passed
@rroelke rroelke deleted the rr/sc-48511-sparse-write-proptest-unique-coordinates branch May 30, 2024 19:03
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