-
Notifications
You must be signed in to change notification settings - Fork 41
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
Correct lexicographic ordering of items from the local vocab #1390
Correct lexicographic ordering of items from the local vocab #1390
Conversation
local Vocab and Vocab. However the sorting between local vocab entries doesn't yet work.
TODO: * Binary search expressions (at least partially). * Fix unit tests * Clean everything up.
Only small things not yet working.
This is still missing comments, and we neeed to discuss and understand the ICU Levels etc.
We really have to fix this...
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, first round. Very tricky and impressive and awesome that this finally works!
TODO<joka921> Add a unit test that would have caught this bug.
…parison-local-vocab # Conflicts: # src/index/PatternCreator.cpp
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1390 +/- ##
==========================================
+ Coverage 89.38% 89.51% +0.13%
==========================================
Files 340 342 +2
Lines 29727 29816 +89
Branches 3309 3310 +1
==========================================
+ Hits 26570 26690 +120
+ Misses 1999 1976 -23
+ Partials 1158 1150 -8 ☔ View full report in Codecov by Sentry. |
Next try: store an IndexPointer in the LocalVocabEntries.
TODO: Index building and all the other TODOs.
…nal or external vocabulary.
# Conflicts: # src/index/PatternCreator.cpp # test/index/PatternCreatorTest.cpp
Feed it to the tools.
…local-vocab # Conflicts: # src/index/IndexImpl.cpp # test/IndexTest.cpp
# Conflicts: # src/index/IndexImpl.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.
Painless second round of 1-1 with Johannes, minor changes left
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.
Tiny change left
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.
Awesome, now we can order strings!
Trying out whether not only gcc 13, but also everyone else (including SonarCloud) will be happy with this.
|
So far, items from the local vocab were handled as if they were lexicographcally larger than all items from both the internal and external vocabulary. This is now fixed, here is an example query: https://qlever.cs.uni-freiburg.de/wikidata/Fkr5Aa . For efficiency reasons, when an entry from the local vocab is compared with an entry from the original vocab, its lower and upper bound in the original vocab are remembered, in order to make future comparisons cheaper. Together with #1405, which fixes the ordering between words in the internal and external vocabularies, QLever can now correctly handle lexicographic ordering of string literals or IRIs in all cases. This concerns operations like
ORDER BY
andFILTER
wth string comparisons or prefix searches.