If would be wonderful if it could go to 9.1.0, that way we could probably skip patching ourselves (we need that in the place were I work). Either way, thank you for the review!
Does it look right?
Sure, can do. What's the rule? Should I put my name and yours as the reviewer?
Ouch, I see that some of the checks fail. Sorry about that, I only run tests. Will fix soon.
@cpoerschke: I would really appreciate if you could review this one as the original author of the code.
This commit removes unnecessary check for
FieldLengthFeatureScorer that otherwise causes
FieldLengthFeature for non-stored or missing fields.
I recently encountered a problem with using
FieldLengthFeature with non-stored fields where feature extraction/LTR reranking would result in
NullPointerException. I check the
FieldLengthFeature docs and briefly inspected implementation to check if for using this feature storing fields is really required. According to my expectations it's not required - the only required information are norms, that my field has enabled.
I search Solr Jira and found SOLR-13219 that seemed very similar in nature. Upon investigation I found that the two issues have indeed the same origin. The root cause seems to be the following line:
final IndexableField idxF = searcher.doc(0).getField(field);
That returns null for non-stored missing fields.
Honestly, I don't understand why
FieldLengthFeatureScorer even checks whether the field omits norms or not. It's only used in
FieldLengthFeature#scorer and this method already checks whether norms are there or not (and if they are missing a constant 0 ValueFeatureScorer is returned).
I removed the problematic code.
description fields (
schema.xml proving that existing tests still work as long as the problematic code is removed (without it
TestFieldLength#testRanking is failing). With this change I also had to slightly change
TestFeatureLogging#testGeneratedFeatures test, but without changing its logic.
I also added
TestFieldLength#testRankingDynamicField test to prove that
TestFieldLength works correctly with dynamic fields too, and doesn't fail if one (or more) documents from rerank candidate set lacks specific field.
Please review the following and check all that apply:
Yes, that's the workaround I used in the past.