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

Feature/db read entry - read entry/s using simple union sql, instead of the complex views/sqEntriesLineage #783

Merged
merged 21 commits into from
Oct 15, 2020

Conversation

tzahij
Copy link
Contributor

@tzahij tzahij commented Oct 7, 2020

not to do yet

@codecov-io
Copy link

codecov-io commented Oct 8, 2020

Codecov Report

Merging #783 into master will increase coverage by 1.38%.
The diff coverage is 86.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #783      +/-   ##
==========================================
+ Coverage   41.26%   42.65%   +1.38%     
==========================================
  Files         131      135       +4     
  Lines       10146    10607     +461     
==========================================
+ Hits         4187     4524     +337     
- Misses       5403     5495      +92     
- Partials      556      588      +32     
Impacted Files Coverage Δ
catalog/db_batch_entry_read.go 76.05% <33.33%> (-0.98%) ⬇️
catalog/db_read_entry.go 89.47% <89.47%> (ø)
catalog/cataloger_diff.go 57.22% <100.00%> (ø)
catalog/views.go 98.65% <100.00%> (-1.35%) ⬇️
onboard/import.go 62.74% <0.00%> (-2.57%) ⬇️
onboard/catalog_actions.go 67.85% <0.00%> (-2.28%) ⬇️
catalog/db_branch_reader.go 85.00% <0.00%> (-1.05%) ⬇️
block/local/adapter.go 5.71% <0.00%> (-0.74%) ⬇️
block/adapter.go 0.00% <0.00%> (ø)
... and 22 more

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 b1751a8...4cf268a. Read the comment docs.

@tzahij tzahij requested a review from nopcoder October 8, 2020 20:17
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

Update the PR title to have a meaningful description.
It will help me to add a short description of what you fixed or where in the code you address the issue.
Added Go related comments, but from the change part - it looks we have another version of 'sqEntriesLineage' just to fix the specific problem?

catalog/db_batch_entry_read.go Outdated Show resolved Hide resolved
catalog/db_batch_entry_read.go Outdated Show resolved Hide resolved
catalog/db_batch_entry_read.go Outdated Show resolved Hide resolved
catalog/db_read_entry.go Outdated Show resolved Hide resolved
catalog/db_read_entry.go Outdated Show resolved Hide resolved
Comment on lines 44 to 49
l := len(paths)
if l == 1 {
rawSelect = rawSelect.Where("path = ?", paths[0])
} else {
rawSelect = rawSelect.Where(sq.Eq{"path": paths})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
l := len(paths)
if l == 1 {
rawSelect = rawSelect.Where("path = ?", paths[0])
} else {
rawSelect = rawSelect.Where(sq.Eq{"path": paths})
}
rawSelect = rawSelect.Where(sq.Eq{"path": paths})

Let SQL optimize the IN with one element to =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have checked this - for some reason IN with one argument consumes more CPU. It may even be because of the driver handling of array

catalog/db_read_entry.go Show resolved Hide resolved
catalog/db_read_entry.go Outdated Show resolved Hide resolved
catalog/cataloger_diff.go Outdated Show resolved Hide resolved
catalog/cataloger_merge_test.go Outdated Show resolved Hide resolved
tzahij and others added 8 commits October 9, 2020 13:28
Co-authored-by: Barak Amar <barak.amar@treeverse.io>
Co-authored-by: Barak Amar <barak.amar@treeverse.io>
Co-authored-by: Barak Amar <barak.amar@treeverse.io>
Co-authored-by: Barak Amar <barak.amar@treeverse.io>
@tzahij tzahij changed the title Feature/db read entry Feature/db read entry - read entry/s using simple union sql, instead of the complex views/sqEntriesLineage Oct 15, 2020
@itaiad200 itaiad200 added this to Review in progress in lakeFS Oct 15, 2020
@nopcoder nopcoder merged commit 89b8b7d into master Oct 15, 2020
lakeFS automation moved this from Review in progress to Done Oct 15, 2020
@tzahij tzahij deleted the feature/db_read_entry branch November 8, 2020 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
lakeFS
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants