Codecov Report

Merging #10191 (41b28b0) into master (7c3c8e8) will increase coverage by 1.99%.
The diff coverage is 38.88%.

@@             Coverage Diff              @@
##             master   #10191      +/-   ##
============================================
+ Coverage     68.27%   70.26%   +1.99%     
- Complexity     5262     6027     +765     
============================================
  Files          2049     2053       +4     
  Lines        111033   111122      +89     
  Branches      16891    16899       +8     
============================================
+ Hits          75810    78085    +2275     
+ Misses        29816    27558    -2258     
- Partials       5407     5479      +72     

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

... and 194 files with indirect coverage changes

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

@Jackie-Jiang I've modified this PR in order to remove the IndexDeclaration, as we discussed online. I'll be checking the tests, but this should be ready to merge

Just realize this is copy-pasted from the class in segment local, but the original IndexHandler is modified recently..
Let's directly move the class to segment spi since there are very few usages of

I've just checked and there is no recent change in the interface. Both interfaces are not the same because I removed IndexCreatorProvider, as it is not needed in this version.

I'm going to move the class. It would increase the number of changes in this PR, but they should only be imports.

PS: I will move the current IndexHandler class, with IndexCreatorProvider. Therefore I will need to modify that class in following PRs

But in ColumnIndexType, the _indexName is always the lower case of the enum value. Can we always use IndexName? That can avoid a lot of confusion. If you really think we need 2 different names, we can call the internal one getInternalName()

I don't like having the two concepts either, but I think we need them for compatibility and usability reasons.

Yes, the ids of the literals in ColumnIndexType are in upper case and the names are the same value in lower case. But the literals are the following:

DICTIONARY("dictionary"),
  FORWARD_INDEX("forward_index"),
  INVERTED_INDEX("inverted_index"),
  BLOOM_FILTER("bloom_filter"),
  NULLVALUE_VECTOR("nullvalue_vector"),
  TEXT_INDEX("text_index"),
  FST_INDEX("fst_index"),
  JSON_INDEX("json_index"),
  RANGE_INDEX("range_index"),
  H3_INDEX("h3_index");

As you can see, all but dictionary and null value have the _index suffix.

In index-spi we have a new concept of id which is the one used to describe the index to the user. We use this id to indicate that something is wrong with an index and the user uses that id to define the config for that specific index. This new concept of id is different than the older one. We don't want the user to write the _index suffix everywhere like the following:

{
  "fieldConfigList": [
    {
       "name": “col1”,
       "indexes": {
         "inverted_index": {
           "enabled": true
         },
         "range_index": {
           "enabled": true
         },
         "text_index": {
           "type": “lucene”,
           "enableQueryCacheForTextIndex": false,
           "stopWordsInclude": ["a", "b", "c"]
         },
         "bloom_index": {
           "fpp": 0.01,
           "maxSizeInBytes": 1000000,
           "loadOnHeap": true
          }
       }
    }
  ]
}

And I think printing errors like "there was a problem with index of type " + indexType would be quite repetitive if they print "there was a problem with index of type bloom_index".

At the same time we cannot decide to just remove the _index suffix because that literal is used in the segment metadata, so we need to read segments commited with _index suffix. And at the same time the suffix is not always there (dictionary and null vector do not add that suffix) so we cannot just create the version with suffix by appending _index to the new user-friendly concept of id.

IMO it is okay to have the _index suffix for inverted, forward etc. We don't have bloom_index (it is bloom_filter), dictionary_index which is controversial.
E.g. in IndexingConfig we have _jsonIndexColumns, _rangeIndexColumns, _bloomFilterColumns

IMO it is okay to have the _index suffix for inverted, forward etc. We don't have bloom_index (it is bloom_filter), dictionary_index which is controversial.

IMO that is cheep, repetitive (we define these objects as keys in indexes) and in general a bad user experience that is very easy to solve (in fact, it is solved with this PR). We can discuss whether we want to use other names on methods (maybe not calling them getId and getName), but forcing users to use these not that friendly names when it is easy to let them use better names is not a good idea.

Should we rename this method to getName()?

I think id is a better description for this concept. Unlike names, ids usually imply unicity and stability in time, while name is usually a term more focused on how to present the information to the user and they usually can change.

Also, all other branches are already using id so I would prefer to do not have to change them again unless you really think name is better.