make index creator provision pluggable #7885

richardstartin · 2021-12-09T16:00:27Z

This allows index creation to be intercepted, so that the current static logic in SegmentIndexCreator can be extended or overridden. This is achieved by introducing a new interface IndexCreatorProvider which provides various new index creators from an IndexCreationContext which bundles all information about index creation. External users can register a decorator which can enhance or entirely replace the default index creator provision logic. Typically, a registered decorator should pattern match the IndexCreatorContext for very specific cases, and return a custom implementation only in those cases, and merely delegate to the default provisioning logic otherwise.

Design doc at #7895

codecov-commenter · 2021-12-09T16:50:47Z

Codecov Report

Merging #7885 (f861dd6) into master (fb12a50) will decrease coverage by 43.69%.
The diff coverage is 15.66%.

❗ Current head f861dd6 differs from pull request most recent head 21140ba. Consider uploading reports for the commit 21140ba to get more accurate results

@@              Coverage Diff              @@
##             master    #7885       +/-   ##
=============================================
- Coverage     71.34%   27.65%   -43.70%     
=============================================
  Files          1587     1583        -4     
  Lines         82071    81996       -75     
  Branches      12267    12241       -26     
=============================================
- Hits          58553    22675    -35878     
- Misses        19550    57261    +37711     
+ Partials       3968     2060     -1908

Flag	Coverage Δ
integration1	`?`
integration2	`27.65% <15.66%> (+<0.01%)`	⬆️
unittests1	`?`
unittests2	`?`

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

Impacted Files	Coverage Δ
.../java/org/apache/pinot/common/utils/DataTable.java	`89.47% <ø> (ø)`
...e/pinot/core/common/datatable/DataTableImplV2.java	`0.00% <0.00%> (-77.78%)`	⬇️
...rg/apache/pinot/core/minion/RawIndexConverter.java	`0.00% <0.00%> (-56.61%)`	⬇️
...ache/pinot/core/plan/InstanceResponsePlanNode.java	`100.00% <ø> (ø)`
...nverttorawindex/ConvertToRawIndexTaskExecutor.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ment/creator/impl/DefaultIndexCreatorProvider.java	`0.00% <0.00%> (ø)`
...ment/creator/impl/SegmentColumnarIndexCreator.java	`0.00% <0.00%> (-84.40%)`	⬇️
...ocal/segment/index/loader/IndexHandlerFactory.java	`0.00% <0.00%> (-100.00%)`	⬇️
...t/index/loader/bloomfilter/BloomFilterHandler.java	`0.00% <0.00%> (-77.59%)`	⬇️
...nt/index/loader/invertedindex/FSTIndexHandler.java	`0.00% <0.00%> (-82.70%)`	⬇️
... and 1171 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 fb12a50...21140ba. Read the comment docs.

...in/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java

Jackie-Jiang

This is great. With this we can easily plug in out own index creators

pinot-core/src/main/java/org/apache/pinot/core/minion/RawIndexConverter.java

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreatorProvider.java

...in/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java

siddharthteotia · 2021-12-13T23:26:59Z

@richardstartin just seeing this. I will review it today. Looks like the design doc link was intended to be an issue but it is linking to this PR ?

pinot-core/src/main/java/org/apache/pinot/core/minion/RawIndexConverter.java

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreatorProvider.java

...segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/TextIndexCreatorProvider.java

...in/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java

...in/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreatorProviders.java

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java

...segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/TextIndexCreatorProvider.java

...in/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java

...in/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java

amrishlal · 2021-12-15T18:07:17Z

If needed in future, would it be possible to extend these interfaces to allow for creation of multiple indices on a column or would it lead to breaking existing index plugins? For example, I could create an H3 index and a quad-tree index on a geospatial column, or a flat index and a tree index on a JSON column and then depending upon the type of query being evaluated, pick one out of the two available indices to evaluate the query.

richardstartin · 2021-12-15T18:19:56Z

If needed in future, would it be possible to extend these interfaces to allow for creation of multiple indices on a column or would it lead to breaking existing index plugins? For example, I could create an H3 index and a quad-tree index on a geospatial column, or a flat index and a tree index on a JSON column and then depending upon the type of query being evaluated, pick one out of the two available indices to evaluate the query.

great question. No, the ability to create multiple indexes on a column is not limited by this PR: that logic stays in SegmentColumnarIndexCreator.

.../org/apache/pinot/segment/local/segment/index/loader/invertedindex/InvertedIndexHandler.java

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java

richardstartin · 2021-12-16T10:00:07Z

@siddharthteotia there is now only one method for creating text indexes now.

siddharthteotia

LGTM. Thanks for addressing comments

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java

Jackie-Jiang

LGTM otherwise

Jackie-Jiang · 2021-12-16T19:17:59Z

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java

+  }
+
+  class Text extends Wrapper {
+    private final boolean _commitOnClose;


This is not really required as it only applies to mutable index (for consuming segment)

well, it was hardcoded to be always true, with a comment to name it. Now it has a variable to give it a name and declare its purpose.

Jackie-Jiang · 2021-12-16T19:34:00Z

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java

+ * The responsibility for ensuring that the correct parameters for a particular
+ * index type lies with the caller.
+ */
+public interface IndexCreationContext {


There are some index independent properties that can be moved to the common: min, max, sortedUniqueValues

They can be added later if they're ever necessary. It's always easier to add things than take them away.

richardstartin force-pushed the pluggable-index-creation branch from d42823d to 4cc35e3 Compare December 9, 2021 16:13

richardstartin marked this pull request as draft December 9, 2021 16:18

richardstartin force-pushed the pluggable-index-creation branch from 4cc35e3 to a28f516 Compare December 9, 2021 17:35

richardstartin commented Dec 9, 2021

View reviewed changes

...in/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java Outdated Show resolved Hide resolved

richardstartin commented Dec 9, 2021

View reviewed changes

...in/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java Outdated Show resolved Hide resolved

Jackie-Jiang reviewed Dec 9, 2021

View reviewed changes

richardstartin force-pushed the pluggable-index-creation branch 2 times, most recently from 874335b to 989f68b Compare December 13, 2021 12:46

richardstartin mentioned this pull request Dec 13, 2021

Pluggable indexes #7895

Closed

richardstartin changed the title ~~make index creator provision interceptible~~ make index creator provision pluggable Dec 13, 2021

richardstartin marked this pull request as ready for review December 13, 2021 13:00

richardstartin force-pushed the pluggable-index-creation branch from 989f68b to d4a8970 Compare December 13, 2021 14:11

richardstartin mentioned this pull request Dec 13, 2021

make index readers/loaders pluggable #7897

Merged

klsince reviewed Dec 13, 2021

View reviewed changes

...in/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java Outdated Show resolved Hide resolved

richardstartin force-pushed the pluggable-index-creation branch from b9344ef to 24b64ff Compare December 13, 2021 19:42

Jackie-Jiang reviewed Dec 14, 2021

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/minion/RawIndexConverter.java Outdated Show resolved Hide resolved

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreatorProvider.java Show resolved Hide resolved

siddharthteotia reviewed Dec 14, 2021

View reviewed changes

...segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/TextIndexCreatorProvider.java Outdated Show resolved Hide resolved

siddharthteotia reviewed Dec 14, 2021

View reviewed changes

...in/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java Show resolved Hide resolved

siddharthteotia reviewed Dec 14, 2021

View reviewed changes

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java Outdated Show resolved Hide resolved

siddharthteotia reviewed Dec 14, 2021

View reviewed changes

...in/java/org/apache/pinot/segment/local/segment/creator/impl/DefaultIndexCreatorProvider.java Show resolved Hide resolved

siddharthteotia reviewed Dec 14, 2021

View reviewed changes

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreatorProviders.java Show resolved Hide resolved

siddharthteotia reviewed Dec 14, 2021

View reviewed changes

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java Outdated Show resolved Hide resolved

richardstartin force-pushed the pluggable-index-creation branch 6 times, most recently from bb122fc to d9c38cf Compare December 14, 2021 18:51

Jackie-Jiang reviewed Dec 15, 2021

View reviewed changes

klsince reviewed Dec 15, 2021

View reviewed changes

.../org/apache/pinot/segment/local/segment/index/loader/invertedindex/InvertedIndexHandler.java Show resolved Hide resolved

richardstartin force-pushed the pluggable-index-creation branch from 81495a5 to 02aa71d Compare December 15, 2021 22:18

make index creation interceptible/pluggable

9dfc034

richardstartin force-pushed the pluggable-index-creation branch from ea56de2 to 9dfc034 Compare December 15, 2021 22:23

siddharthteotia reviewed Dec 16, 2021

View reviewed changes

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java Outdated Show resolved Hide resolved

richardstartin force-pushed the pluggable-index-creation branch from 1cf8942 to ca18760 Compare December 16, 2021 09:57

richardstartin force-pushed the pluggable-index-creation branch from ca18760 to 1b3eb5a Compare December 16, 2021 10:38

allow override of text index creation as well as FST

21140ba

richardstartin force-pushed the pluggable-index-creation branch from 2b42bc9 to 21140ba Compare December 16, 2021 13:35

siddharthteotia approved these changes Dec 16, 2021

View reviewed changes

siddharthteotia reviewed Dec 16, 2021

View reviewed changes

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/creator/IndexCreationContext.java Show resolved Hide resolved

richardstartin added 2 commits December 16, 2021 18:58

refactor BitSlicedRangeIndexCreator not to need ColumnMetadata

99b5d9e

clarify purpose of IndexCreationContext.Text constructors

02ca79f

Jackie-Jiang approved these changes Dec 16, 2021

View reviewed changes

Jackie-Jiang merged commit f9ab252 into apache:master Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make index creator provision pluggable #7885

make index creator provision pluggable #7885

richardstartin commented Dec 9, 2021 •

edited

Loading

codecov-commenter commented Dec 9, 2021 •

edited

Loading

Jackie-Jiang left a comment

siddharthteotia commented Dec 13, 2021

amrishlal commented Dec 15, 2021 •

edited

Loading

richardstartin commented Dec 15, 2021

richardstartin commented Dec 16, 2021 •

edited

Loading

siddharthteotia left a comment

Jackie-Jiang left a comment

Jackie-Jiang Dec 16, 2021

richardstartin Dec 16, 2021

Jackie-Jiang Dec 16, 2021

richardstartin Dec 16, 2021

make index creator provision pluggable #7885

make index creator provision pluggable #7885

Conversation

richardstartin commented Dec 9, 2021 • edited Loading

codecov-commenter commented Dec 9, 2021 • edited Loading

Codecov Report

Jackie-Jiang left a comment

Choose a reason for hiding this comment

siddharthteotia commented Dec 13, 2021

amrishlal commented Dec 15, 2021 • edited Loading

richardstartin commented Dec 15, 2021

richardstartin commented Dec 16, 2021 • edited Loading

siddharthteotia left a comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang Dec 16, 2021

Choose a reason for hiding this comment

richardstartin Dec 16, 2021

Choose a reason for hiding this comment

Jackie-Jiang Dec 16, 2021

Choose a reason for hiding this comment

richardstartin Dec 16, 2021

Choose a reason for hiding this comment

richardstartin commented Dec 9, 2021 •

edited

Loading

codecov-commenter commented Dec 9, 2021 •

edited

Loading

amrishlal commented Dec 15, 2021 •

edited

Loading

richardstartin commented Dec 16, 2021 •

edited

Loading