Repository navigation
SOLR-18379: remove DocsStreamer.KNOWN_TYPES for the ExternalizeStoredValuesAsObjects marker - #4758
Conversation
…ubclasses Scaffolding only, no production change. SOLR-18379 wants KNOWN_TYPES replaced by the ExternalizeStoredValuesAsObjects marker, and that swap is not equivalent: KNOWN_TYPES is consulted on class equality, the marker on instanceof, so the DenseVectorField subclasses ScalarQuantizedDenseVectorField and BinaryQuantizedDenseVectorField would move from FieldType.toExternal to FloatPointField.toObject - String to Float per vector element. Nothing would have noticed. Making DocsStreamer.getValue throw for any DenseVectorField subclass left both quantized field tests green, 9 tests and no failures, because they never traverse that code. These two tests do: with the same throwing control they now fail while the plain DenseVectorField test still passes, which is the asymmetry that proves they reach it for a subclass and only for a subclass. They assert today's values - the Strings 1.1 through 4.4 and their type - so the migration turns a silent change of the response format into two failing assertions. AI-assisted (Claude Sonnet 5)
…ValuesAsObjects marker KNOWN_TYPES was consulted on class equality, the marker on instanceof, so this is not an equivalent swap - it widens the toObject path to subclasses. Ten of the sixteen listed types override toObject(IndexableField), so the widening is only observable below those ten, and the marker is placed on ten bases rather than all sixteen: the five Trie*Field inherit it from TrieField and DenseVectorField from FloatPointField, both of which were themselves in the set. Two behaviour changes follow and both are deliberate. Quantized dense vector fields - ScalarQuantizedDenseVectorField and BinaryQuantizedDenseVectorField - now return stored values as numbers rather than strings. That repairs an inconsistency rather than introducing one: they returned strings only because the equality check excluded them while their own superclass returned floats. Nothing caught this before; making DocsStreamer.getValue throw for any DenseVectorField subclass left both quantized field tests green because they never traversed it. The pins added in the preceding commit fail on the change instead, which is what they are for. BinaryField subclasses lose their toExternal representation, because BinaryField narrows toObject's return type to ByteBuffer and no subclass can override it back to a String. There is no version of this removal that spares them, and leaving BinaryField unmarked would change BinaryField itself. In-tree that reaches only test support: StrBinaryField and SwapBytesBinaryField in core tests, and SortableBinaryField in test-framework, whose stored values go from base64 to binary. TestBinaryField's bean field for str_data is updated accordingly; str_data_dv keeps its string because docValues take a different overload. StrField and TextField don't override toObject(IndexableField) at all - its default already delegates to toExternal - so the marker changes nothing for them or for UUIDField, which inherits it from StrField. Verified: compileJava/compileTestJava, ecjLintMain/Test, spotlessJavaCheck, renderJavadoc, zero remaining references to KNOWN_TYPES anywhere, and DocsStreamerTest + TestBinaryField + ScalarQuantizedDenseVectorFieldTest + DenseVectorFieldTest - 59 tests, 0 failures. AI-assisted (Claude Sonnet 5)
…sstreamer-known-types
| * @see PointField | ||
| */ | ||
| public class DatePointField extends PointField implements DateValueFieldType { | ||
| public class DatePointField extends PointField |
There was a problem hiding this comment.
so, this ends up being more intrusive because we are now implementing FieldType.ExternalizeStoredValuesAsObjects..... Maybe that is okay?
There was a problem hiding this comment.
Yes, deliberately -- instanceof means subclasses inherit trust (old KNOWN_TYPES used exact-class equality). UUIDField extends StrField without overriding toObject, so it still returns a String -- safe.
…sstreamer-known-types
…sstreamer-known-types # Conflicts: # solr/core/src/java/org/apache/solr/response/DocsStreamer.java # solr/core/src/java/org/apache/solr/schema/TrieField.java
|
@epugh anything else I should fix here? |
|
While this is a removeal of deprecation, the way it works adds a new interface... Which I really don't have a strong sense of "is this right or not" approach... Again, one of the areas where I am maybe less comfortable saying "this is the right path forward". Would you be willing to send an email to dev@ mailing list with a short summary and get peoples thoughts on the use of the marker interface? If the approach is one folks like, then this looks great. |
|
The marker isn't new — |
…sstreamer-known-types
…sstreamer-known-types
…sstreamer-known-types
2 participants
https://issues.apache.org/jira/browse/SOLR-18379
Replaces
DocsStreamer.KNOWN_TYPES(16 classes, exact equality) with theExternalizeStoredValuesAsObjectsmarker (instanceof) on 10 base types — the 5Trie*FieldandDenseVectorFieldinherit it from their parents, so 16 types end up covered by 10 declarations. Landed as two commits: the first pins today's stored-value output for the quantized vector fields (values that were never actually verified before, per a throwing control), the second does the swap so those same pins catch any drift.Despite touching 15 files, most give identical output either way:
StrField/TextField(andUUIDField, inheriting fromStrField) don't overridetoObject(IndexableField), so it already delegates totoExternal— the marker changes nothing for them. Only two real behaviour changes: quantized dense-vector fields now return numbers instead of strings (repairs an inconsistency with their own superclass), andBinaryFieldtest subclasses (StrBinaryField,SwapBytesBinaryField,SortableBinaryField) lose their string form sinceBinaryField.toObjectreturnsByteBuffer, which no subclass can override back toString.Zero remaining references to
KNOWN_TYPES. 59 tests acrossDocsStreamerTest,TestBinaryField,ScalarQuantizedDenseVectorFieldTest,DenseVectorFieldTest— 0 failures.AI-assisted (Claude Sonnet 5)