Index Service Provider Interface (SPI)

[gortiz]

Why

The objective of this proposal is to add the ability to include new index types at runtime in Apache Pinot. This opens the ability of adding third party indexes, including proprietary indexes. When we talk about index types in this proposal, we are focused on single field index. Constructions like StarTree indexes or future indexes that index more than one column are deliberately ignored.

PEP

https://docs.google.com/document/d/1L_GJ2tAgZVxI_Ssa59--Mso3mK_HeAS7s7G9261-sOE

The plan

All ideas presented here have been already drafted in this branch, where we have been exploring the problem in order to find the solution we propose here. Given the large number of changed in that branch, we think that it is better to use that branch as a reference, but split the changes in different PRs. Right now the idea is to create partial PRs in the following order:

• Add the IndexType without modifying already existing code. Include few example implementations
  - PR to master (already merged): [Index SPI] IndexType #10191
• Add the IndexService without modifying already existing code.
  - PR to master (reviewed & ready to merge): Index spi: index service #10192
• Partially implement IndexType for each existent index type without modifying already existing code. Methods that define how to create IndexCreators, IndexReaders, IndexHandlers, etc are not going to be implemented here.
  - PR to master (to review, with previous changes included): Index spi: all types #10193
• Just skip this one and focus on the last Refactor existing code to use IndexType whenever ColumnIdexType is being used right now.
  - PR to master (to review, with previous changes included): Index spi: remove columnindextype #10194
• The rest of the code
  - PR to master (to review, with previous changes included): Complete index spi #10184
  - This includes:
  - Create config objects for each index type
  - Transform old configs (including usage of IndexLoadingConfig and SegmentGeneratorConfig)
  - Refactor each IndexCreator to have a common interface.
  - Remove IndexCreatorProvider and IndexReaderProvider interfaces and implementations, moving the code to each IndexType. Here classes which delegate on these providers (like PhysicalColumnIndexContainer) will be modified to call the equivalent methods on IndexType.
  - Refactor classes like PhysicalColumnIndexContainer or SegmentColumnarIndexCreator to call just iterate over the IndexTypes returned by IndexService instead of explicitly ask for each one

---

Below is the original issue message. We created a PEP after the issue was created. In order to DRY, we are going to use the google drive document as the reference. Therefore text below is kept only for historical reasons.

---

Blockers in Apache Pinot 0.12

1. Index types are treated as enums, which are, by definition, decided at compile time. For example, ColumnIndexType.
2. Index configuration is specified in TableConfig, specially in FieldConfig and IndexingConfig. These classes are POJOs with one or more attributes per index type. Therefore to add a new index it is needed to add attributes and recompile the class.
3. There is at least one IndexCreator per index type, but each IndexCreator is different to the other. They do not fulfill a common contract. The same happen with IndexReader. Ideally IndexCreators and IndexReaders should be closer to IndexHandlers, which do follow the same contract for each index type.
4. IndexCreators are created using different XIndexCreatorProvider, where X is the index type (like ForwardIndexCreatorProvider) but:
   1. Each provider has its own methods with its own arguments that derive from IndexCreationContext.
   2. These XIndexCreatorProviders do not have a common interface.
   3. There is a IndexCreatorProvider interface, but it extends all other XIndexCreatorProvider. That pattern goes against the extensibility. For example, in case a new index type called Test was added, we would need to modify IndexCreatorProvider to also extend TextIndexCreatorProvider and implement its method in all implementations, which obviously requires to recompile the project.
   4. Same happens with IndexReaderProvider
   5. Same happens with MutableIndexProvider
5. Indexes are being used when PlanNodes are translated into physical operators. Right now this transformation is totally static. In case we want to add a new index we need to modify methods like FilterPlanNode.constructPhysicalOperator or FilterOperatorUtils.getLeafFilterOperator in order to add new transformation rules, which are eagerly executed.

The IndexService and the IndexType class

This proposal is evolves around the idea that indexes should be obtained from an IndexService. There should be a single instance on every Pinot component that uses indexes (like Pinot Servers). Optionally this singleton can be substituted by a bunch of static methods in the IndexService class.

This IndexService will contain a map from a String id to values of IndexType, which has to be a new interface that completely identifies an index. The IndexType interface will have methods that define the index id, how to configure the index given a TableConfig and how to obtain its IndexCreator, IndexReader, IndexHandler and optionally its MutableIndex. These last methods will require information from the environment. Specially they will require some contextual information (like statistics for the column) and some configuration. There should be a single IndexType instance for each index type in Pinot (ie one for forward indexes, one for range indexes, etc).

