Refactor the writing and reading of the fulltext index #1699

Flixtastic · 2025-01-04T22:40:16Z

Move the code for reading and writing the data structures of the text index to a separate class, making the large IndexImpl class significantly smaller.
While doing so, also completely refactor and modernize this code.

…yet, commit is used to initialize branch

…quency and gap compressed lists.

codecov · 2025-01-04T23:25:16Z

Codecov Report

Attention: Patch coverage is 93.42723% with 14 lines in your changes missing coverage. Please review.

Project coverage is 89.95%. Comparing base (d060444) to head (149ef23).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/index/TextIndexReadWrite.cpp	91.57%	4 Missing and 4 partials ⚠️
src/index/TextIndexReadWrite.h	91.66%	2 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1699      +/-   ##
==========================================
- Coverage   89.95%   89.95%   -0.01%     
==========================================
  Files         393      395       +2     
  Lines       37639    37625      -14     
  Branches     4233     4229       -4     
==========================================
- Hits        33860    33847      -13     
- Misses       2480     2482       +2     
+ Partials     1299     1296       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joka921

The most important thing is:
Please tell me in which places you have changed something, and where you only extracted, s.t. we can properly review the nontrivial changes.
I really like this idea, everything that makes the index class smaller is good.

joka921 · 2025-01-08T19:48:06Z

src/index/IndexImpl.Text.cpp

+  std::ranges::copy(TextIndexReadWrite::readFreqComprList<Id, Score>(
+                        tbmd._cl._nofElements, tbmd._cl._startScorelist,
+                        static_cast<size_t>(tbmd._cl._lastByte + 1 -
+                                            tbmd._cl._startScorelist),
+                        textIndexFile_, &Id::makeFromInt),
+                    idTable.getColumn(2).begin());


What do you think of the following (index-breaking) suggestion, which makes this code possibly simpler
(maybe we can postpone it to another PR, if this stalls your work here):

We consistently directly compress and store the bits of the ID (as they are also consecutive for positive integers, the gap encoding and frequency encoding should still work). This gets rid of all the Id::makeFromBlaIndex(BlaIndex::make(...)) calls in the transform and copy calls.

Again: After some thought please remember this idea, but probably this is for future changes, as it is rather intrusive.

This would theoretically work fine but there is one slight Problem. This problem has do to with simple8b encoding after gap encoding. If we try to gap encode IDs the first element will be the starting ID without any encoding. Because IDs use their first few bits to determine what type of ID they are there will be a one in the first 4 bits of the ID. This then becomes a problem in simple8b encoding, since it only works for uint64_t with the first 4 bits being 0.

src/index/TextIndexReadWrite.cpp

joka921 · 2025-01-08T20:00:15Z

src/index/TextIndexReadWrite.h

+  explicit GapEncode(const TypedVector& vectorToEncode);
+
+  void writeToFile(ad_utility::File& out, size_t nofElements,
+                   off_t& currentOffset);


I think you can get away with a lazy view as the input to the constructor.

Would the code in IndexImpl.cpp become simpler if you make a static function that does the encoding + writing in one step (same for the other encoders).

(Also maybe part for a separate PR, your work here is valuable, by moving it to a separate file it now has a size where we can see the possible improvements much simpler.

src/index/TextIndexReadWrite.h

…eview and should be removed later

…se namespace)

…th using vector.data()

Flixtastic · 2025-01-15T21:29:08Z

The 2 SonarQube issues can't be adressed with the current implementation of ranges if I am correct.

joka921

Small reviews from the diff, I'll have another pass over everything with your // MODIFIED comments.

src/index/TextIndexReadWrite.cpp

src/index/TextIndexReadWrite.h

joka921

Some additional small comments.

src/index/TextIndexReadWrite.h

joka921 · 2025-01-16T11:40:04Z

src/index/TextIndexReadWrite.h

