Improve document API for stored fields. #12116

jpountz · 2023-01-27T16:13:56Z

Currently stored fields have to look at binaryValue(), stringValue() and numericValue() to guess the type of the value and then store it. This has a few issues:

If there is a problem, e.g. all of these 3 methods return null, it's currently discovered late, when we already passed the responsibility of writing data from IndexingChain to the codec.
numericValue() is used both for numeric doc values and storage. This makes it impossible to implement a double field that is stored and doc-valued, as numericValue() needs to return simultaneously a number that consists of the double for storage, and the long bits of the double for doc values.
binaryValue() is used both for sorted(_set) doc values and storage. This makes it impossible to implement keyword fields that is stored and doc-valued, as the field returns a non-null value for both binaryValue() and stringValue() and stored fields no longer know which field to store.

This commit introduces IndexableField#storedValue(), which is used only for stored fields. This addresses the above issues. IndexingChain passes the storedValue() directly to the codec, so it's impossible for a stored fields format to mistakenly use binaryValue()/stringValue()/numericValue() instead of storedValue().

Currently stored fields have to look at binaryValue(), stringValue() and numericValue() to guess the type of the value and then store it. This has a few issues: - If there is a problem, e.g. all of these 3 methods return null, it's currently discovered late, when we already passed the responsibility of writing data from IndexingChain to the codec. - numericValue() is used both for numeric doc values and storage. This makes it impossible to implement a `double` field that is stored and doc-valued, as numericValue() needs to return simultaneously a number that consists of the double for storage, and the long bits of the double for doc values. - binaryValue() is used both for sorted(_set) doc values and storage. This makes it impossible to implement `keyword` fields that is stored and doc-valued, as the field returns a non-null value for both binaryValue() and stringValue() and stored fields no longer know which field to store. This commit introduces `IndexableField#storedValue()`, which is used only for stored fields. This addresses the above issues. IndexingChain passes the storedValue() directly to the codec, so it's impossible for a stored fields format to mistakenly use binaryValue()/stringValue()/numericValue() instead of storedValue().

rmuir · 2023-01-27T19:17:10Z

This is great, thanks for looking into it. It moves "type-guessing" into the one place that should be doing it, which is the generic Field.java

i didn't really think too deeply about implementation but I suppose there might be a few options:

"Box" the value along with its type, like what you've done here (are there perf implications?)
change api of the StoredFieldsWriter (codec api), e.g. to provide field's type (like the enum inside your current "box") as parameter or separate method (e.g. writeIntField, writeStringField, ...)

edit: try to organize thoughts better here

jpountz · 2023-01-30T09:37:50Z

@rmuir I tried your idea of using a different method per type of stored value, and I liked the result: StoredValue no longer escapes the document API and the default impl for merging became more straightforward.

lucene/core/src/java/org/apache/lucene/document/StoredValue.java

…ld, DoubleField.

jpountz · 2023-02-02T15:57:31Z

I went one step further:

StringField now implements storedValue() instead of relying on the guessing of Field#storedValue()
IntField, LongField, FloatField and DoubleField now support storing the value

This looks good to me overall, so I marked the PR as ready for review, what do you think?

rmuir · 2023-02-02T16:51:22Z

Thanks for removing the type-guessing from the "structured" fields and supporting store! Can we fix the class-level javadocs for IntField & co?

They still say this: "If you also need to store the value, you should add a separate StoredField instance."

jpountz · 2023-02-02T17:43:38Z

Thanks for catching this, I just pushed a fix.

rmuir · 2023-02-02T18:55:10Z

lucene/core/src/java/org/apache/lucene/codecs/StoredFieldsWriter.java

