pyknowhere - support dump and load index file #145
Conversation
alwayslove2013
changed the title
|
@alwayslove2013 🔍 Important: PR Classification Needed! For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:
For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”. Thanks for your efforts and contribution to the community!. |
python/knowhere/knowhere.i
Outdated
| auto binary_map = binset -> binary_map_; | ||
| std::ofstream outfile; | ||
| outfile.open(file_name, std::ios::out | std::ios::trunc); | ||
| for (auto it = binary_map.begin(); it != binary_map.end(); ++it) { |
There was a problem hiding this comment.
please check is the file was opened successfully via if (outfile.good()) {}
python/knowhere/knowhere.i
Outdated
| Load(knowhere::BinarySetPtr binset, const std::string& file_name) { | ||
| std::ifstream infile; | ||
| infile.open(file_name, std::ios::in); | ||
| size_t name_len; |
There was a problem hiding this comment.
please check is the file was opened via if (infile.good()) {}
python/knowhere/knowhere.i
Outdated
| // serialization: name_length(size_t); name(char[]); binset_size(size_t); binset(uint8[]); | ||
| auto name = it->first; | ||
| outfile << name.size(); | ||
| outfile << name; |
There was a problem hiding this comment.
name.size() is type size_t and is platform dependent, please write and read sizes as uint64_t
python/knowhere/knowhere.i
Outdated
| int64_t size; | ||
| infile >> size; | ||
|
|
||
| auto data = new uint8_t[size]; |
There was a problem hiding this comment.
any checks if the size is too large, negative or making no sense? :)
There was a problem hiding this comment.
negative checked.
how to check for too large size? or we can just wait for the program to report an exception?
python/knowhere/knowhere.i
Outdated
|
|
||
| auto data = new uint8_t[size]; | ||
| infile.read(reinterpret_cast<char*>(data), size); | ||
| std::shared_ptr<uint8_t[]> data_ptr(data); |
There was a problem hiding this comment.
std::shared_ptr<> first, read data into it next
There was a problem hiding this comment.
how about this
auto data = new uint8_t[size];
std::shared_ptr<uint8_t[]> data_ptr(data);
infile.read(reinterpret_cast<char*>(data_ptr.get()), size);
There was a problem hiding this comment.
yes.
Also, please note that this code needs C++17, check https://stackoverflow.com/questions/13061979/can-you-make-a-stdshared-ptr-manage-an-array-allocated-with-new-t , I don't know how this shared_ptr<uint8_t[]> will be used later.
python/knowhere/knowhere.i
Outdated
| } | ||
| // end with 0 | ||
| outfile << 0; | ||
| outfile.close(); |
There was a problem hiding this comment.
outfile.close() is not needed here. I would rather use outfile.flush();
python/knowhere/knowhere.i
Outdated
|
|
||
| binset->Append(name, data_ptr, size); | ||
| } | ||
| infile.close(); |
There was a problem hiding this comment.
infile.close(); is not needed
|
/lgtm |
|
It needs to be noted somewhere that reading/writing errors are not exposed |
Signed-off-by: min.tian <[email protected]> review Signed-off-by: min.tian <[email protected]> fix pre-commit Signed-off-by: min.tian <[email protected]>
alwayslove2013
force-pushed
the
pyknowhere_benchmark
branch
from
2404bc1 to
4e1f78a
Compare
|
@alexanderguzhva The load and dump API changes will only appear in By the way, I have squashed all commits into one, and |
|
/lgtm |
|
@alwayslove2013 issue link plz |
|
related issue: #154 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alwayslove2013, liliu-z 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 |
/kind feature
Issue: #154