Pinot will have a new configuration to specify which indexes should be available and at startup Pinot will look for the corresponding IndexType in the classpath and add it to the IndexService singleton. After that, whenever Pinot requires to use an index Pinot will use the IndexService to get the IndexType instance. To look for all indexes in the classpath, the draft implementation uses an intermediate IndexPlugin interface that is discovered using Java Service Providers.

The index configuration

As said above, current way to configure is very static, requiring to add new attributes at compile time to the POJOs we have. At the same time, indexes are configured in different ways depending on their requirements and when it was created.

Right now, index configuration is spread across different fields in the TableConfig, which has two key resposibilities:

1. Being the class that is being used to define how the JSON is written by the users
2. Being the class that devs use to create the readers and creators.

Although it is not impossible to correctly implement both things in the same class, we think that it would be better to clearly differentiate between the concrete syntax used by the user to write JSONs and the classes with the final configuration (with default values resolved, for example). Having both responsibilities in the same classes makes more difficult to modify the way indexes are configured by the users without impacting on the code and the other way around. For example, bloom filters were originally designed to be configured as a list of columns that have to be indexed but at some point we wanted to let users to customize the bloom filter properties, so TableConfig contains both bloomFilterColumns and bloomFilterConfigs. Instead of being constraint at TableConfig level, this duality permeated to all other parts of the code, so a very common pattern was to first read bloomFilterColumns and then bloomFilterConfigs in several places of the code.

Previous TableConfig has other problems:

• Documentation is spread between tableIndexConfig and fieldConfigList. Text search support doc page says that fieldConfigList is the newest way to configure indexes, but when using that advanced configuration has to be done in a generic properties field, which was mapped to a Map<String, String>. This is bad for various reasons:
  • It creates a single namespace where properties of different index may collide.
  • It restricts the value of the configuration to strings, so in order to support advanced configurations devs must parse the string their self and users must know which syntax is expected for each value. This is even worse when the parsing has to be done in several different places in the code (for example, when creating an index and when reading an index).
• In order to add a new index, TableConfig source file has to be changed to add a new attribute in order to be known by Jackson. That is strong blocker in order to have index types defined at runtime.
• It is impossible to create instances or modify of TableConfig in a programmatic type safe way. Therefore we introduced some abstractions like IndexLoadingConfig or ColumnIndexCreationInfo so we can programmatically modify the config (mainly, but not only, in tests).
• In order to create or read some indexes, we need not only the configuration provided by the user but also some contextual information that has to be pre-processed (like for example the max or min values). Therefore we created classes like IndexCreationContext, where we merged the contextual information with index specific configurations. This is again incompatible with having index types defined in runtime. For example, we would need to create a method like IndexCreationContext.Common.forBloomFilter() for each new index type, which would require to modify the source code each time a new index is added.

TableConfig is not in fact very bad designed. The actual problem is that we are using TableConfig for everything. What this proposal does is to keep TableConfig as the concrete syntax specific object that is usually instantiated by Jackson when a table is deserialized from JSON. In parallel, each IndexType has a generic C (which stands for config) and defines how to transform a TableConfig into an instance of C

Therefore all the dirty code we have to introduce in order to support historical ways to configure the index is written only once and in the same method for each IndexType instead of being spread across the code. This should produce code that is easier to read and to maintain. Methods that are used to instantiate IndexCreators, IndexHandlers or IndexReaders will be explained bellow, but they do not receive the TableConfig (or a wrapper like IndexLoadingConfig) as an argument. Instead, they receive the already prepared generic C object they expect.

With the change explained above we have the ability to specify all IndexType methods without depending on the specially crafted interfaces like IndexLoadingConfig, SegmentGeneratorConfig or IndexCreationContext. But it doesn't explain how to open the TableConfig to be able to store indexes that are unknown at compile time. The proposal here is to modify a little bit the idea of configuring all indexes in FieldConfig. Instead of list active indexes in the field indexTypes, here we propose to create a new field called indexes. That field is going to be either null or a JSON object whose keys are the id of the configured indexes and the values are defined by each IndexType. To implement that, we propose the following:

• FieldConfig.indexes will be of type JsonNode, which is the most abstract Jackson type. Anything that can be deserialized with Jackson can be deserialized into a JsonNode.
• The class IndexType will have a method that returns the unique id of the index (for example, ForwardIndexType.getId() can return "forward").
• The class IndexType will also have a method that given a JsonNode, returns an instance of C.

