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 2021/03/08 18:25:31 UTC
[GitHub] [lucene-solr] jpountz commented on a change in pull request #2186: LUCENE-9334 Consistency of field data structures
jpountz commented on a change in pull request #2186:
URL: https://github.com/apache/lucene-solr/pull/2186#discussion_r589531266
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfo.java
##########
@@ -111,6 +111,43 @@ public FieldInfo(
/** Performs internal consistency checks. Always returns true (or throws IllegalStateException) */
public boolean checkConsistency() {
+ return checkOptionsCorrectness(
Review comment:
the different name suggests that it's checking something different, I think it would be less confusing to use the same method name since this does the same thing?
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfo.java
##########
@@ -130,127 +167,252 @@ public boolean checkConsistency() {
}
}
- if (pointDimensionCount < 0) {
+ if (docValuesType == null) {
+ throw new IllegalStateException("DocValuesType must not be null (field: '" + name + "')");
+ }
+ if (dvGen != -1 && docValuesType == DocValuesType.NONE) {
throw new IllegalStateException(
- "pointDimensionCount must be >= 0; got " + pointDimensionCount);
+ "field '"
+ + name
+ + "' cannot have a docvalues update generation without having docvalues");
}
+ if (pointDimensionCount < 0) {
+ throw new IllegalStateException(
+ "pointDimensionCount must be >= 0; got "
+ + pointDimensionCount
+ + " (field: '"
+ + name
+ + "')");
+ }
if (pointIndexDimensionCount < 0) {
throw new IllegalStateException(
- "pointIndexDimensionCount must be >= 0; got " + pointIndexDimensionCount);
+ "pointIndexDimensionCount must be >= 0; got "
+ + pointIndexDimensionCount
+ + " (field: '"
+ + name
+ + "')");
}
-
if (pointNumBytes < 0) {
- throw new IllegalStateException("pointNumBytes must be >= 0; got " + pointNumBytes);
+ throw new IllegalStateException(
+ "pointNumBytes must be >= 0; got " + pointNumBytes + " (field: '" + name + "')");
}
if (pointDimensionCount != 0 && pointNumBytes == 0) {
throw new IllegalStateException(
- "pointNumBytes must be > 0 when pointDimensionCount=" + pointDimensionCount);
+ "pointNumBytes must be > 0 when pointDimensionCount="
+ + pointDimensionCount
+ + " (field: '"
+ + name
+ + "')");
}
-
if (pointIndexDimensionCount != 0 && pointDimensionCount == 0) {
throw new IllegalStateException(
- "pointIndexDimensionCount must be 0 when pointDimensionCount=0");
+ "pointIndexDimensionCount must be 0 when pointDimensionCount=0"
+ + " (field: '"
+ + name
+ + "')");
}
-
if (pointNumBytes != 0 && pointDimensionCount == 0) {
throw new IllegalStateException(
- "pointDimensionCount must be > 0 when pointNumBytes=" + pointNumBytes);
+ "pointDimensionCount must be > 0 when pointNumBytes="
+ + pointNumBytes
+ + " (field: '"
+ + name
+ + "')");
}
- if (dvGen != -1 && docValuesType == DocValuesType.NONE) {
+ if (vectorSearchStrategy == null) {
throw new IllegalStateException(
- "field '"
- + name
- + "' cannot have a docvalues update generation without having docvalues");
+ "Vector search strategy must not be null (field: '" + name + "')");
}
-
if (vectorDimension < 0) {
- throw new IllegalStateException("vectorDimension must be >=0; got " + vectorDimension);
+ throw new IllegalStateException(
+ "vectorDimension must be >=0; got " + vectorDimension + " (field: '" + name + "')");
}
-
if (vectorDimension == 0 && vectorSearchStrategy != VectorValues.SearchStrategy.NONE) {
throw new IllegalStateException(
- "vector search strategy must be NONE when dimension = 0; got " + vectorSearchStrategy);
+ "vector search strategy must be NONE when dimension = 0; got "
+ + vectorSearchStrategy
+ + " (field: '"
+ + name
+ + "')");
}
-
return true;
}
- // should only be called by FieldInfos#addOrUpdate
- void update(
- boolean storeTermVector,
- boolean omitNorms,
- boolean storePayloads,
- IndexOptions indexOptions,
- Map<String, String> attributes,
- int dimensionCount,
- int indexDimensionCount,
- int dimensionNumBytes) {
- if (indexOptions == null) {
- throw new NullPointerException("IndexOptions must not be null (field: \"" + name + "\")");
- }
- // System.out.println("FI.update field=" + name + " indexed=" + indexed + " omitNorms=" +
- // omitNorms + " this.omitNorms=" + this.omitNorms);
- if (this.indexOptions != indexOptions) {
- if (this.indexOptions == IndexOptions.NONE) {
- this.indexOptions = indexOptions;
- } else if (indexOptions != IndexOptions.NONE) {
- throw new IllegalArgumentException(
- "cannot change field \""
- + name
- + "\" from index options="
- + this.indexOptions
- + " to inconsistent index options="
- + indexOptions);
- }
+ void verifySameSchema(FieldInfo o, long dvGen) {
+ String fieldName = this.name;
+ verifySameIndexOptions(fieldName, this.indexOptions, o.getIndexOptions());
+ if (this.indexOptions != IndexOptions.NONE) {
+ verifySameOmitNorms(fieldName, this.omitNorms, o.omitNorms);
+ verifySameStoreTermVectors(fieldName, this.storeTermVector, o.storeTermVector);
+ }
+ verifySameDocValuesType(fieldName, this.docValuesType, o.docValuesType);
+ verifySameDVGen(fieldName, this.dvGen, dvGen);
Review comment:
this call is a bit weird as the doc values generation is not part of the schema of a field, can you explain?
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfo.java
##########
@@ -130,127 +167,252 @@ public boolean checkConsistency() {
}
}
- if (pointDimensionCount < 0) {
+ if (docValuesType == null) {
+ throw new IllegalStateException("DocValuesType must not be null (field: '" + name + "')");
+ }
+ if (dvGen != -1 && docValuesType == DocValuesType.NONE) {
throw new IllegalStateException(
- "pointDimensionCount must be >= 0; got " + pointDimensionCount);
+ "field '"
+ + name
+ + "' cannot have a docvalues update generation without having docvalues");
}
+ if (pointDimensionCount < 0) {
+ throw new IllegalStateException(
+ "pointDimensionCount must be >= 0; got "
+ + pointDimensionCount
+ + " (field: '"
+ + name
+ + "')");
+ }
if (pointIndexDimensionCount < 0) {
throw new IllegalStateException(
- "pointIndexDimensionCount must be >= 0; got " + pointIndexDimensionCount);
+ "pointIndexDimensionCount must be >= 0; got "
+ + pointIndexDimensionCount
+ + " (field: '"
+ + name
+ + "')");
}
-
if (pointNumBytes < 0) {
- throw new IllegalStateException("pointNumBytes must be >= 0; got " + pointNumBytes);
+ throw new IllegalStateException(
+ "pointNumBytes must be >= 0; got " + pointNumBytes + " (field: '" + name + "')");
}
if (pointDimensionCount != 0 && pointNumBytes == 0) {
throw new IllegalStateException(
- "pointNumBytes must be > 0 when pointDimensionCount=" + pointDimensionCount);
+ "pointNumBytes must be > 0 when pointDimensionCount="
+ + pointDimensionCount
+ + " (field: '"
+ + name
+ + "')");
}
-
if (pointIndexDimensionCount != 0 && pointDimensionCount == 0) {
throw new IllegalStateException(
- "pointIndexDimensionCount must be 0 when pointDimensionCount=0");
+ "pointIndexDimensionCount must be 0 when pointDimensionCount=0"
+ + " (field: '"
+ + name
+ + "')");
}
-
if (pointNumBytes != 0 && pointDimensionCount == 0) {
throw new IllegalStateException(
- "pointDimensionCount must be > 0 when pointNumBytes=" + pointNumBytes);
+ "pointDimensionCount must be > 0 when pointNumBytes="
+ + pointNumBytes
+ + " (field: '"
+ + name
+ + "')");
}
- if (dvGen != -1 && docValuesType == DocValuesType.NONE) {
+ if (vectorSearchStrategy == null) {
throw new IllegalStateException(
- "field '"
- + name
- + "' cannot have a docvalues update generation without having docvalues");
+ "Vector search strategy must not be null (field: '" + name + "')");
}
-
if (vectorDimension < 0) {
- throw new IllegalStateException("vectorDimension must be >=0; got " + vectorDimension);
+ throw new IllegalStateException(
+ "vectorDimension must be >=0; got " + vectorDimension + " (field: '" + name + "')");
}
-
if (vectorDimension == 0 && vectorSearchStrategy != VectorValues.SearchStrategy.NONE) {
throw new IllegalStateException(
- "vector search strategy must be NONE when dimension = 0; got " + vectorSearchStrategy);
+ "vector search strategy must be NONE when dimension = 0; got "
+ + vectorSearchStrategy
+ + " (field: '"
+ + name
+ + "')");
}
-
return true;
}
- // should only be called by FieldInfos#addOrUpdate
- void update(
- boolean storeTermVector,
- boolean omitNorms,
- boolean storePayloads,
- IndexOptions indexOptions,
- Map<String, String> attributes,
- int dimensionCount,
- int indexDimensionCount,
- int dimensionNumBytes) {
- if (indexOptions == null) {
- throw new NullPointerException("IndexOptions must not be null (field: \"" + name + "\")");
- }
- // System.out.println("FI.update field=" + name + " indexed=" + indexed + " omitNorms=" +
- // omitNorms + " this.omitNorms=" + this.omitNorms);
- if (this.indexOptions != indexOptions) {
- if (this.indexOptions == IndexOptions.NONE) {
- this.indexOptions = indexOptions;
- } else if (indexOptions != IndexOptions.NONE) {
- throw new IllegalArgumentException(
- "cannot change field \""
- + name
- + "\" from index options="
- + this.indexOptions
- + " to inconsistent index options="
- + indexOptions);
- }
+ void verifySameSchema(FieldInfo o, long dvGen) {
Review comment:
Can you add javadocs or comments about the contract of this function?
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -481,205 +433,102 @@ synchronized int addOrGet(
+ fieldName
+ "] as non-soft-deletes already");
}
-
return fieldNumber.intValue();
}
- synchronized void verifyConsistent(Integer number, String name, IndexOptions indexOptions) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- IndexOptions currentIndexOptions = this.indexOptions.get(name);
- if (indexOptions != IndexOptions.NONE
- && currentIndexOptions != null
- && currentIndexOptions != IndexOptions.NONE
- && indexOptions != currentIndexOptions) {
- throw new IllegalArgumentException(
- "cannot change field \""
- + name
- + "\" from index options="
- + currentIndexOptions
- + " to inconsistent index options="
- + indexOptions);
- }
- }
-
- synchronized void verifyConsistent(Integer number, String name, DocValuesType dvType) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- DocValuesType currentDVType = docValuesType.get(name);
- if (dvType != DocValuesType.NONE
- && currentDVType != null
- && currentDVType != DocValuesType.NONE
- && dvType != currentDVType) {
- throw new IllegalArgumentException(
- "cannot change DocValues type from "
- + currentDVType
- + " to "
- + dvType
- + " for field \""
- + name
- + "\"");
- }
- }
-
- synchronized void verifyConsistentDimensions(
- Integer number,
- String name,
- int dataDimensionCount,
+ private void verifySameSchema(
+ String fieldName,
+ IndexOptions indexOptions,
+ boolean storeTermVector,
+ boolean omitNorms,
+ DocValuesType dvType,
+ int dimensionCount,
int indexDimensionCount,
- int dimensionNumBytes) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- FieldDimensions dim = dimensions.get(name);
- if (dim != null) {
- if (dim.dimensionCount != dataDimensionCount) {
- throw new IllegalArgumentException(
- "cannot change point dimension count from "
- + dim.dimensionCount
- + " to "
- + dataDimensionCount
- + " for field=\""
- + name
- + "\"");
- }
- if (dim.indexDimensionCount != indexDimensionCount) {
- throw new IllegalArgumentException(
- "cannot change point index dimension count from "
- + dim.indexDimensionCount
- + " to "
- + indexDimensionCount
- + " for field=\""
- + name
- + "\"");
- }
- if (dim.dimensionNumBytes != dimensionNumBytes) {
- throw new IllegalArgumentException(
- "cannot change point numBytes from "
- + dim.dimensionNumBytes
- + " to "
- + dimensionNumBytes
- + " for field=\""
- + name
- + "\"");
- }
- }
+ int dimensionNumBytes,
+ int vectorDimension,
+ VectorValues.SearchStrategy searchStrategy) {
+
+ IndexOptions currentOpts = this.indexOptions.get(fieldName);
+ verifySameIndexOptions(fieldName, currentOpts, indexOptions);
+ if (currentOpts != IndexOptions.NONE) {
+ boolean curStoreTermVector = this.storeTermVectors.get(fieldName);
+ verifySameStoreTermVectors(fieldName, curStoreTermVector, storeTermVector);
+ boolean curOmitNorms = this.omitNorms.get(fieldName);
+ verifySameOmitNorms(fieldName, curOmitNorms, omitNorms);
+ }
+
+ DocValuesType currentDVType = docValuesType.get(fieldName);
+ verifySameDocValuesType(fieldName, currentDVType, dvType);
+
+ FieldDimensions dims = dimensions.get(fieldName);
+ verifySamePointsOptions(
+ fieldName,
+ dims.dimensionCount,
+ dims.indexDimensionCount,
+ dims.dimensionNumBytes,
+ dimensionCount,
+ indexDimensionCount,
+ dimensionNumBytes);
+
+ FieldVectorProperties props = vectorProps.get(fieldName);
+ verifySameVectorOptions(
+ fieldName, props.numDimensions, props.searchStrategy, vectorDimension, searchStrategy);
}
- synchronized void verifyConsistentVectorProperties(
- Integer number,
- String name,
- int numDimensions,
- VectorValues.SearchStrategy searchStrategy) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- FieldVectorProperties props = vectorProps.get(name);
- if (props != null) {
- if (props.numDimensions != numDimensions) {
- throw new IllegalArgumentException(
- "cannot change vector dimension from "
- + props.numDimensions
- + " to "
- + numDimensions
- + " for field=\""
- + name
- + "\"");
- }
- if (props.searchStrategy != searchStrategy) {
- throw new IllegalArgumentException(
- "cannot change vector search strategy from "
- + props.searchStrategy
- + " to "
- + searchStrategy
- + " for field=\""
- + name
- + "\"");
- }
- }
+ /**
+ * Returns true if the {@code fieldName} exists in the map and is of the same {@code dvType},
+ * and it is docValues only field.
+ */
+ synchronized boolean containsDvOnlyField(String fieldName, DocValuesType dvType) {
+ if (nameToNumber.containsKey(fieldName) == false) return false;
+ if (dvType != docValuesType.get(fieldName)) return false;
+ FieldDimensions fdimensions = dimensions.get(fieldName);
+ if (fdimensions != null && fdimensions.dimensionCount != 0) return false;
+ IndexOptions ioptions = indexOptions.get(fieldName);
+ if (ioptions != null && ioptions != IndexOptions.NONE) return false;
+ FieldVectorProperties fvp = vectorProps.get(fieldName);
+ if (fvp != null && fvp.numDimensions != 0) return false;
+ return true;
}
/**
- * Returns true if the {@code fieldName} exists in the map and is of the same {@code dvType}.
+ * Construct a new FieldInfo based on the options in global field numbers This method is not
+ * synchronized as all the options it uses are not modifiable.
+ *
+ * @param fieldName name of the field
+ * @param dvType doc values type
+ * @param newFieldNumber a new field number
+ * @return {@code null} if {@code fieldName} doesn't exist in the map or is not of the same
+ * {@code dvType} returns a new FieldInfo based based on the options in global field numbers
*/
- synchronized boolean contains(String fieldName, DocValuesType dvType) {
- // used by IndexWriter.updateNumericDocValue
- if (!nameToNumber.containsKey(fieldName)) {
- return false;
- } else {
- // only return true if the field has the same dvType as the requested one
- return dvType == docValuesType.get(fieldName);
- }
+ FieldInfo constructFieldInfo(String fieldName, DocValuesType dvType, int newFieldNumber) {
+ Integer fieldNumber;
+ synchronized (this) {
+ fieldNumber = nameToNumber.get(fieldName);
+ }
+ if (fieldNumber == null) return null;
+ DocValuesType dvType0 = docValuesType.get(fieldName);
+ if (dvType != dvType0) return null;
+
+ boolean isSoftDeletesField = fieldName.equals(softDeletesFieldName);
+ FieldDimensions dims = dimensions.get(fieldName);
+ FieldVectorProperties vectors = vectorProps.get(fieldName);
Review comment:
shouldn't it always be null in practice, as this looks used for doc-value updates? (Or am I missing something?
same question for dimensions
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfo.java
##########
@@ -130,127 +167,252 @@ public boolean checkConsistency() {
}
}
- if (pointDimensionCount < 0) {
+ if (docValuesType == null) {
+ throw new IllegalStateException("DocValuesType must not be null (field: '" + name + "')");
+ }
+ if (dvGen != -1 && docValuesType == DocValuesType.NONE) {
throw new IllegalStateException(
- "pointDimensionCount must be >= 0; got " + pointDimensionCount);
+ "field '"
+ + name
+ + "' cannot have a docvalues update generation without having docvalues");
}
+ if (pointDimensionCount < 0) {
+ throw new IllegalStateException(
+ "pointDimensionCount must be >= 0; got "
+ + pointDimensionCount
+ + " (field: '"
+ + name
+ + "')");
+ }
if (pointIndexDimensionCount < 0) {
throw new IllegalStateException(
- "pointIndexDimensionCount must be >= 0; got " + pointIndexDimensionCount);
+ "pointIndexDimensionCount must be >= 0; got "
+ + pointIndexDimensionCount
+ + " (field: '"
+ + name
+ + "')");
}
-
if (pointNumBytes < 0) {
- throw new IllegalStateException("pointNumBytes must be >= 0; got " + pointNumBytes);
+ throw new IllegalStateException(
+ "pointNumBytes must be >= 0; got " + pointNumBytes + " (field: '" + name + "')");
}
if (pointDimensionCount != 0 && pointNumBytes == 0) {
throw new IllegalStateException(
- "pointNumBytes must be > 0 when pointDimensionCount=" + pointDimensionCount);
+ "pointNumBytes must be > 0 when pointDimensionCount="
+ + pointDimensionCount
+ + " (field: '"
+ + name
+ + "')");
}
-
if (pointIndexDimensionCount != 0 && pointDimensionCount == 0) {
throw new IllegalStateException(
- "pointIndexDimensionCount must be 0 when pointDimensionCount=0");
+ "pointIndexDimensionCount must be 0 when pointDimensionCount=0"
+ + " (field: '"
+ + name
+ + "')");
}
-
if (pointNumBytes != 0 && pointDimensionCount == 0) {
throw new IllegalStateException(
- "pointDimensionCount must be > 0 when pointNumBytes=" + pointNumBytes);
+ "pointDimensionCount must be > 0 when pointNumBytes="
+ + pointNumBytes
+ + " (field: '"
+ + name
+ + "')");
}
- if (dvGen != -1 && docValuesType == DocValuesType.NONE) {
+ if (vectorSearchStrategy == null) {
throw new IllegalStateException(
- "field '"
- + name
- + "' cannot have a docvalues update generation without having docvalues");
+ "Vector search strategy must not be null (field: '" + name + "')");
}
-
if (vectorDimension < 0) {
- throw new IllegalStateException("vectorDimension must be >=0; got " + vectorDimension);
+ throw new IllegalStateException(
+ "vectorDimension must be >=0; got " + vectorDimension + " (field: '" + name + "')");
}
-
if (vectorDimension == 0 && vectorSearchStrategy != VectorValues.SearchStrategy.NONE) {
throw new IllegalStateException(
- "vector search strategy must be NONE when dimension = 0; got " + vectorSearchStrategy);
+ "vector search strategy must be NONE when dimension = 0; got "
+ + vectorSearchStrategy
+ + " (field: '"
+ + name
+ + "')");
}
-
return true;
}
- // should only be called by FieldInfos#addOrUpdate
- void update(
- boolean storeTermVector,
- boolean omitNorms,
- boolean storePayloads,
- IndexOptions indexOptions,
- Map<String, String> attributes,
- int dimensionCount,
- int indexDimensionCount,
- int dimensionNumBytes) {
- if (indexOptions == null) {
- throw new NullPointerException("IndexOptions must not be null (field: \"" + name + "\")");
- }
- // System.out.println("FI.update field=" + name + " indexed=" + indexed + " omitNorms=" +
- // omitNorms + " this.omitNorms=" + this.omitNorms);
- if (this.indexOptions != indexOptions) {
- if (this.indexOptions == IndexOptions.NONE) {
- this.indexOptions = indexOptions;
- } else if (indexOptions != IndexOptions.NONE) {
- throw new IllegalArgumentException(
- "cannot change field \""
- + name
- + "\" from index options="
- + this.indexOptions
- + " to inconsistent index options="
- + indexOptions);
- }
+ void verifySameSchema(FieldInfo o, long dvGen) {
+ String fieldName = this.name;
+ verifySameIndexOptions(fieldName, this.indexOptions, o.getIndexOptions());
+ if (this.indexOptions != IndexOptions.NONE) {
+ verifySameOmitNorms(fieldName, this.omitNorms, o.omitNorms);
+ verifySameStoreTermVectors(fieldName, this.storeTermVector, o.storeTermVector);
+ }
+ verifySameDocValuesType(fieldName, this.docValuesType, o.docValuesType);
+ verifySameDVGen(fieldName, this.dvGen, dvGen);
+ verifySamePointsOptions(
+ fieldName,
+ this.pointDimensionCount,
+ this.pointIndexDimensionCount,
+ this.pointNumBytes,
+ o.pointDimensionCount,
+ o.pointIndexDimensionCount,
+ o.pointNumBytes);
+ verifySameVectorOptions(
+ fieldName,
+ this.vectorDimension,
+ this.vectorSearchStrategy,
+ o.vectorDimension,
+ o.vectorSearchStrategy);
+ }
+
+ /**
+ * Verify that the provided index options are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameIndexOptions(
Review comment:
Does it need to be public or could we make it pkg-private? (likewise for the similar functions that this class holds)
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
##########
@@ -1860,8 +1862,8 @@ public long softUpdateDocument(
*/
public long updateNumericDocValue(Term term, String field, long value) throws IOException {
ensureOpen();
- if (!globalFieldNumberMap.contains(field, DocValuesType.NUMERIC)) {
- throw new IllegalArgumentException("can only update existing numeric-docvalues fields!");
+ if (globalFieldNumberMap.containsDvOnlyField(field, DocValuesType.NUMERIC) == false) {
+ throw new IllegalArgumentException("can only update existing numeric docvalues only fields!");
}
Review comment:
I wonder if we should refactor this method from returning a boolean to throwing the exception so that it could tell what feature exactly the field has that it shouldn't have.
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -972,6 +671,64 @@ private boolean assertNotFinished() {
return true;
}
+ private FieldInfo addField(
Review comment:
should it be called just `add` for consistency since other methods are just called `add` or `getOrAdd`?
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexingChain.java
##########
@@ -581,37 +584,59 @@ private void finishStoredFields() throws IOException {
}
void processDocument(int docID, Iterable<? extends IndexableField> document) throws IOException {
-
- // How many indexed field names we've seen (collapses
- // multiple field instances by the same name):
- int fieldCount = 0;
-
+ int fieldCount =
+ 0; // How many field names we've seen (collapses multiple field instances by the same name)
+ int indexedFieldCount = 0; // number of unique fields indexed with postings
long fieldGen = nextFieldGen++;
-
- // NOTE: we need two passes here, in case there are
- // multi-valued fields, because we must process all
- // instances of a given field at once, since the
- // analyzer is free to reuse TokenStream across fields
- // (i.e., we cannot have more than one TokenStream
- // running "at once"):
Review comment:
is this comment no longer applicable?
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexingChain.java
##########
@@ -581,37 +584,59 @@ private void finishStoredFields() throws IOException {
}
void processDocument(int docID, Iterable<? extends IndexableField> document) throws IOException {
-
- // How many indexed field names we've seen (collapses
- // multiple field instances by the same name):
- int fieldCount = 0;
-
+ int fieldCount =
+ 0; // How many field names we've seen (collapses multiple field instances by the same name)
Review comment:
maybe put the comment before the variable declaration again so that it doesn't affect indentation?
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexingChain.java
##########
@@ -79,6 +79,7 @@
// Holds fields seen in each document
private PerField[] fields = new PerField[1];
+ private PerField[] docFields = new PerField[10];
Review comment:
should we set a smaller initial size to make sure that we exercise the growing logic in our tests?
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -481,205 +433,102 @@ synchronized int addOrGet(
+ fieldName
+ "] as non-soft-deletes already");
}
-
return fieldNumber.intValue();
}
- synchronized void verifyConsistent(Integer number, String name, IndexOptions indexOptions) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- IndexOptions currentIndexOptions = this.indexOptions.get(name);
- if (indexOptions != IndexOptions.NONE
- && currentIndexOptions != null
- && currentIndexOptions != IndexOptions.NONE
- && indexOptions != currentIndexOptions) {
- throw new IllegalArgumentException(
- "cannot change field \""
- + name
- + "\" from index options="
- + currentIndexOptions
- + " to inconsistent index options="
- + indexOptions);
- }
- }
-
- synchronized void verifyConsistent(Integer number, String name, DocValuesType dvType) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- DocValuesType currentDVType = docValuesType.get(name);
- if (dvType != DocValuesType.NONE
- && currentDVType != null
- && currentDVType != DocValuesType.NONE
- && dvType != currentDVType) {
- throw new IllegalArgumentException(
- "cannot change DocValues type from "
- + currentDVType
- + " to "
- + dvType
- + " for field \""
- + name
- + "\"");
- }
- }
-
- synchronized void verifyConsistentDimensions(
- Integer number,
- String name,
- int dataDimensionCount,
+ private void verifySameSchema(
+ String fieldName,
+ IndexOptions indexOptions,
+ boolean storeTermVector,
+ boolean omitNorms,
+ DocValuesType dvType,
+ int dimensionCount,
int indexDimensionCount,
- int dimensionNumBytes) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- FieldDimensions dim = dimensions.get(name);
- if (dim != null) {
- if (dim.dimensionCount != dataDimensionCount) {
- throw new IllegalArgumentException(
- "cannot change point dimension count from "
- + dim.dimensionCount
- + " to "
- + dataDimensionCount
- + " for field=\""
- + name
- + "\"");
- }
- if (dim.indexDimensionCount != indexDimensionCount) {
- throw new IllegalArgumentException(
- "cannot change point index dimension count from "
- + dim.indexDimensionCount
- + " to "
- + indexDimensionCount
- + " for field=\""
- + name
- + "\"");
- }
- if (dim.dimensionNumBytes != dimensionNumBytes) {
- throw new IllegalArgumentException(
- "cannot change point numBytes from "
- + dim.dimensionNumBytes
- + " to "
- + dimensionNumBytes
- + " for field=\""
- + name
- + "\"");
- }
- }
+ int dimensionNumBytes,
+ int vectorDimension,
+ VectorValues.SearchStrategy searchStrategy) {
+
+ IndexOptions currentOpts = this.indexOptions.get(fieldName);
+ verifySameIndexOptions(fieldName, currentOpts, indexOptions);
+ if (currentOpts != IndexOptions.NONE) {
+ boolean curStoreTermVector = this.storeTermVectors.get(fieldName);
+ verifySameStoreTermVectors(fieldName, curStoreTermVector, storeTermVector);
+ boolean curOmitNorms = this.omitNorms.get(fieldName);
+ verifySameOmitNorms(fieldName, curOmitNorms, omitNorms);
+ }
+
+ DocValuesType currentDVType = docValuesType.get(fieldName);
+ verifySameDocValuesType(fieldName, currentDVType, dvType);
+
+ FieldDimensions dims = dimensions.get(fieldName);
+ verifySamePointsOptions(
+ fieldName,
+ dims.dimensionCount,
+ dims.indexDimensionCount,
+ dims.dimensionNumBytes,
+ dimensionCount,
+ indexDimensionCount,
+ dimensionNumBytes);
+
+ FieldVectorProperties props = vectorProps.get(fieldName);
+ verifySameVectorOptions(
+ fieldName, props.numDimensions, props.searchStrategy, vectorDimension, searchStrategy);
}
- synchronized void verifyConsistentVectorProperties(
- Integer number,
- String name,
- int numDimensions,
- VectorValues.SearchStrategy searchStrategy) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- FieldVectorProperties props = vectorProps.get(name);
- if (props != null) {
- if (props.numDimensions != numDimensions) {
- throw new IllegalArgumentException(
- "cannot change vector dimension from "
- + props.numDimensions
- + " to "
- + numDimensions
- + " for field=\""
- + name
- + "\"");
- }
- if (props.searchStrategy != searchStrategy) {
- throw new IllegalArgumentException(
- "cannot change vector search strategy from "
- + props.searchStrategy
- + " to "
- + searchStrategy
- + " for field=\""
- + name
- + "\"");
- }
- }
+ /**
+ * Returns true if the {@code fieldName} exists in the map and is of the same {@code dvType},
+ * and it is docValues only field.
+ */
+ synchronized boolean containsDvOnlyField(String fieldName, DocValuesType dvType) {
+ if (nameToNumber.containsKey(fieldName) == false) return false;
+ if (dvType != docValuesType.get(fieldName)) return false;
+ FieldDimensions fdimensions = dimensions.get(fieldName);
+ if (fdimensions != null && fdimensions.dimensionCount != 0) return false;
+ IndexOptions ioptions = indexOptions.get(fieldName);
+ if (ioptions != null && ioptions != IndexOptions.NONE) return false;
+ FieldVectorProperties fvp = vectorProps.get(fieldName);
+ if (fvp != null && fvp.numDimensions != 0) return false;
+ return true;
}
/**
- * Returns true if the {@code fieldName} exists in the map and is of the same {@code dvType}.
+ * Construct a new FieldInfo based on the options in global field numbers This method is not
+ * synchronized as all the options it uses are not modifiable.
Review comment:
is it correct? it looks to me like `docValuesType` could be modified by another thread concurrently to the current thread reading from it through this method?
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -455,8 +373,42 @@ synchronized int addOrGet(
fieldNumber = lowestUnassignedFieldNumber;
}
assert fieldNumber >= 0;
+ FieldInfo.checkOptionsCorrectness(
+ fieldName,
+ false,
Review comment:
why are we not passing `storeTermVector` here?
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -481,205 +433,102 @@ synchronized int addOrGet(
+ fieldName
+ "] as non-soft-deletes already");
}
-
return fieldNumber.intValue();
}
- synchronized void verifyConsistent(Integer number, String name, IndexOptions indexOptions) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- IndexOptions currentIndexOptions = this.indexOptions.get(name);
- if (indexOptions != IndexOptions.NONE
- && currentIndexOptions != null
- && currentIndexOptions != IndexOptions.NONE
- && indexOptions != currentIndexOptions) {
- throw new IllegalArgumentException(
- "cannot change field \""
- + name
- + "\" from index options="
- + currentIndexOptions
- + " to inconsistent index options="
- + indexOptions);
- }
- }
-
- synchronized void verifyConsistent(Integer number, String name, DocValuesType dvType) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- DocValuesType currentDVType = docValuesType.get(name);
- if (dvType != DocValuesType.NONE
- && currentDVType != null
- && currentDVType != DocValuesType.NONE
- && dvType != currentDVType) {
- throw new IllegalArgumentException(
- "cannot change DocValues type from "
- + currentDVType
- + " to "
- + dvType
- + " for field \""
- + name
- + "\"");
- }
- }
-
- synchronized void verifyConsistentDimensions(
- Integer number,
- String name,
- int dataDimensionCount,
+ private void verifySameSchema(
+ String fieldName,
+ IndexOptions indexOptions,
+ boolean storeTermVector,
+ boolean omitNorms,
+ DocValuesType dvType,
+ int dimensionCount,
int indexDimensionCount,
- int dimensionNumBytes) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- FieldDimensions dim = dimensions.get(name);
- if (dim != null) {
- if (dim.dimensionCount != dataDimensionCount) {
- throw new IllegalArgumentException(
- "cannot change point dimension count from "
- + dim.dimensionCount
- + " to "
- + dataDimensionCount
- + " for field=\""
- + name
- + "\"");
- }
- if (dim.indexDimensionCount != indexDimensionCount) {
- throw new IllegalArgumentException(
- "cannot change point index dimension count from "
- + dim.indexDimensionCount
- + " to "
- + indexDimensionCount
- + " for field=\""
- + name
- + "\"");
- }
- if (dim.dimensionNumBytes != dimensionNumBytes) {
- throw new IllegalArgumentException(
- "cannot change point numBytes from "
- + dim.dimensionNumBytes
- + " to "
- + dimensionNumBytes
- + " for field=\""
- + name
- + "\"");
- }
- }
+ int dimensionNumBytes,
+ int vectorDimension,
+ VectorValues.SearchStrategy searchStrategy) {
+
+ IndexOptions currentOpts = this.indexOptions.get(fieldName);
+ verifySameIndexOptions(fieldName, currentOpts, indexOptions);
+ if (currentOpts != IndexOptions.NONE) {
+ boolean curStoreTermVector = this.storeTermVectors.get(fieldName);
+ verifySameStoreTermVectors(fieldName, curStoreTermVector, storeTermVector);
+ boolean curOmitNorms = this.omitNorms.get(fieldName);
+ verifySameOmitNorms(fieldName, curOmitNorms, omitNorms);
+ }
+
+ DocValuesType currentDVType = docValuesType.get(fieldName);
+ verifySameDocValuesType(fieldName, currentDVType, dvType);
+
+ FieldDimensions dims = dimensions.get(fieldName);
+ verifySamePointsOptions(
+ fieldName,
+ dims.dimensionCount,
+ dims.indexDimensionCount,
+ dims.dimensionNumBytes,
+ dimensionCount,
+ indexDimensionCount,
+ dimensionNumBytes);
+
+ FieldVectorProperties props = vectorProps.get(fieldName);
+ verifySameVectorOptions(
+ fieldName, props.numDimensions, props.searchStrategy, vectorDimension, searchStrategy);
}
- synchronized void verifyConsistentVectorProperties(
- Integer number,
- String name,
- int numDimensions,
- VectorValues.SearchStrategy searchStrategy) {
- if (name.equals(numberToName.get(number)) == false) {
- throw new IllegalArgumentException(
- "field number "
- + number
- + " is already mapped to field name \""
- + numberToName.get(number)
- + "\", not \""
- + name
- + "\"");
- }
- if (number.equals(nameToNumber.get(name)) == false) {
- throw new IllegalArgumentException(
- "field name \""
- + name
- + "\" is already mapped to field number \""
- + nameToNumber.get(name)
- + "\", not \""
- + number
- + "\"");
- }
- FieldVectorProperties props = vectorProps.get(name);
- if (props != null) {
- if (props.numDimensions != numDimensions) {
- throw new IllegalArgumentException(
- "cannot change vector dimension from "
- + props.numDimensions
- + " to "
- + numDimensions
- + " for field=\""
- + name
- + "\"");
- }
- if (props.searchStrategy != searchStrategy) {
- throw new IllegalArgumentException(
- "cannot change vector search strategy from "
- + props.searchStrategy
- + " to "
- + searchStrategy
- + " for field=\""
- + name
- + "\"");
- }
- }
+ /**
+ * Returns true if the {@code fieldName} exists in the map and is of the same {@code dvType},
+ * and it is docValues only field.
Review comment:
I think we could use more docs here, e.g. explaining that this is called from IndexWriter to know whether a field may be updated through doc values?
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -796,167 +565,97 @@ public void add(FieldInfos other) {
/** Create a new field, or return existing one. */
public FieldInfo getOrAdd(String name) {
FieldInfo fi = fieldInfo(name);
- if (fi == null) {
- assert assertNotFinished();
- // This field wasn't yet added to this in-RAM
- // segment's FieldInfo, so now we get a global
- // number for this field. If the field was seen
- // before then we'll get the same name and number,
- // else we'll allocate a new one:
- final boolean isSoftDeletesField = name.equals(globalFieldNumbers.softDeletesFieldName);
- final int fieldNumber =
- globalFieldNumbers.addOrGet(
- name,
- -1,
- IndexOptions.NONE,
- DocValuesType.NONE,
- 0,
- 0,
- 0,
- 0,
- VectorValues.SearchStrategy.NONE,
- isSoftDeletesField);
- fi =
- new FieldInfo(
- name,
- fieldNumber,
- false,
- false,
- false,
- IndexOptions.NONE,
- DocValuesType.NONE,
- -1,
- new HashMap<>(),
- 0,
- 0,
- 0,
- 0,
- VectorValues.SearchStrategy.NONE,
- isSoftDeletesField);
- assert !byName.containsKey(fi.name);
- globalFieldNumbers.verifyConsistent(
- Integer.valueOf(fi.number), fi.name, DocValuesType.NONE);
- byName.put(fi.name, fi);
+ if (fi != null) {
+ return fi;
+ } else {
+ return addField(
+ name,
+ -1,
+ false,
+ false,
+ false,
+ IndexOptions.NONE,
+ DocValuesType.NONE,
+ -1,
+ new HashMap<>(),
+ 0,
+ 0,
+ 0,
+ 0,
+ VectorValues.SearchStrategy.NONE,
+ name.equals(globalFieldNumbers.softDeletesFieldName));
}
+ }
- return fi;
+ public FieldInfo add(FieldInfo fi) {
+ return add(fi, -1);
+ }
+
+ public FieldInfo add(FieldInfo fi, long dvGen) {
+ // IMPORTANT - reuse the field number if possible for consistent field numbers across segments
+ if (fi.getDocValuesType() == null) {
+ throw new NullPointerException("DocValuesType must not be null");
+ }
+ final FieldInfo curFi = fieldInfo(fi.name);
+ if (curFi == null) {
+ // original attributes is UnmodifiableMap
+ Map<String, String> attributes =
+ fi.attributes() == null ? null : new HashMap<>(fi.attributes());
+ return addField(
+ fi.name,
+ fi.number,
+ fi.hasVectors(),
+ fi.omitsNorms(),
+ fi.hasPayloads(),
+ fi.getIndexOptions(),
+ fi.getDocValuesType(),
+ dvGen,
+ attributes,
+ fi.getPointDimensionCount(),
+ fi.getPointIndexDimensionCount(),
+ fi.getPointNumBytes(),
+ fi.getVectorDimension(),
+ fi.getVectorSearchStrategy(),
+ fi.isSoftDeletesField());
+ } else {
+ curFi.verifySameSchema(fi, dvGen);
+ if (fi.attributes() != null) {
+ fi.attributes().forEach((k, v) -> curFi.putAttribute(k, v));
+ }
+ return curFi;
+ }
}
- private FieldInfo addOrUpdateInternal(
+ public FieldInfo add(
Review comment:
Can you add javadocs since it's public?
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexingChain.java
##########
@@ -399,7 +401,8 @@ private void writeDocValues(SegmentWriteState state, Sorter.DocMap sortMap) thro
}
perField.docValuesWriter.flush(state, sortMap, dvConsumer);
perField.docValuesWriter = null;
- } else if (perField.fieldInfo.getDocValuesType() != DocValuesType.NONE) {
+ } else if (perField.fieldInfo != null
Review comment:
shouldn't fieldInfo always be non null when reaching here?
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexingChain.java
##########
@@ -349,7 +350,8 @@ private void writePoints(SegmentWriteState state, Sorter.DocMap sortMap) throws
perField.pointValuesWriter.flush(state, sortMap, pointsWriter);
perField.pointValuesWriter = null;
- } else if (perField.fieldInfo.getPointDimensionCount() != 0) {
+ } else if (perField.fieldInfo != null
Review comment:
shouldn't fieldInfo always be non null when reaching here?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org