About FieldConfig.IndexType values

[gortiz]

Objective

The objective of this issue is to discuss about FieldConfig.IndexType, which:

• Has been superseded by Index Service Provider Interface (SPI) #10183
• Is being used inconsistently

We cannot remove this enum because we need to keep backward compatibility, but we should decide what to do with the four literals that are not documented and are not being used (or used in an inconsistent way).

Description

FieldConfig.indexTypes was intended to be the new way to create all indexes, but it was superseded by FieldConfig.indexes attribute introduced in #10183. The main problems with FieldConfig.indexTypes were that:

• Most indexes can be configured in some way. To do so, FieldConfig.indexTypes required to introduce unsafe string-based configs in FieldConfig.properties.
• IndexTypes is an enum that doesn't contain all valid indexes and includes some extra values that are not valid.

This issue is related to the last point. Reading the documentation, the valid values here should be: Text, FST, Timestamp and h3. But the enum right now contains the following literals:

• TEXT
• FST
• JSON
• H3
• INVERTED
• SORTED
• RANGE
• TIMESTAMP

The last 4 are not included in the documentation and they are not being consistently used in the code.

Inconsistent usage

To do this analysis I used tag release-0.12.1. The reason to do not use master is that #10183 (already merged) modifies a lot of code related to indexes, so I want to be sure that the inconsistent usage of these literals was already there before index-spi was merged:

Inverted

• It was introduced in the first version of this document (2020-02-07).
• It is not read in production code. The only two usages are in MutableSegmentImpl (here, and here). In both cases it is being used to report some error.
• It is being used in test, but its usage doesn't seem actually important.

Sorted

• It was introduced in the first version of this document (2020-02-07).
• It is only being used in HadoopSegmentPreprocessingJob. This class does not exist in master and it is not being used in master. The fact that it is not an actual index type and it was only used here makes me think that it was introduced as an attempt to do something, but it wasn't finished.

Range

• This was introduced in make it possible to specify field config properties for columns with JSON and RANGE indexes #7615 (2021-10-25).
• It is not used in production code. Like inverted, it is being used in tests, but just doesn't seem to do anything.
• Range is an actual index type, but no range index is created when this literal is added in indexTypes.

Timestamp

• This was introduced in Timestamp type index #8343 (2022-4-11).
• The only usage in production code is a check. This check fails if the FieldConfig points to a column that is not a timestamp but indexTypes contains the TIMESTAMP literal.
• Timestamp indexes are not actual indexes. There is no timestamp index writer, reader or handler. They are syntactic sugar (or an alias). When pinot detects that this syntactic sugar has to be applied (more on that later) what actually happens is that Pinot generates one extra row for each timestamp granularity and index that column with a range index.
• Contrary to what documentation says, FieldConfig.indexTypes doesn't have to contain the TIMESTAMP literal in order to activate this syntactic sugar. It is activated if and only if the FieldConfig.timestampConfig is not null. Whether or not TIMESTAMP is included in FieldConfig.indexTypes has zero impact on that decision.
• The timestamp syntactic sugar is just a modification of the table config and the schema and it is applied in:
  • SegmentProcessorConfig (when it is created)
  • IndexLoadingConfig (when it is configured from a TableConfig)
  • SegmentGeneratorConfig (when it is created)

[Jackie-Jiang]

Trying to understand the proposal here. Are you suggesting removing the 4 unused enums?
In long term, we want to use indexes config and remove IndexType, so I'm okay removing the unused one and add some comments stating that the remaining 4 are for backward compatibility purpose

[gortiz]

I'm just describing the problems I found to open a discussion looking for solutions.
Recently, I was involved in at least two discussion about the timestamp index or the sorted index where different people had different assumptions about the indexTypes values and their meaning and I wanted to make clear how are they used from code's perspective in order to use that as a base on which we can build the business or product terms we want to define to talk with users and even between devs.

About what to do with them. The options I see are:

1. Remove them
2. Ignore them (like what we do right now, but in a more consistent way)
3. Honor them.

I think the second is the better right now. Easier to implement and the one that doesn't actually impact existent table configs. It is not great from the developer perspective, but it is not that harmful.

Remove them

As you say, we cannot just remove literals without breaking backward compatibility.
At most, we may remove literals that were never documented.
If we do that, Pinot won't be able to read TableConfigs that were using undocumented things, but we can consider that if they are not documented, they are not part of the stable config and therefore there is no backward compatibility break.

Ignore them

But even if we do not remove them, we can use them in a deterministic way.
For example, we annotate them as deprecated and ensure that we don't use them anywhere (including tests).
We can also include a warning when they are using, to indicate users that these literals are actually doing nothing.

Specially in the case of timestamp, we should actually ignore the field and actually do the check if timestampConfig is not null, which is what actually enable the index.

Honor them

Another alternative would be just the opposite.
Modify the code to actually change its behavior when these literals are present.
That would also be a backward incompatible change, but it may be seen as a bugfix, given that if the user included range in their indexTypes, we can assume they though that a range index was going to be created.