When Pinot requires to transform FieldConfig.indexes from JsonNode into proper configuration objects, it will do something like the following:

  if (fieldConfig.indexes.isNull) {
    return null;
  }
  if (fieldConfig.indexes.isObject) {
    Iterator<Map.Entry<String, JsonNode>> it = node.fields();

    Map<IndexType, Object> builder = new HashMap<>();
    while (it.hasNext()) {
      Map.Entry<String, JsonNode> entry = it.next();
      String indexId = entry.getKey();

      IndexType indexType = IndexService.getInstance().getIndexType(indexId);
      if (indexType == null) {
         // decide what to do: continue or fail
      }
      JsonNode indexNode = entry.getValue();
      Object deserialized = indexType.deserialize(indexNode);
      builder.put(indexType, deserialized);
    }
  }

By doing that, we could be sure that if we do something like map.get(BloomIndexType.INSTACE) we will receive a BloomFilterConfig. The draft includes a intermediate class called FieldIndexConfigs which is a typesafe wrapper on top of a map like the one shown here.

So, for example, this configuration configuration that is currently supported by Pinot:

{
  "tableIndexConfig": {
    "rangeIndexColumns": [
       "col1"
    ],
    "bloomFilterConfigs": {
      "col1": {
        "fpp": 0.01,
        "maxSizeInBytes": 1000000,
        "loadOnHeap": true
      }
    },
    "invertedIndexColumns": [
      "col1"
    ]
  },
  "fieldConfigList": [
    {
       "name": “col1”,
       "indexTypes": [
          “text”
       ],
       "properties": {
          "fstType": “lucene”,
          "stopWordsInclude": "a,b,c",
          "enableQueryCacheForTextIndex": “false”,
          "isSegmentPartitioned": “true” // this property is not related to an specific index
       }
    }
  ]
}

Could be implemented as:

{
  "fieldConfigList": [
    {
       "name": “col1”,
       "indexes": {
         "inverted": true, // or {enabled: true}
         "range": true, // or {enabled: true}
         "text": {
           "type": “lucene”,
           "enableQueryCacheForTextIndex": false,
           "stopWordsInclude": ["a", "b", "c"]
         }
         "bloom": {
           "fpp": 0.01,
           "maxSizeInBytes": 1000000,
           "loadOnHeap": true
          },
       },
       "properties": {
          "isSegmentPartitioned": “true” // this property is not related to an specific index
       }
    }
  ]
}

In cases like BloomFilter, when BloomFilterConfig is already serializable/deserializable with Jackson, the method that transform from JSON to object could be:

  BloomFilterConfig deserialize(JsonNode node)
      throws IOException {
    return JsonUtils.jsonNodeToObject(node, getIndexConfigClass());
  }

Assuming that Java null means that the index is not configured, in case we need to customize that, for example on inverted indexes when there is no argument, we can do the same:

  public Boolean deserialize(JsonNode node)
      throws IOException {
    if (node.isBoolean()) {
      if (node.booleanValue()) {
        return Boolean.TRUE;
      } else {
        return null;
      }
    }
    throw ...
  }

In case we want to support different ways to configure the index, for example RangeIndexType when we would like to either use true to use the default or an object to be able to indicate the version of the range index, we can do something like:

  public RangeIndexConfig deserialize(JsonNode node)
      throws IOException {
    if (node.isBoolean()) {
      if (node.booleanValue()) {
        return getDefaultConfig();
      } else {
        return null;
      }
    }
    return JsonUtils.jsonNodeToObject(node, getIndexConfigClass());
  }

Most of these methods can also be delegated to Jackson by using customized deserializers, but we think that having this method here is more explicit and requires less knowledge of Jackson, which could make it easier to read.

This proposal makes this new way to configure indexes optional to indexes in Apache Pinot repository but mandatory for indexes outside this repo. Therefore the proposal must support the older way to configure indexes. This can partially be done by the already mentioned method that transform from TableConfig to C, but we may also need other methods to transform from IndexLoadingConfig and SegmentGeneratorConfig to C, as they are usually modified by tests.

[gortiz]

The partial PRs are subsets of the changes included in #10184. These partial PRs are here to help with the review. It is not clear whether we are going to merge the whole PR or the partial PRs, but it is important to understand that the partial PRs include some design decisions that may look strange in the partial PR because they were designed like that to solve some problem we are going to find in future PRs.

We need to analyze the whole picture to understand some design decisions that are shown in the partial PRs.

[gortiz]

cc @zhtaoxiang @walterddr @npawar @klsince

[npawar]

fyi @siddharthteotia @yupeng9 @vvivekiyer

[siddharthteotia]

Yes I will be going over the SPI PRs to be aware of the changes. @Jackie-Jiang suggested to start with IndexType.

[yupeng9]

Thanks for starting this effort. It's a huge one and it will be a significant improvement to the Pinot architecture. A couple of questions:

• From the descriptions, this seems to have quite a few breaking changes. Will this be merged after 1.0 release?
• Can you put the description into a google doc (PEP?), so the ease of commenting?