Complete index spi #10184

gortiz · 2023-01-26T15:23:47Z

This is a very large PR that is a draft created when we were exploring #10183.

This PR is a fast forward from PRs:

The actual feature is in this PR, previous PRs are there in order to help the reviewers.

codecov-commenter · 2023-01-27T09:51:48Z

Codecov Report

Attention: Patch coverage is 81.89756% with 311 lines in your changes missing coverage. Please review.

Project coverage is 70.10%. Comparing base (985d482) to head (e590233).
Report is 2792 commits behind head on master.

Files with missing lines	Patch %	Lines
...ent/creator/impl/SameValueForwardIndexCreator.java	28.20%	28 Missing ⚠️
...ment/creator/impl/SegmentColumnarIndexCreator.java	79.64%	16 Missing and 7 partials ⚠️
...pi/index/creator/CombinedInvertedIndexCreator.java	44.73%	18 Missing and 3 partials ⚠️
...he/pinot/segment/spi/index/ForwardIndexConfig.java	62.00%	16 Missing and 3 partials ⚠️
...ocal/segment/index/inverted/InvertedIndexType.java	64.86%	8 Missing and 5 partials ⚠️
...not/segment/spi/index/IndexConfigDeserializer.java	82.08%	11 Missing and 1 partial ⚠️
...ocal/segment/index/loader/ForwardIndexHandler.java	87.77%	0 Missing and 11 partials ⚠️
...t/index/loader/bloomfilter/BloomFilterHandler.java	62.50%	9 Missing ⚠️
.../segment/local/segment/index/fst/FstIndexType.java	77.77%	4 Missing and 4 partials ⚠️
...egment/local/segment/index/json/JsonIndexType.java	72.41%	4 Missing and 4 partials ⚠️
... and 44 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #10184      +/-   ##
============================================
- Coverage     70.15%   70.10%   -0.05%     
- Complexity     6114     6246     +132     
============================================
  Files          2090     2111      +21     
  Lines        112209   113108     +899     
  Branches      17078    17152      +74     
============================================
+ Hits          78716    79296     +580     
- Misses        27943    28203     +260     
- Partials       5550     5609      +59

