-
Notifications
You must be signed in to change notification settings - Fork 1.2k
LUCENE-9705: Create Lucene90PointsFormat #52
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
Conversation
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.
As you mentioned, this is the only codec we're moving that still shares a chunk of logic with older codecs (through BKDWriter
and BKDReader
). A couple questions:
- How does this affect your planned work in LUCENE-9047, which was a big motivation for creating these new codecs? Will you just add another 'internal version' for the endianness change?
- Confirming my understanding, we plan to follow up with something like LUCENE-9820 to avoid having to share so many classes with older codecs in the future?
super.testMergeStability(); | ||
} | ||
|
||
public void testEstimatePointCount() throws IOException { |
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.
Is it possible to move these methods to BasePointsFormatTestCase
? It'd be nice to cut down on duplicated test cases.
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.
I moved some test but the ones that relay on knowing the maxPointsInLeafNode cannot be moved as the information is not expose through the PointValues API.
writeState.segmentInfo.name, | ||
writeState.segmentSuffix, | ||
Lucene86PointsFormat.DATA_EXTENSION); | ||
Lucene86RWPointsFormat.DATA_EXTENSION); |
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.
Small comment, seems clearer to reference Lucene86PointsFormat
here.
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.
good catch
I don't think we will need to add a new internal version. I think for backwards codecs we can wrap the IndexInputs so it would be transparent for the BKD reader.
I hope so :). What I am trying to avoid is to have to rewrite many BKD readers if we decide to change the Points API (for example LUCENE-9619). So separating reading a specific Index format from the API should help. |
* Solr's GlobalTracer is gone. Use OpenTracing's GlobalTracer, or even better: getters on CoreContainer, SolrQueryRequest, etc. * No more "samplingThreshold" cluster property. Instead, configure sampling specific to the plugin. * Ensure the active span is visible from all threads by using ExecutorUtil * JaegerTracerConfigurator is now configured via sys props / env vars
For now this is just a copy of Lucene86PointsFormat. The existing Lucene86PointsFormat is moved to backwards-codecs.
This is actually the last codec that needs to be moved to Lucene90 :)