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

Split big preloading-IN query into multiple queries (#283) #285

Merged
merged 5 commits into from May 27, 2022

Conversation

aligator
Copy link
Contributor

@aligator aligator commented May 24, 2022

This is an initial version to fix #283.

Feel free to review it :-)

@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #285 (c668072) into master (d4f89cf) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #285   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           33        33           
  Lines         2768      2778   +10     
=========================================
+ Hits          2768      2778   +10     
Impacted Files Coverage Δ
repository.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4f89cf...c668072. Read the comment docs.

@aligator aligator marked this pull request as draft May 24, 2022 16:51
@aligator aligator marked this pull request as ready for review May 24, 2022 16:54
@Fs02
Copy link
Member

Fs02 commented May 24, 2022

hi, can you fix the test coverage? thanks

@aligator
Copy link
Contributor Author

Yes of course, I will do that. 👍
I first wanted to know if it goes into the right direction.

Copy link
Member

@Fs02 Fs02 left a comment

Choose a reason for hiding this comment

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

lgtm in terms of the logic

cursor.go Outdated Show resolved Hide resolved
repository.go Outdated Show resolved Hide resolved
repository.go Outdated Show resolved Hide resolved
repository.go Outdated Show resolved Hide resolved
repository.go Outdated Show resolved Hide resolved

return scanMulti(cur, keyField, keyType, targets)
scanFinish := r.instrumenter.Observe(cw.ctx, "rel-scan-multi", "scanning all records to multiple targets")
err = scanMulti(cur, keyField, keyType, targets)
Copy link
Member

Choose a reason for hiding this comment

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

can you confirm calling this multiple time won't reset the slice of association field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's something I am also not sure. In my first test with my example project it works just well, but I will look into it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this can be added as assertion in the unit test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, as long as the keys do not re-occur in the subsequent calls it works just fine, as each scanMulti call only modifies the occurred keys.
This works as long as each call uses unique keys which is here the case as r.targetIDs returns unique keys.

So it should be fine here. Tell me if you think otherwise.

I added a test to scanMulti for this.

@aligator aligator requested a review from Fs02 May 27, 2022 14:12
Copy link
Member

@Fs02 Fs02 left a comment

Choose a reason for hiding this comment

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

LGTM

@Fs02 Fs02 merged commit ae7d739 into go-rel:master May 27, 2022
@aligator aligator deleted the #283-preloading-in branch May 28, 2022 07:18
@Fs02 Fs02 changed the title Split preloading-IN query into multiple queries (#283) Split big preloading-IN query into multiple queries (#283) May 29, 2022
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.

(panic) Preloading does not work with many values due to using IN
2 participants