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/12 10:17:12 UTC
[GitHub] [lucene] s1monw commented on a change in pull request #11: LUCENE-9334 Consistency of field data structures
s1monw commented on a change in pull request #11:
URL: https://github.com/apache/lucene/pull/11#discussion_r593002781
##########
File path: lucene/MIGRATE.md
##########
@@ -358,8 +358,10 @@ Rather, an IllegalArgumentException shall be thrown. This is introduced for bett
defence and to ensure that there is no bubbling up of errors when Lucene is
used in multi level applications
-## Assumption of data consistency between different data-structures sharing the same field name
+### Require consistency between data-structures on a per-field basis
-Sorting on a numeric field that is indexed with both doc values and points may use an
-optimization to skip non-competitive documents. This optimization relies on the assumption
-that the same data is stored in these points and doc values.
+A field must be indexed with the same index options and data-structures across
+all documents within an index. Thus, for example, it is not allowed to have
+one document in a index where a certain field is indexed with doc values
+and points, and another document where the same field is indexed only with
+points.
Review comment:
would it make sense to say something like "the per field data-structures are implicitly defined by the first document indexed that contains a certain field"?
##########
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 + "')");
Review comment:
same here? IAE?
##########
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);
Review comment:
do we need reformat this? Can it be on the same line?
##########
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(
+ String fieldName, IndexOptions indexOptions1, IndexOptions indexOptions2) {
+ if (indexOptions1 != indexOptions2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from index options="
+ + indexOptions1
+ + " to inconsistent index options="
+ + indexOptions2);
}
+ }
- if (this.pointDimensionCount == 0 && dimensionCount != 0) {
- this.pointDimensionCount = dimensionCount;
- this.pointIndexDimensionCount = indexDimensionCount;
- this.pointNumBytes = dimensionNumBytes;
- } else if (dimensionCount != 0
- && (this.pointDimensionCount != dimensionCount
- || this.pointIndexDimensionCount != indexDimensionCount
- || this.pointNumBytes != dimensionNumBytes)) {
+ /**
+ * Verify that the provided docValues type are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameDocValuesType(
+ String fieldName, DocValuesType docValuesType1, DocValuesType docValuesType2) {
+ if (docValuesType1 != docValuesType2) {
throw new IllegalArgumentException(
"cannot change field \""
- + name
+ + fieldName
+ + "\" from doc values type="
+ + docValuesType1
+ + " to inconsistent doc values type="
+ + docValuesType2);
+ }
+ }
+
+ /**
+ * Verify that the provided doc values generations are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ // TODO: not sure if gen also must be the same
+ public static void verifySameDVGen(String fieldName, long docValuesGen1, long docValuesGen2) {
Review comment:
can be pkg private?
##########
File path: lucene/core/src/java/org/apache/lucene/index/TermVectorsConsumerPerField.java
##########
@@ -149,9 +149,6 @@ boolean start(IndexableField field, boolean first) {
doVectors = field.fieldType().storeTermVectors();
if (doVectors) {
-
- termsWriter.hasVectors = true;
Review comment:
we don't set this here anymore? why not?
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -339,110 +350,17 @@ synchronized int addOrGet(
String fieldName,
int preferredFieldNumber,
IndexOptions indexOptions,
+ boolean storeTermVector,
Review comment:
we duplicate the args list from FieldInfo across many files. I wonder if we should rather go and pay the price of constructing more than one FieldInfo object in some cases and trade this for calling a simpler method in many places. For instance everywhere where we pass all these args we can pass a FieldInfo object instead. In this method we can modify it if we can't assign the preferred FN or if it's `-1` WDYT?
##########
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(
+ String fieldName, IndexOptions indexOptions1, IndexOptions indexOptions2) {
+ if (indexOptions1 != indexOptions2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from index options="
+ + indexOptions1
+ + " to inconsistent index options="
+ + indexOptions2);
}
+ }
- if (this.pointDimensionCount == 0 && dimensionCount != 0) {
- this.pointDimensionCount = dimensionCount;
- this.pointIndexDimensionCount = indexDimensionCount;
- this.pointNumBytes = dimensionNumBytes;
- } else if (dimensionCount != 0
- && (this.pointDimensionCount != dimensionCount
- || this.pointIndexDimensionCount != indexDimensionCount
- || this.pointNumBytes != dimensionNumBytes)) {
+ /**
+ * Verify that the provided docValues type are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameDocValuesType(
+ String fieldName, DocValuesType docValuesType1, DocValuesType docValuesType2) {
+ if (docValuesType1 != docValuesType2) {
throw new IllegalArgumentException(
"cannot change field \""
- + name
+ + fieldName
+ + "\" from doc values type="
+ + docValuesType1
+ + " to inconsistent doc values type="
+ + docValuesType2);
+ }
+ }
+
+ /**
+ * Verify that the provided doc values generations are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ // TODO: not sure if gen also must be the same
+ public static void verifySameDVGen(String fieldName, long docValuesGen1, long docValuesGen2) {
+ if (docValuesGen1 != docValuesGen2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from doc values generation="
+ + docValuesGen1
+ + " to inconsistent doc values generation="
+ + docValuesGen2);
+ }
+ }
+
+ /**
+ * Verify that the provided store term vectors options are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameStoreTermVectors(
Review comment:
can this be pkg protected?
##########
File path: lucene/CHANGES.txt
##########
@@ -92,6 +92,12 @@ API Changes
* LUCENE-9480: Make DataInput's skipBytes(long) abstract as the implementation was not performant.
IndexInput's api is unaffected: skipBytes() is implemented via seek(). (Greg Miller)
+* LUCENE-9334: Require consistency between data-structures on a per-field basis.
+ A field across all documents within an index must be indexed with the same index
+ options and data-structures. As a consequence of this, doc values updates are
+ only applicable for fields that are indexed with doc values only
Review comment:
this needs your name on it in parentheses ;)
##########
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(
+ name,
+ storeTermVector,
+ omitNorms,
+ storePayloads,
+ indexOptions,
+ docValuesType,
+ dvGen,
+ pointDimensionCount,
+ pointIndexDimensionCount,
+ pointNumBytes,
+ vectorDimension,
+ vectorSearchStrategy);
+ }
+
+ /**
+ * Check correctness of FieldInfo options
+ *
+ * @throws IllegalStateException if some options are incorrect
+ * @return {@code true} if all options are correct
+ */
+ public static boolean checkOptionsCorrectness(
+ String name,
Review comment:
is it required to wrap around every argument? I'd prefer to not do that it's hard to read IMO
##########
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(
+ String fieldName, IndexOptions indexOptions1, IndexOptions indexOptions2) {
+ if (indexOptions1 != indexOptions2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from index options="
+ + indexOptions1
+ + " to inconsistent index options="
+ + indexOptions2);
}
+ }
- if (this.pointDimensionCount == 0 && dimensionCount != 0) {
- this.pointDimensionCount = dimensionCount;
- this.pointIndexDimensionCount = indexDimensionCount;
- this.pointNumBytes = dimensionNumBytes;
- } else if (dimensionCount != 0
- && (this.pointDimensionCount != dimensionCount
- || this.pointIndexDimensionCount != indexDimensionCount
- || this.pointNumBytes != dimensionNumBytes)) {
+ /**
+ * Verify that the provided docValues type are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameDocValuesType(
+ String fieldName, DocValuesType docValuesType1, DocValuesType docValuesType2) {
+ if (docValuesType1 != docValuesType2) {
throw new IllegalArgumentException(
"cannot change field \""
- + name
+ + fieldName
+ + "\" from doc values type="
+ + docValuesType1
+ + " to inconsistent doc values type="
+ + docValuesType2);
+ }
+ }
+
+ /**
+ * Verify that the provided doc values generations are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ // TODO: not sure if gen also must be the same
+ public static void verifySameDVGen(String fieldName, long docValuesGen1, long docValuesGen2) {
+ if (docValuesGen1 != docValuesGen2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from doc values generation="
+ + docValuesGen1
+ + " to inconsistent doc values generation="
+ + docValuesGen2);
+ }
+ }
+
+ /**
+ * Verify that the provided store term vectors options are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameStoreTermVectors(
+ String fieldName, boolean storeTermVector1, boolean storeTermVector2) {
+ if (storeTermVector1 != storeTermVector2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from storeTermVector="
+ + storeTermVector1
+ + " to inconsistent storeTermVector="
+ + storeTermVector2);
+ }
+ }
+
+ /**
+ * Verify that the provided omitNorms are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameOmitNorms(String fieldName, boolean omitNorms1, boolean omitNorms2) {
+ if (omitNorms1 != omitNorms2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from omitNorms="
+ + omitNorms1
+ + " to inconsistent omitNorms="
+ + omitNorms2);
+ }
+ }
+
+ /**
+ * Verify that the provided points indexing options are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySamePointsOptions(
Review comment:
can be pkg private?
##########
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(
+ String fieldName, IndexOptions indexOptions1, IndexOptions indexOptions2) {
+ if (indexOptions1 != indexOptions2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from index options="
+ + indexOptions1
+ + " to inconsistent index options="
+ + indexOptions2);
}
+ }
- if (this.pointDimensionCount == 0 && dimensionCount != 0) {
- this.pointDimensionCount = dimensionCount;
- this.pointIndexDimensionCount = indexDimensionCount;
- this.pointNumBytes = dimensionNumBytes;
- } else if (dimensionCount != 0
- && (this.pointDimensionCount != dimensionCount
- || this.pointIndexDimensionCount != indexDimensionCount
- || this.pointNumBytes != dimensionNumBytes)) {
+ /**
+ * Verify that the provided docValues type are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameDocValuesType(
Review comment:
can be pkg private?
##########
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(
+ name,
+ storeTermVector,
+ omitNorms,
+ storePayloads,
+ indexOptions,
+ docValuesType,
+ dvGen,
+ pointDimensionCount,
+ pointIndexDimensionCount,
+ pointNumBytes,
+ vectorDimension,
+ vectorSearchStrategy);
+ }
+
+ /**
+ * Check correctness of FieldInfo options
+ *
+ * @throws IllegalStateException if some options are incorrect
+ * @return {@code true} if all options are correct
+ */
+ public static boolean checkOptionsCorrectness(
+ String name,
+ boolean storeTermVector,
+ boolean omitNorms,
+ boolean storePayloads,
+ IndexOptions indexOptions,
+ DocValuesType docValuesType,
+ long dvGen,
+ int pointDimensionCount,
+ int pointIndexDimensionCount,
+ int pointNumBytes,
+ int vectorDimension,
+ VectorValues.SearchStrategy vectorSearchStrategy) {
+ if (indexOptions == null) {
+ throw new IllegalStateException("IndexOptions must not be null (field: '" + name + "')");
Review comment:
should this be IAE instead?
##########
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 "
Review comment:
this is preexisting right? I personally still thing IAE is better here. but I am ok both ways
##########
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(
+ String fieldName, IndexOptions indexOptions1, IndexOptions indexOptions2) {
+ if (indexOptions1 != indexOptions2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from index options="
+ + indexOptions1
+ + " to inconsistent index options="
+ + indexOptions2);
}
+ }
- if (this.pointDimensionCount == 0 && dimensionCount != 0) {
- this.pointDimensionCount = dimensionCount;
- this.pointIndexDimensionCount = indexDimensionCount;
- this.pointNumBytes = dimensionNumBytes;
- } else if (dimensionCount != 0
- && (this.pointDimensionCount != dimensionCount
- || this.pointIndexDimensionCount != indexDimensionCount
- || this.pointNumBytes != dimensionNumBytes)) {
+ /**
+ * Verify that the provided docValues type are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameDocValuesType(
+ String fieldName, DocValuesType docValuesType1, DocValuesType docValuesType2) {
+ if (docValuesType1 != docValuesType2) {
throw new IllegalArgumentException(
"cannot change field \""
- + name
+ + fieldName
+ + "\" from doc values type="
+ + docValuesType1
+ + " to inconsistent doc values type="
+ + docValuesType2);
+ }
+ }
+
+ /**
+ * Verify that the provided doc values generations are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ // TODO: not sure if gen also must be the same
+ public static void verifySameDVGen(String fieldName, long docValuesGen1, long docValuesGen2) {
+ if (docValuesGen1 != docValuesGen2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from doc values generation="
+ + docValuesGen1
+ + " to inconsistent doc values generation="
+ + docValuesGen2);
+ }
+ }
+
+ /**
+ * Verify that the provided store term vectors options are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameStoreTermVectors(
+ String fieldName, boolean storeTermVector1, boolean storeTermVector2) {
+ if (storeTermVector1 != storeTermVector2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from storeTermVector="
+ + storeTermVector1
+ + " to inconsistent storeTermVector="
+ + storeTermVector2);
+ }
+ }
+
+ /**
+ * Verify that the provided omitNorms are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameOmitNorms(String fieldName, boolean omitNorms1, boolean omitNorms2) {
+ if (omitNorms1 != omitNorms2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from omitNorms="
+ + omitNorms1
+ + " to inconsistent omitNorms="
+ + omitNorms2);
+ }
+ }
+
+ /**
+ * Verify that the provided points indexing options are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySamePointsOptions(
+ String fieldName,
+ int pointDimensionCount1,
+ int indexDimensionCount1,
+ int numBytes1,
+ int pointDimensionCount2,
+ int indexDimensionCount2,
+ int numBytes2) {
+ if (pointDimensionCount1 != pointDimensionCount2
+ || indexDimensionCount1 != indexDimensionCount2
+ || numBytes1 != numBytes2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ "\" from points dimensionCount="
- + this.pointDimensionCount
+ + pointDimensionCount1
+ ", indexDimensionCount="
- + this.pointIndexDimensionCount
+ + indexDimensionCount1
+ ", numBytes="
- + this.pointNumBytes
+ + numBytes1
+ " to inconsistent dimensionCount="
- + dimensionCount
+ + pointDimensionCount2
+ ", indexDimensionCount="
- + indexDimensionCount
+ + indexDimensionCount2
+ ", numBytes="
- + dimensionNumBytes);
+ + numBytes2);
}
+ }
- // if updated field data is not for indexing, leave the updates out
- if (this.indexOptions != IndexOptions.NONE) {
- this.storeTermVector |= storeTermVector; // once vector, always vector
- this.storePayloads |= storePayloads;
-
- // Awkward: only drop norms if incoming update is indexed:
- if (indexOptions != IndexOptions.NONE && this.omitNorms != omitNorms) {
- this.omitNorms = true; // if one require omitNorms at least once, it remains off for life
- }
- }
- if (this.indexOptions == IndexOptions.NONE
- || this.indexOptions.compareTo(IndexOptions.DOCS_AND_FREQS_AND_POSITIONS) < 0) {
- // cannot store payloads if we don't store positions:
- this.storePayloads = false;
- }
- if (attributes != null) {
- this.attributes.putAll(attributes);
+ /**
+ * Verify that the provided vector indexing options are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameVectorOptions(
Review comment:
can be pkg private?
##########
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);
Review comment:
now that we don't pass the dvGen maybe we can use equals to compare? It might be controversial since we can't take the dvGen in to account.
##########
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(
+ String fieldName, IndexOptions indexOptions1, IndexOptions indexOptions2) {
+ if (indexOptions1 != indexOptions2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from index options="
+ + indexOptions1
+ + " to inconsistent index options="
+ + indexOptions2);
}
+ }
- if (this.pointDimensionCount == 0 && dimensionCount != 0) {
- this.pointDimensionCount = dimensionCount;
- this.pointIndexDimensionCount = indexDimensionCount;
- this.pointNumBytes = dimensionNumBytes;
- } else if (dimensionCount != 0
- && (this.pointDimensionCount != dimensionCount
- || this.pointIndexDimensionCount != indexDimensionCount
- || this.pointNumBytes != dimensionNumBytes)) {
+ /**
+ * Verify that the provided docValues type are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameDocValuesType(
+ String fieldName, DocValuesType docValuesType1, DocValuesType docValuesType2) {
+ if (docValuesType1 != docValuesType2) {
throw new IllegalArgumentException(
"cannot change field \""
- + name
+ + fieldName
+ + "\" from doc values type="
+ + docValuesType1
+ + " to inconsistent doc values type="
+ + docValuesType2);
+ }
+ }
+
+ /**
+ * Verify that the provided doc values generations are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ // TODO: not sure if gen also must be the same
+ public static void verifySameDVGen(String fieldName, long docValuesGen1, long docValuesGen2) {
+ if (docValuesGen1 != docValuesGen2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from doc values generation="
+ + docValuesGen1
+ + " to inconsistent doc values generation="
+ + docValuesGen2);
+ }
+ }
+
+ /**
+ * Verify that the provided store term vectors options are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameStoreTermVectors(
+ String fieldName, boolean storeTermVector1, boolean storeTermVector2) {
+ if (storeTermVector1 != storeTermVector2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from storeTermVector="
+ + storeTermVector1
+ + " to inconsistent storeTermVector="
+ + storeTermVector2);
+ }
+ }
+
+ /**
+ * Verify that the provided omitNorms are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameOmitNorms(String fieldName, boolean omitNorms1, boolean omitNorms2) {
Review comment:
can be pkg private?
##########
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:
maybe we can safe some lines and put some args on the same line?
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -972,6 +671,64 @@ private boolean assertNotFinished() {
return true;
}
+ private FieldInfo addField(
+ String name,
+ int preferredFieldNumber,
+ boolean storeTermVector,
+ boolean omitNorms,
+ boolean storePayloads,
+ IndexOptions indexOptions,
+ DocValuesType docValues,
+ long dvGen,
+ Map<String, String> attributes,
+ int dataDimensionCount,
+ int indexDimensionCount,
+ int dimensionNumBytes,
+ int vectorDimension,
+ VectorValues.SearchStrategy vectorSearchStrategy,
+ boolean isSoftDeletesField) {
+ // 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:
+ assert assertNotFinished();
+ final int fieldNumber =
+ globalFieldNumbers.addOrGet(
+ name,
+ preferredFieldNumber,
+ indexOptions,
+ storeTermVector,
+ omitNorms,
+ docValues,
+ dataDimensionCount,
+ indexDimensionCount,
+ dimensionNumBytes,
+ vectorDimension,
+ vectorSearchStrategy,
+ isSoftDeletesField);
+ FieldInfo fi =
+ new FieldInfo(
+ name,
+ fieldNumber,
+ storeTermVector,
+ omitNorms,
+ storePayloads,
+ indexOptions,
+ docValues,
+ dvGen,
+ attributes,
+ dataDimensionCount,
+ indexDimensionCount,
+ dimensionNumBytes,
+ vectorDimension,
+ vectorSearchStrategy,
+ isSoftDeletesField);
+ assert byName.containsKey(fi.name) == false;
Review comment:
can we have an assertion message here just in case
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -787,7 +557,6 @@ synchronized void setVectorDimensionsAndSearchStrategy(
}
public void add(FieldInfos other) {
Review comment:
I think we can make this method pkg private too?
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexingChain.java
##########
@@ -1313,4 +1307,118 @@ public void recycleIntBlocks(int[][] blocks, int offset, int length) {
bytesUsed.addAndGet(-(length * (IntBlockPool.INT_BLOCK_SIZE * Integer.BYTES)));
}
}
+
+ /**
+ * A schema of the field in the current document. With every new document this schema is reset. As
+ * the document fields are processed, we update the schema with options encountered in this
+ * document. Once the processing for the document is done, we compare the built schema of the
+ * current document with the corresponding FieldInfo (FieldInfo is built on a first document in
+ * the segment where we encounter this field). If there is inconsistency, we raise an error. This
+ * ensures that a field has the same data structures across all documents.
+ */
+ private static final class FieldSchema {
Review comment:
can't we fold this somehow into `FieldInfo` and then just create a single FieldInfo that we carry through out the indexing process for a segment. Maybe I am missing something which is absolutely possible but if we see FieldInfos / FieldNumbers as the source of truth it should never every change after we added it the first time or are there some parts of it that can change? I think it should be consistent for a segment at least except of attributes but they can be mutable for the segment? Maybe I am asking for too much, I really try to prevent having all these args again
##########
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:
we can also remove the dv gen from here I think
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -972,6 +671,64 @@ private boolean assertNotFinished() {
return true;
}
+ private FieldInfo addField(
+ String name,
+ int preferredFieldNumber,
+ boolean storeTermVector,
+ boolean omitNorms,
+ boolean storePayloads,
+ IndexOptions indexOptions,
+ DocValuesType docValues,
+ long dvGen,
+ Map<String, String> attributes,
+ int dataDimensionCount,
+ int indexDimensionCount,
+ int dimensionNumBytes,
+ int vectorDimension,
+ VectorValues.SearchStrategy vectorSearchStrategy,
+ boolean isSoftDeletesField) {
+ // 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:
+ assert assertNotFinished();
+ final int fieldNumber =
+ globalFieldNumbers.addOrGet(
Review comment:
maybe we can simplify this a bit and return a `FieldInfo` from this call then we don't need to pass all the args again and we guarantee that we constructed what we checked?
##########
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(
+ name,
+ storeTermVector,
+ omitNorms,
+ storePayloads,
+ indexOptions,
+ docValuesType,
+ dvGen,
+ pointDimensionCount,
+ pointIndexDimensionCount,
+ pointNumBytes,
+ vectorDimension,
+ vectorSearchStrategy);
+ }
+
+ /**
+ * Check correctness of FieldInfo options
+ *
+ * @throws IllegalStateException if some options are incorrect
+ * @return {@code true} if all options are correct
+ */
+ public static boolean checkOptionsCorrectness(
+ String name,
+ boolean storeTermVector,
+ boolean omitNorms,
+ boolean storePayloads,
+ IndexOptions indexOptions,
+ DocValuesType docValuesType,
+ long dvGen,
+ int pointDimensionCount,
+ int pointIndexDimensionCount,
+ int pointNumBytes,
+ int vectorDimension,
+ VectorValues.SearchStrategy vectorSearchStrategy) {
+ if (indexOptions == null) {
+ throw new IllegalStateException("IndexOptions must not be null (field: '" + name + "')");
Review comment:
since we are including the field name in every exception now maybe it might make sense to have a static method with _String field, String msg_ to not repeat yourself all other the place?
##########
File path: lucene/core/src/java/org/apache/lucene/index/IndexingChain.java
##########
@@ -581,78 +584,160 @@ 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"):
+ int docFieldIdx = 0;
termsHash.startDocument();
-
startStoredFields(docID);
try {
+ // 1st pass over doc fields – verify that doc schema matches the index schema
+ // build schema for each unique doc field
+ for (IndexableField field : document) {
+ IndexableFieldType fieldType = field.fieldType();
+ PerField pf = getOrAddPerField(field.name(), fieldType);
+ if (pf.fieldGen != fieldGen) { // first time we see this field in this document
+ fields[fieldCount++] = pf;
+ pf.fieldGen = fieldGen;
+ pf.reset(docID);
+ }
+ if (docFieldIdx >= docFields.length) oversizeDocFields();
+ docFields[docFieldIdx++] = pf;
+ updateDocFieldSchema(field.name(), pf.schema, fieldType);
+ }
+ // for each field verify that its schema within the current doc matches its schema in the
+ // index
+ for (int i = 0; i < fieldCount; i++) {
+ PerField pf = fields[i];
+ if (pf.fieldInfo == null) { // the first time we see this field in this segment
+ initializeFieldInfo(pf);
+ } else {
+ pf.schema.assertSameSchema(pf.fieldInfo);
+ }
+ }
+
+ // 2nd pass over doc fields – index each field
+ // also count the number of unique fields indexed with postings
+ docFieldIdx = 0;
for (IndexableField field : document) {
- fieldCount = processField(docID, field, fieldGen, fieldCount);
+ if (processField(docID, field, docFields[docFieldIdx])) {
+ fields[indexedFieldCount] = docFields[docFieldIdx];
+ indexedFieldCount++;
+ }
+ docFieldIdx++;
}
} finally {
if (hasHitAbortingException == false) {
// Finish each indexed field name seen in the document:
- for (int i = 0; i < fieldCount; i++) {
+ for (int i = 0; i < indexedFieldCount; i++) {
fields[i].finish(docID);
}
finishStoredFields();
+ // TODO: for broken docs, optimize termsHash.finishDocument
Review comment:
did you add this? What is there to optimize? :)
##########
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(
+ String fieldName, IndexOptions indexOptions1, IndexOptions indexOptions2) {
+ if (indexOptions1 != indexOptions2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from index options="
+ + indexOptions1
+ + " to inconsistent index options="
+ + indexOptions2);
}
+ }
- if (this.pointDimensionCount == 0 && dimensionCount != 0) {
- this.pointDimensionCount = dimensionCount;
- this.pointIndexDimensionCount = indexDimensionCount;
- this.pointNumBytes = dimensionNumBytes;
- } else if (dimensionCount != 0
- && (this.pointDimensionCount != dimensionCount
- || this.pointIndexDimensionCount != indexDimensionCount
- || this.pointNumBytes != dimensionNumBytes)) {
+ /**
+ * Verify that the provided docValues type are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameDocValuesType(
+ String fieldName, DocValuesType docValuesType1, DocValuesType docValuesType2) {
+ if (docValuesType1 != docValuesType2) {
throw new IllegalArgumentException(
"cannot change field \""
- + name
+ + fieldName
+ + "\" from doc values type="
+ + docValuesType1
+ + " to inconsistent doc values type="
+ + docValuesType2);
+ }
+ }
+
+ /**
+ * Verify that the provided doc values generations are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ // TODO: not sure if gen also must be the same
+ public static void verifySameDVGen(String fieldName, long docValuesGen1, long docValuesGen2) {
Review comment:
can this be a private method?
##########
File path: lucene/core/src/java/org/apache/lucene/index/FieldInfos.java
##########
@@ -787,7 +557,6 @@ synchronized void setVectorDimensionsAndSearchStrategy(
}
public void add(FieldInfos other) {
Review comment:
or even merge it into `getMergedFieldInfos`
##########
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(
+ String fieldName, IndexOptions indexOptions1, IndexOptions indexOptions2) {
+ if (indexOptions1 != indexOptions2) {
+ throw new IllegalArgumentException(
+ "cannot change field \""
+ + fieldName
+ + "\" from index options="
+ + indexOptions1
+ + " to inconsistent index options="
+ + indexOptions2);
}
+ }
- if (this.pointDimensionCount == 0 && dimensionCount != 0) {
- this.pointDimensionCount = dimensionCount;
- this.pointIndexDimensionCount = indexDimensionCount;
- this.pointNumBytes = dimensionNumBytes;
- } else if (dimensionCount != 0
- && (this.pointDimensionCount != dimensionCount
- || this.pointIndexDimensionCount != indexDimensionCount
- || this.pointNumBytes != dimensionNumBytes)) {
+ /**
+ * Verify that the provided docValues type are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ public static void verifySameDocValuesType(
+ String fieldName, DocValuesType docValuesType1, DocValuesType docValuesType2) {
+ if (docValuesType1 != docValuesType2) {
throw new IllegalArgumentException(
"cannot change field \""
- + name
+ + fieldName
+ + "\" from doc values type="
+ + docValuesType1
+ + " to inconsistent doc values type="
+ + docValuesType2);
+ }
+ }
+
+ /**
+ * Verify that the provided doc values generations are the same
+ *
+ * @throws IllegalArgumentException if they are not the same
+ */
+ // TODO: not sure if gen also must be the same
+ public static void verifySameDVGen(String fieldName, long docValuesGen1, long docValuesGen2) {
Review comment:
in fact I think we can remove it, see my comment here https://github.com/apache/lucene/pull/11/files?diff=unified&w=1#diff-b81932cf39b70347481b9659dd55b9b373427c2992cdacdfbc10ff8f4ea9eec0R594
##########
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) {
Review comment:
I think we can remove this method and only expose the `add(FieldInfo fi)` the DV gen should be irrelevant in all the cases when this is called (here in the builder and in the ParallelLeafReader) In fact, I think it would actually be wrong to enforce they are consistent. There might be 2 parallel leaf readers that have different DV gens and it will fail while that's ok?! I think we don't have a test for this at all.
----------------------------------------------------------------
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