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

Add more test coverage for Tsavorite primitives #1121

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

PaulusParssinen
Copy link
Contributor

@PaulusParssinen PaulusParssinen commented Mar 21, 2025

Add more SpanByte and its associated logic unit test coverage. Also contains some Tsavorite test code housekeeping.

TODO

  • More SpanByte specific unit tests to Tsavorite.test

@badrishc
Copy link
Collaborator

badrishc commented Mar 21, 2025

This may still be needed, as the MetadataSize check is different from Length check. A SpanByte may have an 8 byte optional header used for storing logical record expiration time (a bit tells us whether there is such a header). The MetadataSize check ensures that both left and right have such a metadata header. MetadataSize will be either 8 or 0, depending on this bit.

@PaulusParssinen
Copy link
Contributor Author

PaulusParssinen commented Mar 21, 2025

Ah, thanks for explanation. I should have looked where that getter leads to before jumping to conclusion. 🤦‍♂️

I probably should pivot this PR to add tests to catch this detail, that the KeyComparer must be able to distinguish SpanBytes if their MetadataSizes are different. Bit spooky that the CI passed

@PaulusParssinen PaulusParssinen marked this pull request as draft March 21, 2025 18:24
@PaulusParssinen PaulusParssinen changed the title Remove repeat length check from SpanByteComparer.Equals Add more test coverage to for Tsavorite primitives Mar 21, 2025
@PaulusParssinen PaulusParssinen changed the title Add more test coverage to for Tsavorite primitives Add more test coverage for Tsavorite primitives Mar 21, 2025
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