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
Improved ql:has-relation versatility. #72
Conversation
0ea60c5
to
a82fbbe
Compare
Can you add queries covering the different uses of |
src/engine/HasRelationScan.cpp
Outdated
os << " "; | ||
} | ||
switch (_type) { | ||
// TODO(florian): these are not good cache keys |
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.
You mean the tree->asString()
in general or these specifically?
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.
HasRelationScan's keys originally contained variable names, I'd fixed that but apparently forgot to remove the TODO.
src/engine/HasRelationScan.cpp
Outdated
} | ||
|
||
size_t HasRelationScan::getSizeEstimate() { | ||
// TODO: these size estimates only work if all predicates are functional |
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.
which they aren't right? So is this a problem?
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.
I apparently forgot to fix this after adding the computation of the has-relation size and multiplicities to the index. Replaced the rather wild guesses used before with the apporximations used by the join operation.
src/engine/HasRelationScan.cpp
Outdated
new std::vector<std::array<Id, 1>>(); | ||
result->_fixedSizeData = fixedSizeData; | ||
|
||
size_t id = 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.
I think should by of type Id
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.
fixed
src/engine/HasRelationScan.cpp
Outdated
} | ||
|
||
template <typename T, typename C> | ||
struct resizeIfVec { |
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 needs comments explaining why and where this is used
static_cast<vector<array<Id, InputColCount + 1>>*>( | ||
result->_fixedSizeData), | ||
hasPattern, hasRelation, patterns); | ||
} else { |
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.
Add a comment to make it clear that this is only for the == 5
case where the input uses _fixedSizeData
still but the output already uses _varSizeData
.
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.
done
src/engine/QueryPlanner.cpp
Outdated
|
||
std::shared_ptr<QueryExecutionTree> hasRelationScan( | ||
new QueryExecutionTree(_qec)); | ||
std::shared_ptr<QueryExecutionTree> other( |
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.
make_shared
@@ -424,7 +424,7 @@ string Server::composeResponseJson(const string& query, | |||
|
|||
string msg = ad_utility::escapeForJson(exception.what()); | |||
|
|||
os << "\"Exception-Error-Message\": \"" << msg << "\"\n" | |||
os << "\"exception\": \"" << msg << "\"\n" |
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.
Do the QLever UI folks know about this change already?
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.
I hadn't realized this could cause problems for them. I changed the name to be consistent with the ad_semsearch::Exception
naming, as the old ui does not print the error message otherwise.
src/index/Index.cpp
Outdated
file.write(entityHasPattern.data(), sizeof(Id) * numHasPatterns * 2); | ||
|
||
// write the entityHasRelation vector | ||
size_t numHasRelation = entityHasRelation.size(); |
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.
numHasRelations
(add s) to make it consistent with numHasPatterns
file.close(); | ||
|
||
hasPattern.resize(entityHasPattern.back()[0] + 1); | ||
// hasRelation.resize(entityHasRelation.back()[0] + 1); | ||
LOG(INFO) << "Done creating patterns file." << std::endl; |
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.
Maybe also print how many patterns we found
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.
I added the number of patterns to the rest of the info a couple of lines above this one.
src/index/Index.cpp
Outdated
while (buffer[i][0] > pos) { | ||
_hasPattern[pos] = NO_PATTERN; | ||
pos++; | ||
// Store all data in the file |
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.
We aren't storing but reading
e2e/scientists_queries.yaml
Outdated
?entity ql:has-relation ?r. | ||
} | ||
checks: | ||
# greater than the current limit |
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.
Copy and paste error.
src/engine/QueryPlanner.cpp
Outdated
// Check if one of the two operations is a HAS_RELATION_SCAN . | ||
// If the join column correspods the the has-relation scans | ||
// Check if one of the two operations is a HAS_RELATION_SCAN. | ||
// If the join column corresponds to the the has-relation scan's |
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.
still contains "the the"
cf9cadc
to
6543351
Compare
ql:has-relation can now be used like any other predicate, with the pattern trick still being used to answer queries matching its specific requirements.
I also ran clang-format on the entire repository which changed a handful of files and added those in the last commit of this pr.