jpountz commented on a change in pull request #1394: LUCENE-9300: Fix field
infos update on doc values update
URL: https://github.com/apache/lucene-solr/pull/1394#discussion_r402231793
##########
File path: lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java
##########
@@ -543,27 +543,39 @@ public synchronized boolean writeFieldUpdates(Directory
dir, FieldInfos.FieldNum
try {
// clone FieldInfos so that we can update their dvGen separately from
- // the reader's infos and write them to a new fieldInfos_gen file
- FieldInfos.Builder builder = new FieldInfos.Builder(fieldNumbers);
- // cannot use builder.add(reader.getFieldInfos()) because it does not
- // clone FI.attributes as well FI.dvGen
+ // the reader's infos and write them to a new fieldInfos_gen file.
+ int maxFieldNumber = -1;
+ Map<String, FieldInfo> newFields = new HashMap<>();
for (FieldInfo fi : reader.getFieldInfos()) {
- FieldInfo clone = builder.add(fi);
- // copy the stuff FieldInfos.Builder doesn't copy
- for (Entry<String,String> e : fi.attributes().entrySet()) {
- clone.putAttribute(e.getKey(), e.getValue());
- }
- clone.setDocValuesGen(fi.getDocValuesGen());
+ // cannot use builder.add(fi) because it does not preserve
+ // the local field number. Field numbers can be different from the
global ones
+ // if the segment was created externally (with
IndexWriter#addIndexes(Directory)).
+ FieldInfo clone = cloneFieldInfo(fi, fi.number);
+ newFields.put(clone.name, clone);
+ maxFieldNumber = Math.max(clone.number, maxFieldNumber);
}
// create new fields with the right DV type
+ FieldInfos.Builder builder = new FieldInfos.Builder(fieldNumbers);
for (List<DocValuesFieldUpdates> updates : pendingDVUpdates.values()) {
DocValuesFieldUpdates update = updates.get(0);
- FieldInfo fieldInfo = builder.getOrAdd(update.field);
- fieldInfo.setDocValuesType(update.type);
+ FieldInfo globalFieldInfo = builder.getOrAdd(update.field);
+ globalFieldInfo.setDocValuesType(update.type);
+ FieldInfo segmentFieldInfo = newFields.get(update.field);
+ if (segmentFieldInfo == null) {
+ // the field is not present in this segment so we can fallback to
the global fields.
+ if (globalFieldInfo.number <= maxFieldNumber) {
+ // the global field number is already used in this segment for a
different field so we force a new one locally.
+ globalFieldInfo = cloneFieldInfo(globalFieldInfo,
++maxFieldNumber);
+ }
Review comment:
I think we should either have an `else` branch that updates the
`maxFieldNumber` to avoid conflicts (in case dv updates introduce multiple
fields) or always clone the `FieldInfo` to update the number. I have a
preference for the latter since it requires fewer branches and makes the code
easier to reason about?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