+  size_t nofCodebookBytes;
+  vector<uint64_t> frequencyEncodedResult;
+  frequencyEncodedResult.resize(nofElements + 250);
+  off_t current = from;
+  size_t ret = textIndexFile.read(&nofCodebookBytes, sizeof(size_t), current);
+  LOG(TRACE) << "Nof Codebook Bytes: " << nofCodebookBytes << '\n';
+  AD_CONTRACT_CHECK(sizeof(size_t) == ret);
+  current += ret;
+  std::vector<From> codebook;
+  codebook.resize(nofCodebookBytes / sizeof(From));
+  ret = textIndexFile.read(codebook.data(), nofCodebookBytes, current);
+  current += ret;
+  AD_CONTRACT_CHECK(ret == size_t(nofCodebookBytes));
+  std::vector<uint64_t> simple8bEncoded;
+  simple8bEncoded.resize(nofElements);
+  ret = textIndexFile.read(simple8bEncoded.data(), nofBytes - (current - from),
+                           current);
+  current += ret;
+  AD_CONTRACT_CHECK(size_t(current - from) == nofBytes);
+  LOG(DEBUG) << "Decoding Simple8b code...\n";
+  ad_utility::Simple8bCode::decode(simple8bEncoded.data(), nofElements,
+                                   frequencyEncodedResult.data());
+  LOG(DEBUG) << "Reverting frequency encoded items to actual IDs...\n";
+  frequencyEncodedResult.resize(nofElements);
+  vector<To> result;
+  result.reserve(frequencyEncodedResult.size());
+  ql::ranges::for_each(frequencyEncodedResult, [&](const auto& encoded) {
+    result.push_back(transformer(codebook.at(encoded)));
+  });
+  LOG(DEBUG) << "Done reading frequency-encoded list. Size: " << result.size()


In one of your next PRs you can refactor the reading and writing here, using the functionalities in
util/serialization , because all those manual read and offset += ret calls are very hard to read.

joka921 · 2025-01-16T11:44:46Z

src/index/TextIndexReadWrite.h

+  From previous = 0;
+  for (size_t i = 0; i < gapEncodedVector.size(); ++i) {
+    previous += gapEncodedVector[i];
+    result.push_back(transformer(previous));
+  }
+  LOG(DEBUG) << "Done reading gap-encoded list. Size: " << result.size()


Does it (same for the frequency encoding)
happen, that To and From are the same type, (and the transformation therefore does nothing).
In this case you could simply return the read vector, without the (then redundant) copy.

With the current implementation that doesn't happen since the type To is always Id when reading (the only reading happens in IndexImpl.Text.cpp) and the type From differs.

src/index/TextIndexReadWrite.cpp

joka921 · 2025-01-16T11:56:53Z

src/index/TextIndexReadWrite.cpp

+template <typename T>
+void writeVectorAndMoveOffset(const std::vector<T>& vectorToWrite,
+                              size_t nofElements, ad_utility::File& file,
+                              off_t& currentOffset) {
+  size_t bytes =
+      textIndexReadWrite::writeList(vectorToWrite, nofElements, file);
+  currentOffset += bytes;
+}
+


In a separate PR, you can refactor this to use our serialization library, which makes the reading and writing of such types to and from disk much more readable.

…g that can be represented as view. Deleted nofElements as parameter of some functions where it wasn't necessary. Applied other small changes requested in PR

…oded list" This reverts commit 464a6fb.

… loop

Flixtastic · 2025-01-21T11:47:50Z

I tried several things to fix the error but I can't get it to work. I don't quite understand how to get a funciton to accept all types of ranges that and views to work with them.

# Conflicts: # src/index/IndexImpl.Text.cpp

Signed-off-by: Johannes Kalmbach <[email protected]>

joka921

Thank you very much!

I merged in the current master (easy) and simply removed the viewable_range concept (see my comment there).

As this is getting much more cleaned up, I have found some smaller cleanups to be performed in existing code, most of them should be easy to perform, otherwise this is almost ready to merge.

src/index/IndexImpl.Text.cpp

src/index/TextIndexReadWrite.cpp

src/index/TextIndexReadWrite.h

…unnecessary construction of spans. Changed manual iterating to compute adjanced differences to using std::adjacent_difference.

…can accept a pointer to write to. Removed unnecessary function writeList and moved it into writeVectorAndMoveOffset which was renamed to encodeAndWriteSpanAndMoveOffset. Removed the communication comments and added correct comments.

joka921

Some final suggestions to make Sonarcloud happy.

src/index/TextIndexReadWrite.h

joka921 · 2025-01-24T13:31:08Z

src/index/TextIndexReadWrite.h

