-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce TestLucene90DocValuesFormatVariableSkipInterval for testing docvalues skipper index #13550
Conversation
lucene/test-framework/src/java/org/apache/lucene/tests/codecs/skipper/SkipperCodec.java
Outdated
Show resolved
Hide resolved
lucene/test-framework/src/java/org/apache/lucene/tests/index/BaseDocValuesFormatTestCase.java
Outdated
Show resolved
Hide resolved
@jpountz I added a new test and change the title and description of the issue as we don't need to add a new Codec. |
@@ -96,6 +97,7 @@ public Lucene90DocValuesConsumer( | |||
state.segmentInfo.getId(), | |||
state.segmentSuffix); | |||
maxDoc = state.segmentInfo.maxDoc(); | |||
this.skipIndexIntervalSize = skipIndexIntervalSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we make any assumption anywhere that this needs to be a power of two? If so we should validate it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No , there is no assumption. On the other hand I added in the codec a check so skipIndexIntervalSize must be 1. In addtion I replace 1 << x by an integer to remove the suggestion that this is required.
In #13449 we introduced a sparse index on top of doc values. One of the challenges was to properly test it as the default interval size is 4096 documents so we need a fair amount of documents to have a good number of intervals.
Therefore in this commit we propose to be able to configure dynamically the interval size for testing, and add a new test that changes the interval size randomly.