Merge related HashMaps in FieldInfos#FieldNumbers into one map #13460
Conversation
| this.omitNorms = new HashMap<>(); | ||
| this.fieldProperties = new HashMap<>(); |
There was a problem hiding this comment.
I wonder if we should add omitNorms and storeTermVectors too? they are only added when they exist.
There was a problem hiding this comment.
We can add nameToNumber too?
There was a problem hiding this comment.
Yes, it looks like we can add all immutable attributes there indeed.
There was a problem hiding this comment.
I added those attributes to FieldProperties. I introduced a IndexOptionsProperties record to capture omitNorms and storeTermVectors.
There was a problem hiding this comment.
Should IndexOptionsProperties also capture IndexOptions?
There was a problem hiding this comment.
Oh I see, no it should not, because we set it to null when IndexOptions is NONE?
| indexOptions.clear(); | ||
| docValuesType.clear(); | ||
| dimensions.clear(); | ||
| fieldProperties.clear(); |
There was a problem hiding this comment.
There is maybe a bug that we don't clear vectorProps hashmap?
There was a problem hiding this comment.
This is a bug indeed. So if you call deleteAll, then you won't be able to index vectors with different options. I don't think it's a common use-case, which is likely why it's never been reported.
iverase
changed the title
| synchronized (this) { | ||
| fieldNumber = nameToNumber.get(fieldName); | ||
| fieldProperties = this.fieldProperties.get(fieldName); |
There was a problem hiding this comment.
This feels like we might change the behaviour here? Not sure why we are getting the number here on a synchronise block.
There was a problem hiding this comment.
This entire class is shared by the IndexWriter so access needs to be protected. The fact that access to docValuesType was not protected is more surprising to me, it seems to rely on subtle happens-before guarantees? Anyway, I like that your change is simplifying this.
| synchronized (this) { | ||
| fieldNumber = nameToNumber.get(fieldName); | ||
| fieldProperties = this.fieldProperties.get(fieldName); |
There was a problem hiding this comment.
This entire class is shared by the IndexWriter so access needs to be protected. The fact that access to docValuesType was not protected is more surprising to me, it seems to rely on subtle happens-before guarantees? Anyway, I like that your change is simplifying this.
|
This class seems to have change quite a bit and some methods did not have a clear path forward for backportso I set the milestone for this change to 10.0.0 |
This commit merges indexOptions, DocValuesType, FieldDimensions and FieldVectorProperties into one map by introducing the record FieldProperties.
In addition it moves to a record the internal classes FieldDimensions and FieldProperties.
fixes #13459