-
Notifications
You must be signed in to change notification settings - Fork 54
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
Provide framework for generic lazily evaluated operation results #1350
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1350 +/- ##
==========================================
+ Coverage 89.37% 89.45% +0.07%
==========================================
Files 345 346 +1
Lines 24886 25262 +376
Branches 3309 3371 +62
==========================================
+ Hits 22242 22597 +355
- Misses 1503 1505 +2
- Partials 1141 1160 +19 ☔ View full report in Codecov by Sentry. |
This PR contains all the changes from the infrastructure for lazy operation evaluation (#1350) that are simple and repetitive, but touch many files. In particular: * Rename the `ResultTable` class to `Result` (a TODO suggested by @hannahbast some time ago). * Add a new parameter `bool requestLaziness` to `Operation::computeResult`. This parameter is currently unused.
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 round of reviews on everything but the tests (those I will review later).
const IdTable& idTable_; | ||
std::ranges::iota_view<uint64_t, uint64_t> view_; |
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.
Do you need the original absolute indices into the IdTable?
Otherwise you could return a single
std::ranges::subrange<const IdTable&>
and all your loops become singular again, and all the sonarcloud stuff goes away?
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.
I need the values as well as a reference to the original IdTable
to fill in the ConstructQueryExportContext
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.
It would be possible however, to refactor ConstructQueryExportContext
to get some row reference instead, but this might be better suited for a follow-up PR.
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.
Thinking about it it doesn't solve the original issue. yielding subranges still requires 2 loops
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.
Please please please fix the large amount of copy and paste code in your tests.
I have made several suggestions for this endeavour.
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.
This is already much simpler,
but I have identified some additional simplifications.
EXPECT_EQ(iterator, generator.end()); | ||
auto referenceTable = makeIdTableFromVector({{2}}); | ||
EXPECT_THAT(convertToVector(std::move(generator)), | ||
ElementsAre(Eq(std::cref(referenceTable)))); |
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 Eq
is redundant/implicit`.
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.
From https://google.github.io/googletest/reference/matchers.html
WARNING: Equality matching via EXPECT_THAT(actual_value, expected_value) is supported, however note that implicit conversions can cause surprising results. For example, EXPECT_THAT(some_bool, "some string") will compile and may pass unintentionally.
Also the Eq
seems to be required for the cref stuff to work otherwise it tries to copy the IdTable
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.
so you say the cref(...)
without Eq()
doesn't also work?
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.
Exactly. No idea why but I just tried it to confirm
…t-table
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.
This looks really nice and is now running on our public endpoint.
I will run some tests tomorrow (but it already looks very nice to look at the incremental runtime information).
Quality Gate passedIssues Measures |
QLever now supports lazy operations, that can produce their result in batches instead of fully materializing them. This PR implements the general infrastructure needed for lazy evaluation, and provides a lazy implementation of the
IndexScan
andFilter
operation. This means that queries that consist of a single triple and one or more subsequent FILTER clauses will not materialize the full index scan.