Skip to content

Use binary search for index creation dictionary lookup#8924

Merged
siddharthteotia merged 1 commit intoapache:masterfrom
Jackie-Jiang:index_creation_binary_search
Jun 21, 2022
Merged

Use binary search for index creation dictionary lookup#8924
siddharthteotia merged 1 commit intoapache:masterfrom
Jackie-Jiang:index_creation_binary_search

Conversation

@Jackie-Jiang
Copy link
Copy Markdown
Contributor

Follow up on #8846

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 19, 2022

Codecov Report

Merging #8924 (07221f8) into master (7fef95e) will decrease coverage by 41.44%.
The diff coverage is 0.00%.

❗ Current head 07221f8 differs from pull request most recent head 3845ec4. Consider uploading reports for the commit 3845ec4 to get more accurate results

@@              Coverage Diff              @@
##             master    #8924       +/-   ##
=============================================
- Coverage     68.09%   26.65%   -41.45%     
+ Complexity     4711        1     -4710     
=============================================
  Files          1813     1801       -12     
  Lines         94522    94159      -363     
  Branches      14111    14073       -38     
=============================================
- Hits          64363    25096    -39267     
- Misses        25642    66660    +41018     
+ Partials       4517     2403     -2114     
Flag Coverage Δ
integration1 26.65% <0.00%> (?)
integration2 ?
unittests1 ?
unittests2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...segment/creator/impl/SegmentDictionaryCreator.java 0.00% <0.00%> (-90.37%) ⬇️
...in/java/org/apache/pinot/spi/utils/StringUtil.java 0.00% <0.00%> (-100.00%) ⬇️
.../java/org/apache/pinot/spi/utils/BooleanUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/BaseRecording.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/NoOpRecording.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/config/table/FSTType.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/config/user/RoleType.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/data/MetricFieldSpec.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/common/tier/TierFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...a/org/apache/pinot/spi/config/table/TableType.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1368 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 7fef95e...3845ec4. Read the comment docs.

@siddharthteotia siddharthteotia merged commit 2644f48 into apache:master Jun 21, 2022
@Jackie-Jiang Jackie-Jiang deleted the index_creation_binary_search branch June 21, 2022 05:07
@Jackie-Jiang
Copy link
Copy Markdown
Contributor Author

@siddharthteotia Sorry didn't call out in the PR description (more context is in #8846), but was planning to run some perf test using this branch. This change can potentially increase the segment creation time, but per the benchmark I performed locally, the overhead should be quite small. If you find the index creation time is significantly longer, we can revert this one.

@kishoreg
Copy link
Copy Markdown
Member

Why are we doing this? Reduce memory usage at cost of performance?

@Jackie-Jiang
Copy link
Copy Markdown
Contributor Author

@kishoreg This is only during the segment creation. Ideally we want to bring everything to off-heap

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.

4 participants