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

Implement lazy BIND #1543

Merged
merged 11 commits into from
Oct 15, 2024
Merged

Implement lazy BIND #1543

merged 11 commits into from
Oct 15, 2024

Conversation

RobinTF
Copy link
Collaborator

@RobinTF RobinTF commented Oct 10, 2024

Allow the BIND operation to handle its input lazily.
NOTE: Currently there is only a single local vocab for all the results of the BIND, so even when a BIND that creates strings is handled lazily, we still need the RAM for the complete local vocab. This will be handled in a follow-up PR.

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 98.43750% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.45%. Comparing base (b97c44c) to head (5938519).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/engine/Bind.cpp 98.36% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1543      +/-   ##
==========================================
+ Coverage   88.33%   88.45%   +0.12%     
==========================================
  Files         362      362              
  Lines       27319    27455     +136     
  Branches     3682     3705      +23     
==========================================
+ Hits        24131    24286     +155     
+ Misses       1952     1938      -14     
+ Partials     1236     1231       -5     

☔ 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.

Thanks for the quick implementation, I have only some relatively minor questions and requests.

src/engine/Bind.cpp Outdated Show resolved Hide resolved
src/engine/Bind.cpp Outdated Show resolved Hide resolved
src/engine/Bind.cpp Outdated Show resolved Hide resolved
test/engine/BindTest.cpp Outdated Show resolved Hide resolved
test/engine/BindTest.cpp Outdated Show resolved Hide resolved
test/engine/BindTest.cpp Outdated Show resolved Hide resolved
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.

I am deeply sorry, I had already added some comments last week, but never submitted them.
We can discuss, which of my suggestions are mandatory for merging, and which not.

src/engine/Bind.cpp Outdated Show resolved Hide resolved
src/engine/Bind.cpp Outdated Show resolved Hide resolved
src/engine/Bind.cpp Outdated Show resolved Hide resolved
src/engine/Bind.cpp Outdated Show resolved Hide resolved
src/engine/idTable/IdTable.h Outdated Show resolved Hide resolved
test/IdTableTest.cpp Outdated Show resolved Hide resolved
src/engine/Bind.cpp Outdated Show resolved Hide resolved
src/engine/Bind.cpp Outdated Show resolved Hide resolved
src/engine/Bind.cpp Outdated Show resolved Hide resolved
src/engine/Bind.cpp Outdated Show resolved Hide resolved
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.

Thank you very much!

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.

I have found the proper way to do things...

Comment on lines 150 to 154
LocalVocab* outputLocalVocab,
ad_utility::SimilarTo<IdTable> auto&& inputIdTable,
const LocalVocab& inputLocalVocab,
const sparqlExpression::SparqlExpression* expression,
std::optional<std::pair<size_t, size_t>> subrange) const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
Now that you've learned the whole truth about forwarding references (or more about it), we can do this the proper way:

This function needs its own IdTable, so it just takes IdTable (by value) and no subrange argument.
(And NO, you also don't really need the subrange for the evaluation context, if your input only consists of the relevant rows.

The you just have the idTable which you use for both the evaluation context and for adding a column etc. to it.
The CALLS to this function then do the move / clone / cloneSubrange which is then much safer etc. That way we also don't need the moveOrClone function anymore .

Copy link

sonarcloud bot commented Oct 15, 2024

@joka921 joka921 changed the title Implement lazy Bind Implement lazy BIND Oct 15, 2024
@joka921 joka921 merged commit 81a9350 into ad-freiburg:master Oct 15, 2024
20 checks passed
@RobinTF RobinTF deleted the lazy-bind branch October 15, 2024 16:14
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