@@ -158,9 +275,15 @@ class FrequencyEncode {
  template <typename View>
  explicit FrequencyEncode(View&& view);


Sonarcloud explains its trouble with this constructor pretty closely, and also suggests a solution:

you have to constrain it via a concept or something similar (see the how tab in the sonarcloud explanation on how to best do this). That is the only way to silence that message (which points out a serious potential issue).

I addressed the probelm I got from trying to fix this in the respective email.

src/index/TextIndexReadWrite.h

…cyEncode. Added required to check that View is not of GapEncode or FreqEncode. Added std::forward.

joka921

This should now really be the last round, I have given you all the information you should need.

joka921 · 2025-01-26T10:45:05Z

src/index/TextIndexReadWrite.h

+  requires(!std::same_as<FrequencyEncode, std::remove_cvref_t<View>>)
+  explicit FrequencyEncode(View&& inputView) {


Please move this to the .cpp-File again.

C++20 requires-clauses must be repeatet in the definition, so
in the .cpp-file you woul also have to write

template <typename View> requires(!std:...) FrequencyEncode(View&&...) {...

2 We are currently backporting QLever to C++ 17, so we use macros that use concepts only in C++17 mode. Please use the CPP_template and CPP_template_def macros, see here for details.
So in the header, you write
CPP_template(typename View)(requires !std::same_as...) exlicit...
And in the .cpp file
CPP_template_def(typename View> (requires ...) FrequencyEncode...

(same for the GapEncode class.

joka921 · 2025-01-26T10:46:05Z

src/index/TextIndexReadWrite.h

-  explicit FrequencyEncode(View&& view);
+  requires(!std::same_as<FrequencyEncode, std::remove_cvref_t<View>>)
+  explicit FrequencyEncode(View&& inputView) {
+    auto view = std::forward<View>(inputView);


That is absolutely a bad idea, it introduces unneeded copies. Please remove this, thecdecov warnings about no forwarding are fine for now.

joka921 · 2025-01-26T10:46:41Z

src/index/TextIndexReadWrite.h

+  requires(!std::same_as<GapEncode, std::remove_cvref_t<View>>)
+  explicit GapEncode(View&& inputView) {


Same comment as above

joka921 · 2025-01-26T10:46:51Z

src/index/TextIndexReadWrite.h

-  explicit GapEncode(View&& view);
+  requires(!std::same_as<GapEncode, std::remove_cvref_t<View>>)
+  explicit GapEncode(View&& inputView) {
+    auto view = std::forward<View>(inputView);


Absolutely same comment as abve.

… that used std::forward

Signed-off-by: Johannes Kalmbach <[email protected]>

This reverts commit 255349e.

… to the cpp file. Forwarded the constructor arguments to that method. Explained the reason in the comment above

sparql-conformance · 2025-01-28T17:47:03Z

Conformance check passed ✅

No test result changes.

Details: https://qlever.cs.uni-freiburg.de/sparql-conformance-ui?cur=149ef2350fb468998d1804e293a1c23a60ba8262&prev=d06044449a00f97837b1fac96723ca1879d45e2b

sonarqubecloud · 2025-01-28T19:02:31Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Flixtastic and others added 3 commits January 3, 2025 00:52

First try at outsourcing the writing of the text index. Doesn't work …

912ebce

…yet, commit is used to initialize branch

Solved issues with compression and also outsourced the reading of fre…

2d6f3ad

…quency and gap compressed lists.

Merge branch 'ad-freiburg:master' into text-index-compression

0b2b516

Flixtastic and others added 6 commits January 6, 2025 13:14

Merge branch 'ad-freiburg:master' into text-index-compression

ff19077

Renaming of TextIndexWriteRead to TextIndexReadWrite

75cf7d1

Improved the readFreqComprList to now be more logical and usable

5b29ca5

Clean up code

57d0ef7

Adjusted the readGapComprList to follow the usage of readFreqComprList

66b60b6

Merge branch 'ad-freiburg:master' into text-index-compression

e07488e

joka921 reviewed Jan 8, 2025

View reviewed changes

Flixtastic and others added 3 commits January 9, 2025 12:44

Merge branch 'ad-freiburg:master' into text-index-compression

fdb6932

Added comments to TextIndexReadWrite.h. Those comments are only for r…

09d4a97

…eview and should be removed later

Implemented the small changes requested. (Using ql::views and lowerca…

777329a

…se namespace)

Flixtastic requested a review from joka921 January 9, 2025 19:38

Addressed SonarQube issues and removed the usage of new and delete wi…

a64e848

…th using vector.data()

joka921 reviewed Jan 16, 2025

View reviewed changes

src/index/TextIndexReadWrite.cpp Outdated Show resolved Hide resolved

src/index/TextIndexReadWrite.cpp Outdated Show resolved Hide resolved

src/index/TextIndexReadWrite.h Outdated Show resolved Hide resolved

joka921 requested changes Jan 16, 2025

View reviewed changes

Flixtastic added 4 commits January 19, 2025 18:58

The FrequencyEncode and GapEncode now can be constructed with anythin…

b93312a

…g that can be represented as view. Deleted nofElements as parameter of some functions where it wasn't necessary. Applied other small changes requested in PR

Small change to directly transfrom to Id when reading gap encoded list

464a6fb

Revert "Small change to directly transfrom to Id when reading gap enc…

ad17e8c

…oded list" This reverts commit 464a6fb.

Corrected a mistake and changed a manual for loop to an for (auto : )…

9208666

… loop

Flixtastic requested a review from joka921 January 21, 2025 11:47

joka921 added 2 commits January 22, 2025 09:05

Merge branch 'master' into text-index-compression

aacb2ab

# Conflicts: # src/index/IndexImpl.Text.cpp

Fix the compilation and merged in the master.

dbbea9d

Signed-off-by: Johannes Kalmbach <[email protected]>

joka921 requested changes Jan 22, 2025

View reviewed changes

Flixtastic and others added 2 commits January 22, 2025 23:05

Merge branch 'ad-freiburg:master' into text-index-compression

5ab7951

Removed unnecessary construction of empty values in hashmap. Removed …

3d49687

…unnecessary construction of spans. Changed manual iterating to compute adjanced differences to using std::adjacent_difference.

Flixtastic added 3 commits January 23, 2025 19:55

Addressed small Sonar issue

aa33ea8

Deleted possibly erroneous constructors of GapEncode and FrequencyEncode

830f83e

Flixtastic requested a review from joka921 January 24, 2025 02:36

joka921 requested changes Jan 24, 2025

View reviewed changes

Flixtastic added 2 commits January 24, 2025 19:26

Merge branch 'ad-freiburg:master' into text-index-compression

9100e3a

Fixed empty namespace. Added deduction guide to GapEncode and Frequen…

ea5175e

…cyEncode. Added required to check that View is not of GapEncode or FreqEncode. Added std::forward.

joka921 approved these changes Jan 26, 2025

View reviewed changes

Flixtastic and others added 5 commits January 26, 2025 13:38

Added CPP_template to ensure backport compatibility. Reverted changes…

a0c31f7

… that used std::forward

Debug the Docker Re2 build on ARM

255349e

Signed-off-by: Johannes Kalmbach <[email protected]>

Revert "Debug the Docker Re2 build on ARM"

99d3867

This reverts commit 255349e.

Merge branch 'ad-freiburg:master' into text-index-compression

fe35886

Moved the implementation of the constructor in a different method and…

149ef23

… to the cpp file. Forwarded the constructor arguments to that method. Explained the reason in the comment above

joka921 approved these changes Jan 30, 2025

View reviewed changes

joka921 changed the title ~~Outsourcing and improving the posting writing of IndexImpl.Text.cpp~~ Refactor the writing and reading of the fulltext index Jan 30, 2025

joka921 merged commit 49934c0 into ad-freiburg:master Jan 30, 2025
24 checks passed

Flixtastic deleted the text-index-compression branch February 2, 2025 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the writing and reading of the fulltext index #1699

Refactor the writing and reading of the fulltext index #1699

Flixtastic commented Jan 4, 2025 •

edited by joka921

Loading

codecov bot commented Jan 4, 2025 •

edited

Loading

joka921 left a comment

joka921 Jan 8, 2025

joka921 Jan 8, 2025

Flixtastic Jan 9, 2025

joka921 Jan 8, 2025

joka921 Jan 8, 2025

Flixtastic commented Jan 15, 2025

joka921 left a comment

joka921 left a comment

joka921 Jan 16, 2025

joka921 Jan 16, 2025

Flixtastic Jan 19, 2025

joka921 Jan 16, 2025

Flixtastic commented Jan 21, 2025

joka921 left a comment

joka921 left a comment

joka921 Jan 24, 2025

Flixtastic Jan 24, 2025

joka921 left a comment

joka921 Jan 26, 2025

joka921 Jan 26, 2025

joka921 Jan 26, 2025

joka921 Jan 26, 2025

sparql-conformance bot commented Jan 28, 2025

sonarqubecloud bot commented Jan 28, 2025

		@@ -158,9 +275,15 @@ class FrequencyEncode {
		template <typename View>
		explicit FrequencyEncode(View&& view);

		requires(!std::same_as<FrequencyEncode, std::remove_cvref_t<View>>)
		explicit FrequencyEncode(View&& inputView) {

		requires(!std::same_as<GapEncode, std::remove_cvref_t<View>>)
		explicit GapEncode(View&& inputView) {

Refactor the writing and reading of the fulltext index #1699

Refactor the writing and reading of the fulltext index #1699

Conversation

Flixtastic commented Jan 4, 2025 • edited by joka921 Loading

codecov bot commented Jan 4, 2025 • edited Loading

Codecov Report

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flixtastic commented Jan 15, 2025

joka921 left a comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flixtastic commented Jan 21, 2025

joka921 left a comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparql-conformance bot commented Jan 28, 2025

Conformance check passed ✅

sonarqubecloud bot commented Jan 28, 2025

Quality Gate passed

Flixtastic commented Jan 4, 2025 •

edited by joka921

Loading

codecov bot commented Jan 4, 2025 •

edited

Loading