Skip to content
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

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

Flixtastic
Copy link
Contributor

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.

Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 92.37057% with 28 lines in your changes missing coverage. Please review.

Project coverage is 90.18%. Comparing base (8fe0642) to head (c398627).

Files with missing lines Patch % Lines
src/parser/TextSearchQuery.cpp 96.01% 4 Missing and 4 partials ⚠️
src/parser/sparqlParser/SparqlQleverVisitor.cpp 73.91% 5 Missing and 1 partial ⚠️
src/engine/TextIndexScanForWord.cpp 90.56% 3 Missing and 2 partials ⚠️
src/parser/TextSearchQuery.h 58.33% 0 Missing and 5 partials ⚠️
src/engine/CheckUsePatternTrick.cpp 0.00% 2 Missing ⚠️
src/engine/TextIndexScanForEntity.cpp 97.77% 0 Missing and 1 partial ⚠️
src/index/IndexImpl.Text.cpp 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@joka921 joka921 left a 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.

Copy link
Member

@joka921 joka921 left a 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.

Copy link
Member

@joka921 joka921 left a 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()));
Copy link
Member

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;
Copy link
Member

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)

@sparql-conformance
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants