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

[ENH]: Rust frontend proptest for collection operations against SQLite impl #3814

Open
wants to merge 1 commit into
base: feat-in-memory-frontend
Choose a base branch
from

Conversation

codetheweb
Copy link
Contributor

@codetheweb codetheweb commented Feb 19, 2025

Description of changes

Adds a Rust proptest for collection operations against the SQLite implementation. Follow up work includes:

  • adding a test against distributed (requires changes to the compaction API)
  • enabling KNN queries
  • improving shrinking
  • backtesting against previous correctness bugs

The proptest outputs stats about generated query selectivity after running as naive generation of requests for .get() will usually result in selecting all or no records. Example output:

Statistics:
  A total of 15646 log operations were created.
  .get() selectivity (570 total requests):
    .get() with a where clause only (64.74%):
      47.97% of queries returned no results
      23.31% of queries returned some results
      28.73% of queries returned all results
    .get() with a where clause & IDs (22.11%):
      69.05% of queries returned no results
      30.95% of queries returned some results
      00.00% of queries returned all results
    .get() with IDs only (13.16%):
      42.67% of queries returned no results
      57.33% of queries returned some results
      00.00% of queries returned all results

Note that 23% of queries returned only part of the collection when filtering with a where clause. This doesn't sound super high, but it started at 3-4% before I fixed filter generation to bias towards known values. We can make this higher if we want by tweaking a few parameters.

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

n/a

Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

Copy link
Contributor Author

codetheweb commented Feb 19, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch 10 times, most recently from 7e087f8 to e8aee9a Compare February 22, 2025 01:28
@codetheweb codetheweb force-pushed the feat-manual-compaction-from-frontend branch from 85b22da to 9c0bb95 Compare February 28, 2025 18:35
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from e8aee9a to 25c8518 Compare February 28, 2025 18:35
@codetheweb codetheweb force-pushed the feat-manual-compaction-from-frontend branch from 9c0bb95 to 9f52e57 Compare March 4, 2025 22:11
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from 25c8518 to 256940a Compare March 4, 2025 22:11
@codetheweb codetheweb force-pushed the feat-manual-compaction-from-frontend branch from 9f52e57 to 0142a9c Compare March 6, 2025 18:26
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from 256940a to 01f72a0 Compare March 6, 2025 18:26
@codetheweb codetheweb force-pushed the feat-manual-compaction-from-frontend branch from 0142a9c to 5cfc254 Compare March 6, 2025 18:31
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from 01f72a0 to a751c89 Compare March 6, 2025 18:31
@codetheweb codetheweb force-pushed the feat-manual-compaction-from-frontend branch from 5cfc254 to 0e896c9 Compare March 6, 2025 18:32
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from a751c89 to ef63c70 Compare March 6, 2025 18:32
@codetheweb codetheweb changed the base branch from feat-manual-compaction-from-frontend to feat-in-memory-frontend March 6, 2025 20:39
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from ef63c70 to 25da282 Compare March 6, 2025 20:45
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch 2 times, most recently from c0c65cf to bd5e73e Compare March 6, 2025 21:22
@codetheweb codetheweb force-pushed the feat-in-memory-frontend branch from 1d59d07 to ee35e1f Compare March 6, 2025 21:25
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from bd5e73e to 01bf625 Compare March 6, 2025 21:25
@codetheweb codetheweb force-pushed the feat-in-memory-frontend branch from ee35e1f to b0c6dd0 Compare March 6, 2025 21:28
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch 3 times, most recently from cff87c2 to 471004f Compare March 6, 2025 22:10
@codetheweb codetheweb force-pushed the feat-in-memory-frontend branch from b0c6dd0 to db72c7c Compare March 6, 2025 23:17
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch 7 times, most recently from 59e8d53 to dbb39bb Compare March 7, 2025 00:34
@codetheweb codetheweb changed the title [ENH]: Rust frontend proptest [ENH]: Rust frontend proptest for collection operations against SQLite impl Mar 7, 2025
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from dbb39bb to 1431bd2 Compare March 7, 2025 00:42
@codetheweb codetheweb marked this pull request as ready for review March 7, 2025 00:43
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch 2 times, most recently from 12507d1 to 61f0604 Compare March 7, 2025 18:29
@codetheweb codetheweb force-pushed the feat-in-memory-frontend branch from db72c7c to 23f31bd Compare March 7, 2025 18:29
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from 61f0604 to 8fbb90d Compare March 7, 2025 18:29
codetheweb added a commit that referenced this pull request Mar 7, 2025
## Description of changes

Found this bug while working on
#3814.

## Test plan
*How are these changes tested?*

- [x] Tests pass locally with `pytest` for python, `yarn test` for js,
`cargo test` for rust

Added test was previously failing.

## Documentation Changes
*Are all docstrings for user-facing APIs updated if required? Do we need
to make documentation changes in the [docs
repository](https://github.com/chroma-core/docs)?*

n/a
@codetheweb codetheweb force-pushed the feat-in-memory-frontend branch from 23f31bd to 7b62478 Compare March 7, 2025 23:39
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from 8fbb90d to 597dd89 Compare March 7, 2025 23:40
@codetheweb codetheweb force-pushed the feat-in-memory-frontend branch from 7b62478 to c0a6303 Compare March 7, 2025 23:44
@codetheweb codetheweb force-pushed the feat-rust-frontend-proptest branch from 597dd89 to 43b6fed Compare March 7, 2025 23:44
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.

1 participant