Flag	Coverage Δ
integration1	`24.44% <0.11%> (-0.10%)`	⬇️
integration2	`24.17% <0.11%> (+0.06%)`	⬆️
unittests1	`67.78% <81.87%> (-0.09%)`	⬇️
unittests2	`13.85% <0.00%> (-0.11%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…dIndexAndDictionaryBasedForwardIndexCreator

…uded in SegmentV1V2ToV3FormatConverter.java

61yao · 2023-03-31T04:50:45Z

...n/java/org/apache/pinot/segment/local/segment/creator/impl/SameValueForwardIndexCreator.java

+
+  @Override
+  public void putInt(int value) {
+    _delegate.putInt((int) _actualValue);


the value is not used?

Nope. This index creator is used to always store a constant value decided at construction time. I know it is strange, but it is only used in a strange situation. Specifically, sometimes we want to disable the forward index in a column with a text index in order to only let the user to search using the text index. There is a plan to actually be able to disable the forward index in that situation, but right now it is not possible, so the thing we do is to create a fake forward index that always store the same value for all rows.

given this was introduced in this PR - can we add some java doc? it confused me too

…nations

klsince · 2023-03-31T18:05:36Z

...java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java

bump this question. not blocking though.

npawar

LGTM!
Non-blocking questions/comments only, feel free to address later on. Will merge this now to avoid you having to rebase yet again after the weekend.
Thanks for breaking this into pieces and making it consumable for reviewers

npawar · 2023-03-31T00:35:03Z

...al/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java


  private String _instanceId;
  private Map<String, Map<String, String>> _instanceTierConfigs;
+  private boolean _dirty = true;


i know this class is fated to just be removed, but some comments in code for dirty and knownColumns would really help, in case it takes you some time to get to the demolition. it is pretty confusing without this context

npawar · 2023-03-31T00:40:31Z

...java/org/apache/pinot/segment/local/segment/creator/impl/inv/BitSlicedRangeIndexCreator.java

  private final RangeBitmap.Appender _appender;
  private final File _rangeIndexFile;
  private final long _minValue;
+  private final FieldSpec.DataType _valueType;


nit: s/valueType/dataType ?

npawar · 2023-03-31T18:19:25Z

...on-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java

  private static final String COLUMN_LENGTH_MAP_KEY = "columnLengthMap";
  private static final String COLUMN_CARDINALITY_MAP_KEY = "columnCardinalityMap";
  private static final String MAX_NUM_MULTI_VALUES_MAP_KEY = "maxNumMultiValuesMap";
-  private static final int DISK_SIZE_IN_BYTES = 20797327;


how did we increase metadata in the process of this?

This value can change a bit depending on the order of the indexes and stuff. @Jackie-Jiang recently changed the v1&v2 conversion in order to make it deterministic.

Today we also found that there was an error in the code and inverted indexes were created even if createInvertedIndexDuringSegmentGeneration was false. It should be fixed in #10524. In that PR the value of this const also changes, but to be reduced to 20797324. TBH I don't know what else may have changed the value, although the new change is just 3 bytes.

npawar · 2023-03-31T18:21:07Z

...n/java/org/apache/pinot/segment/local/segment/creator/impl/SameValueForwardIndexCreator.java

+
+  @Override
+  public void putInt(int value) {
+    _delegate.putInt((int) _actualValue);


given this was introduced in this PR - can we add some java doc? it confused me too

npawar · 2023-03-31T18:25:29Z

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

+    for (String columnName : indexConfigs.keySet()) {
+      FieldSpec fieldSpec = schema.getFieldSpecFor(columnName);
+      if (fieldSpec == null) {
+        Preconditions.checkState(schema.hasColumn(columnName),


do we need a precondition check again, if fieldSpec was null? i understand this is checking key, vs that one value, but can column exist with null fieldspec in that map?

npawar · 2023-03-31T18:29:21Z

...t-local/src/main/java/org/apache/pinot/segment/local/segment/index/bloom/BloomIndexType.java

-  public BloomFilterConfig getConfig(TableConfig tableConfig, Schema schema) {
-    throw new UnsupportedOperationException("To be implemented in a future PR");
+  public ColumnConfigDeserializer<BloomFilterConfig> createDeserializer() {
+    return IndexConfigDeserializer.fromIndexes("bloom", getIndexConfigClass())


isn't there a constant you created somewhre that you can use here? (for all *IndexType)

npawar · 2023-03-31T19:05:55Z

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/RangeIndexConfig.java

+  public RangeIndexConfig(@JsonProperty("enabled") Boolean enabled,
+      @JsonProperty("version") @Nullable Integer version) {
+    super(enabled != null && enabled && version != null);
+    _version = version != null ? version : 2;


can we take this from the creator/reader or define it here as current version, instead of hardcoding?

npawar · 2023-03-31T19:12:25Z

...n/java/org/apache/pinot/segment/local/segment/virtualcolumn/VirtualColumnIndexContainer.java

+    if (indexType.equals(StandardIndexes.forward())) {
+      return (I) _forwardIndex;
+    }
+    if (indexType.equals(StandardIndexes.inverted())) {


huh i didn't know we could create inverted index on an inverted column!

gortiz mentioned this pull request Jan 26, 2023

Index Service Provider Interface (SPI) #10183

Closed

5 tasks

gortiz marked this pull request as draft January 26, 2023 15:26

gortiz force-pushed the index-spi branch from 347242f to 7c4b734 Compare January 27, 2023 09:10

This was referenced Jan 27, 2023

[Index SPI] IndexType #10191

Merged

Draft commit with the IndexService draft gortiz/pinot#1

Draft

Draft commit with simplified version of all index types

91ada0c

gortiz mentioned this pull request Jan 27, 2023

Simplified version of all index types gortiz/pinot#2

Draft

gortiz added 9 commits January 27, 2023 11:34

Simplify IndexType with tons of UnsupportedOperationException

3ed6dfe

Remove BloomIndexType to make this code compile

049bf33

Merge branch 'index-spi-index-type-only' into index-spi-index-service

4b8462e

Recover simplified BloomIndexType

e1239a4

Merge branch 'index-spi-index-service' into index-spi-all-types

febc78b

Fix an error on IndexService

0e85045

Merge branch 'index-spi-index-service' into index-spi-all-types

d0cf567

Add all types to StandardIndexes

cb9758a

Refactor to remove ColumnIndexType

ac2421a

This was referenced Jan 27, 2023

Index spi: index service #10192

Merged

Index spi: all types #10193

Merged

Index spi: remove columnindextype #10194

Closed

Refactor to remove ColumnIndexType gortiz/pinot#3

Draft

gortiz added 9 commits January 27, 2023 16:17

Fix two checkstyle issues

6e4c5d3

Merge branch 'index-spi-index-type-only' into index-spi-index-service

b37b6ba

Merge branch 'index-spi-index-service' into index-spi-all-types

38b6617

Merge branch 'index-spi-all-types' into index-spi-remove-columnindextype

12dc249

Fix a checkstyle issue

bd4645c

supress unchecked cast warning in StandardIndexes.java

8e8edc1

Merge branch 'index-spi-index-service' into index-spi-all-types

30e64e3

Fix spotless code style violations

0186245

Merge branch 'index-spi-all-types' into index-spi-remove-columnindextype

74c8bd0

gortiz added 18 commits March 30, 2023 15:33

Add javadoc

a60ef32

Rename ForwardIndexType.getReader to read and add some javadoc

32e16e8

Make IndexReaderFactory classes private

14b6dc8

code style change

1a10045

fix typo

fcb6002

Add TODO

86bfe3d

Remove javadoc

3820504

s/withIsOptimizedDictionary/withOptimizedDictionary

767cd80

Add constant

321e966

code style

6c85d64

code style

c1ba5ff

Move range index version constant to RangeIndexConfig

7bb33b9

Fix an error on how fst property was read

5b54566

Make InvertedIndexType.ReaderFactory public, as it is used in Inverte…

41f5c27

…dIndexAndDictionaryBasedForwardIndexCreator

Support nullable text and fst columns

774eda6

Merge branch 'master' into index-spi

70b399e

Remove IndexType.storedAsBuffer given that master has this check incl…

5fb0a87

…uded in SegmentV1V2ToV3FormatConverter.java

Remove unused import

e0d45ce

61yao reviewed Mar 31, 2023

View reviewed changes

gortiz added 5 commits March 31, 2023 08:24

Change the expected size in bytes

d05f290

Add some extra tests

5adb264

Remove a confusing check and add a comment with some contextual expla…

864fcc7

…nations

remove unused methods in TextIndexConfigBuilder.

74ca733

Make StreamOp retry check resilient to NPE

e590233

klsince approved these changes Mar 31, 2023

View reviewed changes

npawar approved these changes Mar 31, 2023

View reviewed changes

npawar merged commit f69557e into apache:master Mar 31, 2023

gortiz mentioned this pull request Apr 3, 2023

Fix an error introduced in index-spi that ignores the createInvertedI… #10524

Merged

sajjad-moradi mentioned this pull request Jun 7, 2023

Segment Purger cannot purge old segments after schema evolution #10868

Closed

Complete index spi #10184

Complete index spi #10184

Uh oh!

Conversation

gortiz commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

npawar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gortiz commented Jan 26, 2023 •

edited

Loading

codecov-commenter commented Jan 27, 2023 •

edited

Loading