-
Notifications
You must be signed in to change notification settings - Fork 57
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
Allow unbound variables in compliance with the SPARQL standard #1586
Allow unbound variables in compliance with the SPARQL standard #1586
Conversation
TODO: Still needs warnings, cleanups, and tests. Signed-off-by: Johannes Kalmbach <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1586 +/- ##
==========================================
+ Coverage 89.24% 89.31% +0.06%
==========================================
Files 374 374
Lines 35315 35376 +61
Branches 3988 3997 +9
==========================================
+ Hits 31518 31595 +77
+ Misses 2503 2491 -12
+ Partials 1294 1290 -4 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Johannes Kalmbach <[email protected]>
Signed-off-by: Johannes Kalmbach <[email protected]>
Would it be possible to keep the current behavior behind some flag? I find it extremely useful that qlever rejects rubbish queries. It saved me a lot of time already. So it would be a pity to loos this functionality. |
@aindlq Thanks for the input and the (in my opinion very reasonable) request. |
For example for queries like in the linked issue, where group by uses non bound variable. It doesn't make any sense to return results, because clearly the query is wrong. I understand the use case of being a bit more relaxed and don't fail hard on broken queries, but most of the time when developing applications with SPARQL I want broken queries to fail hard, so I can actually fix them. Sorry for the rant, but I believe that at this point this feature saved me countless hours of debugging. Because if query fails, I check qlever logs and immediately see what is the problem. |
Signed-off-by: Johannes Kalmbach <[email protected]>
# Conflicts: # src/global/RuntimeParameters.h
Signed-off-by: Johannes Kalmbach <[email protected]>
Signed-off-by: Johannes Kalmbach <[email protected]>
…ns' into undefined-variables-in-expressions # Conflicts: # test/QueryPlannerTestHelpers.h
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.
1-1 with Johannes, this looks great, adding some minor changes of mine in a subsequent commit
Signed-off-by: Johannes Kalmbach <[email protected]>
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 now, I will add a description + once everything is green merge this!
Signed-off-by: Johannes Kalmbach <[email protected]>
Signed-off-by: Johannes Kalmbach <[email protected]>
Signed-off-by: Johannes Kalmbach <[email protected]>
Conformance check passed ✅Test Status Changes 📊
|
Quality Gate passedIssues Measures |
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.
Thanks a lot for the final fix + I tested it end-to-end and it works fine + will merge this now!
@joka921 Thank you for the |
So far, QLever reported an error when an unbound variable was used in a
GROUP BY
,ORDER BY
, or at other places, where an unbound variable does not really make sense. However, the SPARQL standard allows this. For example,SELECT * WHERE { ?s ?p ?o } GROUP BY ?doof
is allowed by the standard and the correct result is a table with a single column?doof
but no results.With this change, QLever complies to the standard regarding unbound variables at all kinds of places. Since queries like the above are often due to a typo in the query, we add a warning to the
application/sparql-results+qlever
results, which, in particular, is displayed in the QLever UI.Additionally, there is a new runtime-parameter and associated command-line argument
--throw-on-unbound-variables
which reinstates the old behavior and reports an error instead of just a warning.Fixes #1575