-
Notifications
You must be signed in to change notification settings - Fork 65
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
Magic service query for text search. #1732
base: master
Are you sure you want to change the base?
Magic service query for text search. #1732
Conversation
…so means there are now 2 new Magic Service Queries. This solution works but can be hard to work with in practice. Further refinement necessary for an easy to use MagicServiceQuery that combines both searches.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1732 +/- ##
========================================
Coverage 90.18% 90.18%
========================================
Files 400 402 +2
Lines 38440 38725 +285
Branches 4306 4345 +39
========================================
+ Hits 34666 34925 +259
- Misses 2479 2493 +14
- Partials 1295 1307 +12 ☔ View full report in Codecov by Sentry. |
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.
An initial round of reviews, please read the comments first, some are of a rather structural nature about the general design.
… the respective configs
…ic/qlever into magic-service-query-text-search
…es but in one class.
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.
In principle the code looks now very nice and clean, with only minor issues.
Of course we are still missing all sorts of unit tests.
… of error messages
…roved Error Messages. Added comparison operator for configs which should be removed once unnecessary.
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.
A review on everything but the tests.
If you need readable output in the tests when something fials,
you either need to overload std::ostream& operator<<(std::ostream&, const YourType&)
or implement the PrintTo
function for googletest and make it accessible to the tests.
configVarToConfigs_[subjectVar].isWordSearch_ = false; | ||
if (object.isLiteral()) { | ||
configVarToConfigs_[subjectVar].entity_ = | ||
std::string(asStringViewUnsafe(object.getLiteral().getContent())); |
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.
Seems to be a missing test case.
conf.varToBindScore_}); | ||
} | ||
} | ||
return output; |
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 old ql:contains-word
has special semantics for strings like "alpha beta blubb*"
wich actually splits into three words "alpha" , "beta" and the prefix "blubb*".
I still have to think what we shall do in this case (Maybe emit a warning, that becomes part of the created config (then you can propagate it in the query planner)
…ry helper methods.
Conformance check passed ✅No test result changes. |
|
Add the possibility to do text seach with a magic service query. This leads to two new service queries, one for ql:contains-word and one for ql:contains-entity.