Listing options on cmdline #439
Conversation
Codecov Report
@@ Coverage Diff @@
## master #439 +/- ##
=======================================
Coverage 93.26% 93.26%
=======================================
Files 92 93 +1
Lines 4926 4941 +15
=======================================
+ Hits 4594 4608 +14
- Misses 332 333 +1
Continue to review full report at Codecov.
|
tools/compute_intersection.cpp
Outdated
| try { | ||
| with_index(app.index_encoding(), app.index_filename(), [&](auto index) { | ||
| intersect<decltype(index), wand_raw_index>( | ||
| app.index_filename(), |
There was a problem hiding this comment.
I am confused, if we do the mapping in the with_index function, why do we pass the index filename and not the index already mapped?
There was a problem hiding this comment.
We don't have the index here. This is the whole point, we don't know the type, and we can get away with it because we pass a template function that takes any index. Then it's all handled in with_index. It's a workaround to avoid having ifs and elses everywhere we load an index.
Maybe load_and_run would be a better name?
I think I mentioned it somewhere on slack, there might be an alternative to write a function that loads an index to std::variant. Then we would have to run
std::visit([&](auto index) {
// Do stuff.
}, load_index(...));
I don't see much advantage in this, it's essentially the same. It could make a difference if we want to pass the index around, which might be handy.
I actually suggest having both methods. One shortcut for just "run with this index type", and the other "give me an index, I'll handle matching later on my own".
In the latter case, I also suggest having a custom function, hence my naming. I imagined having:
load_index(encoding, path);
with_index(index_as_variant, fn);
with_index(encoding, path, fn);
Not sure about the name though. Maybe run_for_index? Idk, open to suggestions.
tools/selective_queries.cpp
Outdated
| try { | ||
| with_index(app.index_encoding(), app.index_filename(), [&](auto index) { | ||
| selective_queries<decltype(index)>( | ||
| app.index_filename(), app.index_encoding(), app.queries()); |
OverviewI refactor this to simplify the usage. There is a single public way of retrieving a "type" object for an index based on the encoding: auto index_type = IndexType::resolve("pef"); // or IndexType::resolve_profiling for the indexes with profilingThis is an object that internally holds a variant of index type markers, meaning objects that have the type of the index encoded as a typedef. This is necessary, because there are some situations where we need to know the type before creating the index itself, in particular when compressing an index. To run some code with the index type, we need to call: index_type.execute([](auto&& index_type) {
using Index = typename decltype(index_type)::type;
});However, as most commonly we want to load an index and do something with it, there's another method that loads the index and runs some code on it: index_type.load_and_execute(index_path, [](auto&& index) {
// decltype(index) is something like `block_simdbp_index`
});which is equivalent to: index_type.execute([&index_path](auto&& index_type) {
using Index = typename decltype(index_type)::type;
auto index = Index(MemorySource::mapped_file(index_path));
});Adding new index typeAll available index types are defined in the When adding a new index type (to avoid macros), one needs to edit |
|
Note: tests are failing because I'm waiting for #457 to be merged. |
|
I'll also work on documentation in code. |
There was a problem hiding this comment.
It looks much cleaner, I think this is definitely a nice improvement. I left a minor comment about a func name. @amallia May want to also take a look here since I missed the initial review for this one.
include/pisa/index_types.hpp
Outdated
| }; | ||
|
|
||
| template <bool Profile, std::size_t I, typename Fn> | ||
| auto resolve_index_n(std::string_view encoding, Fn fn) -> bool |
There was a problem hiding this comment.
Why is this called resolve_index_n, am I missing something?
There was a problem hiding this comment.
Well, I suppose it's a big inconsistent. What this one does is it attempts to resolve the encoding at position I (template parameter), and returns true if it succeeds. I guess it would be clearer if I used N instead. I'll also add some docs to all of these today.
|
GCC 7 keeps crashing with an internal bug, even though the same version compiles it just fine locally. I couldn't yet figure out what's wrong. I'll probably look at it again in a few days. It might be some issue with libstdc++ rather than the compiler alone but I'm not sure. |
|
RE. failing builds, I think this is a memory consumption issue. For reference, the peak memory usage for builds with a single thread on the
This is using gcc 7.3.0 on Ubuntu. |
Thanks for looking into this. I'll look into it in a spare moment. If it can't be fixed, we'll need to look for other solutions, this is not an acceptable tradeoff. |
|
@JMMackenzie I just ran the whole compilation from |
|
For posterity, using |
|
Looks like |
|
Right, the problem from this PR still persists, but fixing the other problem makes some tests pass. |
I also got rid of the index types macro replacing it with some template metaprogramming.