-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[BUG] Count should handle uninit'ed segments #2193
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
match *e { | ||
RecordSegmentReaderCreationError::UninitializedSegment => { | ||
// This means no compaction has occured. | ||
return Ok(CountRecordsOutput { count: 0 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect. In this case, the count should be the count from the logs and not 0. My 2c is to do it correctly now instead of deferring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh woops :). you are correct! my bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm actually no. I don't think this is wrong. The calling code adds the log count in.
match channel.send(Ok(msg.count + self.log_record_count)) { |
I think in #2185 you change the structure which makes the comment above relevant.
Would you prefer I wait that is landed and merge accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are correct. I had #2185 in mind when making this comment. I had mentally merged the two PRs in my mind (since that is what would've happened when resolving conflicts). I think we should wait for that to be landed
## Description of changes *Summarize the changes made by this PR.* - Improvements & Bug fixes - Count should handle uninit'ed segments - New functionality - None ## Test plan *How are these changes tested?* chroma-core#2185 should add tests for the count() operator - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Documentation Changes None
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
#2185 should add tests for the count() operator
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
None