-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
enhance: Improve GetVectorById of Sparse Float Vector #33209
Conversation
d22efab
to
3f23438
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #33209 +/- ##
==========================================
+ Coverage 82.15% 82.18% +0.02%
==========================================
Files 1012 1003 -9
Lines 128957 129221 +264
==========================================
+ Hits 105944 106195 +251
- Misses 19038 19040 +2
- Partials 3975 3986 +11
|
2ee55e6
to
2b4a913
Compare
@zhengbuqian E2e jenkins job failed, comment |
/run-cpu-e2e |
@zhengbuqian E2e jenkins job failed, comment |
2b4a913
to
d87894e
Compare
@zhengbuqian E2e jenkins job failed, comment |
/run-cpu-e2e |
@zhengbuqian E2e jenkins job failed, comment |
/run-cpu-e2e |
just passed, seems the test is flaky |
ut in test_sealed.cpp is skipped due to #33210 |
internal/core/src/mmap/Column.h
Outdated
@@ -123,6 +128,7 @@ class ColumnBase { | |||
AssertInfo(data_ != MAP_FAILED, | |||
"failed to create file-backed map, err: {}", | |||
strerror(errno)); | |||
madvise(data_, mapped_size, MADV_WILLNEED); |
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.
madvise here may lead to memory increase. Perhaps it's best not to add it for now.
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.
ack, removed.
virtual const char* | ||
Data() const { | ||
return data_; | ||
} | ||
|
||
// MmappedData() returns the mmaped address | ||
const char* | ||
MmappedData() const { |
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.
any difference compared to the Data()?
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.
Data()
points at an array of elements, while MmappedData()
points at the entire mmap-ed memory. They are the same for dense vectors but not the same for sparse vectors. SparseFloatColumn
overrides Data()
to return something different.
/lgtm for the GetVector part |
Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
d87894e
to
46570e0
Compare
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: congqixia, zhengbuqian The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
issue: milvus-io#29419 * sparse float vector to support raw data mmap For get vector from chunk cache, I added a unit test but marking it as skipped due to a known issue. I have tested it locally. Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
issue: #29419
For get vector from chunk cache, I added a unit test but marking it as skipped due to a known issue. I have tested it locally.