Skip to content

Conversation

@gortiz
Copy link
Contributor

@gortiz gortiz commented Jan 27, 2023

This is a partial draft PR related to #10183. The whole PR is in #10184. It isn't intended to be merged as it is and it doesn't require to compile or pass the checks. It has been created in order to make it easier to understand the PR with all the changes.

This PR depends on #10193. Readers only interested in the difference between this and the previous PR can focus on gortiz#3

In this PR, ColumnIndexType has been deleted and its usages replaces by using either StandardIndexes or IndexService. Like previous PRs, this almost doesn't contain important code changes. The most significant change is on how the file extension used by V1 and V2 segments is calculated. It was previously implemented with a switch. Obviously we cannot use a switch with classes in Java, so usually we would need to change it with a bunch of if-then-else (like this PR does with IndexHandlerFactory) but if that was the case, given that the number of indexes is not defined at compile time we need another solution and the easiest way to do that is to move the responsibility of how to calculate the extension into the IndexType itself.

@gortiz gortiz marked this pull request as draft January 27, 2023 14:56
@gortiz gortiz changed the title Index spi: remove columnindextype [draft] Index spi: remove columnindextype Jan 27, 2023
gortiz added 22 commits March 15, 2023 14:54
@codecov-commenter
Copy link

Codecov Report

Merging #10194 (7d38a03) into master (dca5d38) will decrease coverage by 45.94%.
The diff coverage is 0.58%.

@@              Coverage Diff              @@
##             master   #10194       +/-   ##
=============================================
- Coverage     70.26%   24.32%   -45.94%     
+ Complexity     6130       58     -6072     
=============================================
  Files          2056     2054        -2     
  Lines        111534   110930      -604     
  Branches      16962    16884       -78     
=============================================
- Hits          78372    26988    -51384     
- Misses        27647    81160    +53513     
+ Partials       5515     2782     -2733     
Flag Coverage Δ
integration1 ?
integration2 24.32% <0.58%> (+0.14%) ⬆️
unittests1 ?
unittests2 ?

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

Impacted Files Coverage Δ
...pinot/core/query/prefetch/DefaultFetchPlanner.java 3.22% <0.00%> (-80.65%) ⬇️
...ator/impl/bloom/OnHeapGuavaBloomFilterCreator.java 0.00% <0.00%> (-100.00%) ⬇️
...nt/local/segment/index/bloom/BloomIndexPlugin.java 0.00% <0.00%> (ø)
...ment/local/segment/index/bloom/BloomIndexType.java 0.00% <0.00%> (ø)
...ent/index/column/PhysicalColumnIndexContainer.java 0.00% <0.00%> (-91.21%) ⬇️
...ndex/converter/SegmentV1V2ToV3FormatConverter.java 0.00% <0.00%> (-78.42%) ⬇️
...egment/index/dictionary/DictionaryIndexPlugin.java 0.00% <0.00%> (ø)
.../segment/index/dictionary/DictionaryIndexType.java 0.00% <0.00%> (ø)
...ocal/segment/index/forward/ForwardIndexPlugin.java 0.00% <0.00%> (ø)
.../local/segment/index/forward/ForwardIndexType.java 0.00% <0.00%> (ø)
... and 48 more

... and 1492 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gortiz gortiz closed this Mar 21, 2023
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