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

Remove unused sort for segmenta meta list #1218

Merged
merged 5 commits into from
Dec 1, 2021

Conversation

k-yomo
Copy link
Contributor

@k-yomo k-yomo commented Nov 26, 2021

https://github.com/quickwit-inc/tantivy/blob/02174d26af50af2cf8b9f7592c35e90aae14de70/src/indexer/segment_updater.rs#L415-L423

The segment_metas method is only used in the above method, but it's re-sorted by doc count in the method as following. So the original sort by segment id can be removed to make it little more efficient.
https://github.com/quickwit-inc/tantivy/blob/02174d26af50af2cf8b9f7592c35e90aae14de70/src/indexer/segment_updater.rs#L438

@k-yomo k-yomo force-pushed the remove-unused-sort branch from d3ccfff to f54d88a Compare November 26, 2021 19:05
@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2021

Codecov Report

Merging #1218 (6f4186c) into main (02174d2) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1218      +/-   ##
==========================================
+ Coverage   94.05%   94.09%   +0.03%     
==========================================
  Files         205      205              
  Lines       34514    34517       +3     
==========================================
+ Hits        32463    32478      +15     
+ Misses       2051     2039      -12     
Impacted Files Coverage Δ
src/indexer/segment_register.rs 84.84% <100.00%> (-0.45%) ⬇️
src/postings/mod.rs 98.57% <100.00%> (+0.22%) ⬆️
src/core/segment_id.rs 64.00% <0.00%> (-12.00%) ⬇️
src/fastfield/reader.rs 94.06% <0.00%> (-0.85%) ⬇️
src/schema/schema.rs 98.40% <0.00%> (-0.19%) ⬇️
src/functional_test.rs 0.00% <0.00%> (ø)
src/positions/reader.rs 100.00% <0.00%> (ø)
src/tokenizer/ascii_folding_filter.rs 99.92% <0.00%> (+0.07%) ⬆️
query-grammar/src/query_grammar.rs 99.64% <0.00%> (+0.35%) ⬆️
... and 12 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 02174d2...6f4186c. Read the comment docs.

src/postings/mod.rs Outdated Show resolved Hide resolved
src/postings/mod.rs Outdated Show resolved Hide resolved
src/postings/mod.rs Outdated Show resolved Hide resolved
src/postings/mod.rs Outdated Show resolved Hide resolved
src/postings/mod.rs Outdated Show resolved Hide resolved
src/postings/mod.rs Outdated Show resolved Hide resolved
@fulmicoton
Copy link
Collaborator

fulmicoton commented Dec 1, 2021

LGTM Thank you @k-yomo. The explanation were very helpful too. I am waiting for the CI and will merge after.

@fulmicoton fulmicoton enabled auto-merge (squash) December 1, 2021 01:48
auto-merge was automatically disabled December 1, 2021 01:58

Head branch was pushed to by a user without write access

@k-yomo k-yomo requested a review from fulmicoton December 1, 2021 01:59
@fulmicoton fulmicoton merged commit bd0f921 into quickwit-oss:main Dec 1, 2021
@k-yomo k-yomo deleted the remove-unused-sort branch December 1, 2021 02:31
fulmicoton pushed a commit to lpouget/tantivy that referenced this pull request Dec 10, 2021
* Remove unused sort for segment meta list
* Fix segment meta order dependent test
This was referenced Feb 18, 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.

3 participants