@@ -43,7 +38,7 @@
 * <ol>
 *   <li>For every document, {@link #startDocument()} is called, informing the Codec that a new
 *       document has started.
- *   <li>{@link #writeField(FieldInfo, IndexableField)} is called for each field in the document.
+ *   <li>{@link #writeField} is called for each field in the document.


would maybe just change this to {@code writeField}. Not sure which one of the overloaded methods it links too now?

rmuir · 2023-02-02T18:55:41Z

lucene/core/src/java/org/apache/lucene/codecs/StoredFieldsWriter.java

-   * Called before writing the stored fields of the document. {@link #writeField(FieldInfo,
-   * IndexableField)} will be called for each stored field. Note that this is called even if the
-   * document has no stored fields.
+   * Called before writing the stored fields of the document. {@link #writeField} will be called for


similar here, i think {@code would be better

rmuir · 2023-02-02T18:56:19Z

lucene/core/src/java/org/apache/lucene/codecs/StoredFieldsWriter.java

-   * #writeField(FieldInfo, IndexableField)}, and {@link #finish(int)}, returning the number of
-   * documents that were written. Implementations can override this method for more sophisticated
-   * merging (bulk-byte copying, etc).
+   * #writeField}, and {@link #finish(int)}, returning the number of documents that were written.


one last ambiguous @link that might better be @code

rmuir

this looks great, not only does it open up new options for Field classes to be simpler, but it cleans up a lot of type-safety and IMO makes a lot of these pieces easier to read.

Currently stored fields have to look at binaryValue(), stringValue() and numericValue() to guess the type of the value and then store it. This has a few issues: - If there is a problem, e.g. all of these 3 methods return null, it's currently discovered late, when we already passed the responsibility of writing data from IndexingChain to the codec. - numericValue() is used both for numeric doc values and storage. This makes it impossible to implement a `double` field that is stored and doc-valued, as numericValue() needs to return simultaneously a number that consists of the double for storage, and the long bits of the double for doc values. - binaryValue() is used both for sorted(_set) doc values and storage. This makes it impossible to implement `keyword` fields that is stored and doc-valued, as the field returns a non-null value for both binaryValue() and stringValue() and stored fields no longer know which field to store. This commit introduces `IndexableField#storedValue()`, which is used only for stored fields. This addresses the above issues. IndexingChain passes the storedValue() directly to the codec, so it's impossible for a stored fields format to mistakenly use binaryValue()/stringValue()/numericValue() instead of storedValue().

jpountz mentioned this pull request Jan 27, 2023

Introduce a new KeywordField. #12054

Merged

Change stored fields API to use a different method per type.

c87f7d8

rmuir reviewed Jan 30, 2023

View reviewed changes

lucene/core/src/java/org/apache/lucene/document/StoredValue.java Outdated Show resolved Hide resolved

rmuir reviewed Jan 30, 2023

View reviewed changes

lucene/core/src/java/org/apache/lucene/document/StoredValue.java Show resolved Hide resolved

jpountz added 4 commits January 31, 2023 15:42

Merge branch 'main' into improve_indexable_field_stored_value

a88c93b

Fix typos.

dafcf63

Merge branch 'main' into improve_indexable_field_stored_value

2549d01

Implement storedValue() in StringField, IntField, LongField, FloatFie…

f6576cb

…ld, DoubleField.

jpountz marked this pull request as ready for review February 2, 2023 15:53

Add missing javadocs.

fd5995f

Fix class-level javadocs.

e23d7d3

rmuir reviewed Feb 2, 2023

View reviewed changes

rmuir approved these changes Feb 2, 2023

View reviewed changes

Migrate TextField too.

f8819dc

jpountz merged commit 96136b4 into apache:main Feb 6, 2023

jpountz deleted the improve_indexable_field_stored_value branch February 6, 2023 16:08

jpountz added this to the 9.6.0 milestone Feb 6, 2023

jpountz added a commit that referenced this pull request Feb 6, 2023

Fix ambiguous links. (#12116)

684f31e

jpountz added a commit that referenced this pull request Feb 7, 2023

Fix nightly compatibility tests after #12116.

8b572f0

rmuir mentioned this pull request Feb 9, 2023

Skip the TokenStream overhead when indexing simple keywords. #12139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve document API for stored fields. #12116

Improve document API for stored fields. #12116

jpountz commented Jan 27, 2023

rmuir commented Jan 27, 2023 •

edited

Loading

jpountz commented Jan 30, 2023

jpountz commented Feb 2, 2023

rmuir commented Feb 2, 2023

jpountz commented Feb 2, 2023

rmuir Feb 2, 2023

rmuir Feb 2, 2023

rmuir Feb 2, 2023

rmuir left a comment

Improve document API for stored fields. #12116

Improve document API for stored fields. #12116

Conversation

jpountz commented Jan 27, 2023

rmuir commented Jan 27, 2023 • edited Loading

jpountz commented Jan 30, 2023

jpountz commented Feb 2, 2023

rmuir commented Feb 2, 2023

jpountz commented Feb 2, 2023

rmuir Feb 2, 2023

Choose a reason for hiding this comment

rmuir Feb 2, 2023

Choose a reason for hiding this comment

rmuir Feb 2, 2023

Choose a reason for hiding this comment

rmuir left a comment

Choose a reason for hiding this comment

rmuir commented Jan 27, 2023 •

edited

Loading