Optimizer is now aware of cached subresults #171
Conversation
There was a problem hiding this comment.
Looks good in general, but I'm still a bit concerned about the possible deadlock we might have seen. Looking at the code I think that if we look up the query execution tree for a tree we are currently building, we will find that in the cache with a ResultTable that is still being computed and trying to wait on it via awaitFinished() we would dead lock waiting on the same thread. However I'd expect this to happen every time so I'm a little confused. One way to mitigate this would be to only cache results which are fully computed, while this is clearly a race condition it would only result in a potential optimization loss.
src/engine/CMakeLists.txt
Outdated
| @@ -29,6 +29,6 @@ add_library(engine | |||
| HasPredicateScan.cpp HasPredicateScan.h | |||
| Union.cpp Union.h | |||
| MultiColumnJoin.cpp MultiColumnJoin.h | |||
| ) | |||
| ../util/vector2d.h ../util/ApplyVarsizeOperation.h) | |||
There was a problem hiding this comment.
I think this is an unrelated change for the planned ResultTable redesign?
| @@ -35,25 +35,23 @@ string QueryExecutionTree::asString(size_t indent) { | |||
| for (size_t i = 0; i < indent; ++i) { | |||
| indentStr += " "; | |||
| } | |||
| if (_asString.size() == 0) { | |||
There was a problem hiding this comment.
If we want to keep this caching we could also remember the indent that was used, right?
src/util/ApplyVarsizeOperation.h
Outdated
| @@ -0,0 +1,82 @@ | |||
| // | |||
There was a problem hiding this comment.
This whole file seems to be an unrelated change
src/util/vector2d.h
Outdated
| @@ -0,0 +1,28 @@ | |||
| // | |||
test/QueryPlannerTest.cpp
Outdated
| @@ -904,6 +904,7 @@ TEST(QueryExecutionTreeTest, testPoliticiansFriendWithScieManHatProj) { | |||
| // "| width: 3} [0] with textLimit = 1 | width: 6} [0]\n) " | |||
| // "| width: 6}", | |||
| // qet.asString()); | |||
| /* | |||
There was a problem hiding this comment.
Replace the commented part, git will remember the old version if we ever need it again
src/engine/QueryExecutionTree.cpp
Outdated
| << _rootOperation->asString(indent + 2) << "\n" | ||
| << indentStr << " qet-width: " << getResultWidth() << " "; | ||
| if (LOGLEVEL >= TRACE && _qec) { | ||
| os << " [estimated size: " << getSizeEstimate() << "]"; |
There was a problem hiding this comment.
After a tree was cached a new call would give new (exact) size estimates so this part would change. So I think with higher LOGLEVEL the optimization doesn't really work? Maybe these estimates really shouldn't be in the cache string. So maybe we should put this part behind a flag parameter that's only used when printing to the console?
There was a problem hiding this comment.
You are right. For now I left it since I would suggest putting the whole cache-key vs nice formatting business in a different PR since I want to do this in a clean way.
There was a problem hiding this comment.
Ok then let's just completely remove this if block here so it isn't subtly broken just by changing the log level. Instead let's add a feature request issue for splitting this up and in that mention that we want to be able to see size estimates.
| @@ -203,7 +208,9 @@ size_t QueryExecutionTree::getCostEstimate() { | |||
| // _____________________________________________________________________________ | |||
| size_t QueryExecutionTree::getSizeEstimate() { | |||
| if (_sizeEstimate == std::numeric_limits<size_t>::max()) { | |||
| if (_qec) { | |||
| if (_cachedResult) { | |||
| _sizeEstimate = _cachedResult->size(); | |||
There was a problem hiding this comment.
I think the size() might be bogus if ResultTable._status != ResultTable::FINISHED
|
I fixed everything except for the one comment with the LOG(Trace) business, see there |
- QueryExecutionTrees try to find their results in the LRU cache after they are initialized. - If the result is found and already finished, it is pinned via a shared pointer - In this case the size estimate is exact and the cost estimate becomes 0 - QueryExecutionTrees use their cached string representation only when the indent matches. This enforces a deterministic cache key
joka921
force-pushed
the
f.cachedOptimizer
branch
from
60e98fb
to
5841cdc
Compare
QueryExecutionTrees try to find their results in the LRU cache after they are initialized.
If the result is found, it is pinned via a shared pointer
In this case the size estimate is exact and the cost estimate becomes 0
We still have to verify that this works indeed by e.g. using Niklas' autocompletion script with similar queries.