You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/04/01 11:05:02 UTC
[GitHub] [lucene-solr] jimczi opened a new pull request #1394: LUCENE-9300:
Fix field infos update on doc values update
jimczi opened a new pull request #1394: LUCENE-9300: Fix field infos update on doc values update
URL: https://github.com/apache/lucene-solr/pull/1394
Today a doc values update creates a new field infos file that contains the original field infos updated for the new generation as well as the new fields created by the doc values update.
However existing fields are cloned through the global fields (shared in the index writer) instead of the local ones (present in the segment).
In practice this is not an issue since field numbers are shared between segments created by the same index writer.
But this assumption doesn't hold for segments created by different writers and added through IndexWriter#addIndexes(Directory).
In this case, the field number of the same field can differ between segments so any doc values update can corrupt the index
by assigning the wrong field number to an existing field in the next generation.
When this happens, queries and merges can access wrong fields without throwing any error, leading to a silent corruption in the index.
This change ensures that we preserve local field numbers when creating
a new field infos generation.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] juanka588 commented on a change in pull request
#1394: LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
juanka588 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_r401559955
##########
File path: lucene/core/src/test/org/apache/lucene/index/TestNumericDocValuesUpdates.java
##########
@@ -1483,6 +1484,81 @@ public void testAddIndexes() throws Exception {
IOUtils.close(dir1, dir2);
}
+ public void testUpdatesAterAddIndexes() throws Exception {
Review comment:
after?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r402394140
##########
File path: lucene/core/src/test/org/apache/lucene/index/TestNumericDocValuesUpdates.java
##########
@@ -1483,6 +1484,83 @@ public void testAddIndexes() throws Exception {
IOUtils.close(dir1, dir2);
}
+ public void testUpdatesAfterAddIndexes() throws Exception {
Review comment:
++, I pushed https://github.com/apache/lucene-solr/pull/1394/commits/00d755888d4744ff6a1beba17e20580f43841f12
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
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_r402233937
##########
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);
Review comment:
maybe add a comment on the above `getOrAdd` call to mention that it never needs to add the field?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
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_r401619000
##########
File path: lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java
##########
@@ -543,27 +543,30 @@ 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.
+ 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 = new FieldInfo(fi);
+ newFields.put(clone.name, clone);
}
// 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 fi = newFields.get(update.field);
+ if (fi == null) {
+ // the field is not present in this segment so we can fallback to the global fields.
+ fi = builder.getOrAdd(update.field);
Review comment:
Had a quick discussion about this line with Jim. This isn't good enough as this might return a field number that already exists in the reader if the field exists in the writer with a number that it less than the number of fields in the reader.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r402404730
##########
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:
we should, thanks. I rewrote the logic in my last commits. I thought that `addOrGet` would preserve the dv type but it doesn't.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r402394008
##########
File path: lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java
##########
@@ -644,6 +656,13 @@ public synchronized boolean writeFieldUpdates(Directory dir, FieldInfos.FieldNum
return true;
}
+ private FieldInfo cloneFieldInfo(FieldInfo fi, int fieldNumber) {
+ return new FieldInfo(fi.name, fieldNumber, fi.hasVectors(), fi.omitsNorms(), fi.hasPayloads(),
+ fi.getIndexOptions(), fi.getDocValuesType(), fi.getDocValuesGen(), new HashMap<>(fi.attributes()),
+ fi.getPointDimensionCount(), fi.getPointIndexDimensionCount(), fi.getPointNumBytes(), fi.isSoftDeletesField());
+
Review comment:
++, https://github.com/apache/lucene-solr/pull/1394/commits/34718cb64e747e56ebc5341f9180d2f12aace029
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r402403738
##########
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)).
Review comment:
++
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r401750154
##########
File path: lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java
##########
@@ -543,27 +543,36 @@ 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 fi = newFields.get(update.field);
+ if (fi == null) {
+ // the field is not present in this segment so we can fallback to the global fields.
+ fi = builder.getOrAdd(update.field);
+ if (fi.number <= maxFieldNumber) {
+ // the global field number is already used in this segment for a different field so we force a new one locally.
+ fi = cloneFieldInfo(fi, ++maxFieldNumber);
Review comment:
> Do we need to set the doc-value type before cloning to make sure the doc-value type is set on the writer?
thanks, I pushed https://github.com/apache/lucene-solr/pull/1394/commits/090ef78171dfe671b0fb1ab238951fdd617bd65c
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
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_r401719320
##########
File path: lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java
##########
@@ -543,27 +543,36 @@ 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 fi = newFields.get(update.field);
+ if (fi == null) {
+ // the field is not present in this segment so we can fallback to the global fields.
+ fi = builder.getOrAdd(update.field);
+ if (fi.number <= maxFieldNumber) {
+ // the global field number is already used in this segment for a different field so we force a new one locally.
+ fi = cloneFieldInfo(fi, ++maxFieldNumber);
Review comment:
Do we need to set the doc-value type before cloning to make sure the doc-value type is set on the writer? I also wonder if we should use `FieldInfos.Builder` since this logic you implemented looks similar to `FieldInfos.Builder#add`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r401672435
##########
File path: lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java
##########
@@ -543,27 +543,30 @@ 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.
+ 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 = new FieldInfo(fi);
+ newFields.put(clone.name, clone);
}
// 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 fi = newFields.get(update.field);
+ if (fi == null) {
+ // the field is not present in this segment so we can fallback to the global fields.
+ fi = builder.getOrAdd(update.field);
Review comment:
How to introduce the same bug but the other way around :(. Thanks for catching this Adrien. I pushed https://github.com/apache/lucene-solr/pull/1394/commits/abd5e08170cc8b7963c0aff3d58afa5766afcb32 to handle this case properly.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
s1monw 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_r402339219
##########
File path: lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java
##########
@@ -644,6 +656,13 @@ public synchronized boolean writeFieldUpdates(Directory dir, FieldInfos.FieldNum
return true;
}
+ private FieldInfo cloneFieldInfo(FieldInfo fi, int fieldNumber) {
+ return new FieldInfo(fi.name, fieldNumber, fi.hasVectors(), fi.omitsNorms(), fi.hasPayloads(),
+ fi.getIndexOptions(), fi.getDocValuesType(), fi.getDocValuesGen(), new HashMap<>(fi.attributes()),
+ fi.getPointDimensionCount(), fi.getPointIndexDimensionCount(), fi.getPointNumBytes(), fi.isSoftDeletesField());
+
Review comment:
extra newline
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi merged pull request #1394: LUCENE-9300: Fix
field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi merged pull request #1394: LUCENE-9300: Fix field infos update on doc values update
URL: https://github.com/apache/lucene-solr/pull/1394
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
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_r402468813
##########
File path: lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java
##########
@@ -543,27 +543,37 @@ 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> perName = new HashMap<>();
Review comment:
nit: we call it `byName`elsewhere
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r402936952
##########
File path: lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java
##########
@@ -543,27 +543,37 @@ 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> perName = new HashMap<>();
Review comment:
https://github.com/apache/lucene-solr/pull/1394/commits/9fed4319409a4d6c3a70e500cc51a58766116e21
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
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_r402234114
##########
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)).
Review comment:
```suggestion
// if the segment was created externally (and added to this index with IndexWriter#addIndexes(Directory)).
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
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_r402225938
##########
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);
Review comment:
Sorry I wasn't sure how that worked when doing previous reviews, but the field infos of the writer should be updated when the doc-value update is performed. So the FieldInfo should always exist at this point and the doc-value type should always be correct. So we should be able to assert here instead of calling `setDocValuesType`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r401749799
##########
File path: lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java
##########
@@ -543,27 +543,36 @@ 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 fi = newFields.get(update.field);
+ if (fi == null) {
+ // the field is not present in this segment so we can fallback to the global fields.
+ fi = builder.getOrAdd(update.field);
+ if (fi.number <= maxFieldNumber) {
+ // the global field number is already used in this segment for a different field so we force a new one locally.
+ fi = cloneFieldInfo(fi, ++maxFieldNumber);
Review comment:
> I also wonder if we should use FieldInfos.Builder since this logic you implemented looks similar to FieldInfos.Builder#add.
I don't think it works since the logic there is to use the global field number if the field already exists in another segment. In the logic above we try to re-assign the global field number locally since it clashes with a local one.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
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_r402228065
##########
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);
Review comment:
Sorry I suggested this on the previous review, but the doc-value type is actually supposed to be set on the updateDocValue call, so we should even be able to assert that the doc-value type is correct at this stage instead of setting it.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r402403634
##########
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);
+ }
+ newFields.put(update.field, globalFieldInfo);
+ } else {
+ segmentFieldInfo.setDocValuesType(update.type);
+ newFields.put(update.field, segmentFieldInfo);
Review comment:
++, removed
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r402403553
##########
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:
Thanks, I reworked the logic to always clone and reassign the field number.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
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_r402231946
##########
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);
+ }
+ newFields.put(update.field, globalFieldInfo);
+ } else {
+ segmentFieldInfo.setDocValuesType(update.type);
+ newFields.put(update.field, segmentFieldInfo);
Review comment:
this `put` call should be unnecessary since the field is already in the map?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r402403065
##########
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);
Review comment:
++, https://github.com/apache/lucene-solr/pull/1394/commits/0b5c6c896bcc67f8ec6d9206d5dc9caa5cc5ee81
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] s1monw commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
s1monw 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_r402341962
##########
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:
we should set the DV type here too no?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r402402939
##########
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);
Review comment:
I added an [assertion](https://github.com/apache/lucene-solr/pull/1394/commits/0b5c6c896bcc67f8ec6d9206d5dc9caa5cc5ee81) to check the dv type of the global field but we still need to set the dv type on the cloned field since it is not preserved by the `getOrAdd` call.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jpountz commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
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_r402237062
##########
File path: lucene/core/src/test/org/apache/lucene/index/TestNumericDocValuesUpdates.java
##########
@@ -1483,6 +1484,83 @@ public void testAddIndexes() throws Exception {
IOUtils.close(dir1, dir2);
}
+ public void testUpdatesAfterAddIndexes() throws Exception {
Review comment:
It might be better to split into one unit test for the addIndexes case when the updated field already exists in the segment, and another one for the case when the updated field doesn't exist in the segment yet?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on issue #1394: LUCENE-9300: Fix
field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi commented on issue #1394: LUCENE-9300: Fix field infos update on doc values update
URL: https://github.com/apache/lucene-solr/pull/1394#issuecomment-607916720
Thanks @jpountz and @s1monw . I pushed new commits to address your comments.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] jimczi commented on a change in pull request #1394:
LUCENE-9300: Fix field infos update on doc values update
Posted by GitBox <gi...@apache.org>.
jimczi 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_r401672524
##########
File path: lucene/core/src/test/org/apache/lucene/index/TestNumericDocValuesUpdates.java
##########
@@ -1483,6 +1484,81 @@ public void testAddIndexes() throws Exception {
IOUtils.close(dir1, dir2);
}
+ public void testUpdatesAterAddIndexes() throws Exception {
Review comment:
thanks, https://github.com/apache/lucene-solr/pull/1394/commits/abd5e08170cc8b7963c0aff3d58afa5766afcb32
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org