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
Refactoring the ParsedQuery
class
#323
Refactoring the ParsedQuery
class
#323
Conversation
…childGraphPattern types - Todo : remove outcommented code - Todo: reinstate the "toString" class.
Start simplifying the parsedQuery by removing the unnecessary shared_ptr and manual constructor shenanigans.
68b5b84
to
552eb6d
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 and I really like how clean this is now!
} else if constexpr (std::is_same_v<T, | ||
GraphPatternOperation::TransPath>) { | ||
AD_CHECK(false); | ||
// we may never be in an transitive path here or a |
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.
The property paths are not yet expanded into trees of operations, thats done in the QueryPlanner. Due to that they are currently all contained in a triple, and the TransPath GraphPatternOperation cannot appear here. Instead, the prefix expansion is done as part of the prefix expansion of triples, a few lines below this one.
The suboperations previously contained a union for the different types (Subquery, Optional, Union + some we have yet to implement) plus a separate tag, which type was in use. This is now refactored as a
std::variant
which is more modern and more explicit, since the "data storage" and the name of the Suboperation type are much closer connected.The
ParsedQuery
class previously usedstd::shared_ptr
all over the place when there was neitherParsedQuery
is built exactly once during the Parsingand is typically never copied and relatively cheap to copy and basically free to move.
Even worse, typically the copy constructors of types were manually specified to ignore the shared_ownership and copy the held data which really defeats the purpose and signals, that is was the wrong tool here.
These places are now all refactored to hold the data directly + The use of typically mechanisms to pass them to functions (
const T&
for const access, T* for readWrite access).since many calls to
shared_ptr<T>.get()
and tomake_shared
can be removed.