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
BIND for Constants and variable renaming. #332
Conversation
I just stumbled upon the following bug. The following query crashes any backend. It does not matter what the triple is. What matters is the combination of an aggregation over a non-existing variable and a BIND, though I don't understand why that matters):
The last line in the log before the crash is:
|
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.
It's been a while since I worked with QLever, so I might have missed some things, but overall this looks good to me. Unit tests and a toString
for the bind grapg pattern are still missing though.
f3cba88
to
17b7ff6
Compare
- BIND(?x as ?y) - BIND(<entity> as ?x) (entity must be contained in KB) - BIND("literal" as ?x) literal must be contained in KB) - BIND(42 AS ?x) (only positive integers) - BIND (?x + ?y as ?sum) (sum of exactly two variables). - Technically this includes a new BIND operation, as well as changes in the parser and the Query Planner to be able to use this operation.
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.
Pass in a 1-1 code review of Hannah and Johannes. Reviewed everything in detail, except the (non-trivial) changes in QueryPlanner.cpp
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.
don't know how to cancel reviews...
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.
Also reviewed the missing QueryPlanner.cpp in a 1-1 with Johannes.
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.
Looks great, thank you very much. I guess the small code duplication for the pattern trick check (QueryPlanner.cpp lines 561-568 and lines 624-631) has historical reasons and can be fixed when QueryPlanner::optimize gets a major overhaul.
Implemented BIND( as ?variable) and BIND(?variable as ?otherVariable).