SOLR-15440: FieldValueFeature now uses DocValues when docValues=true and stored=true are combined.

[cpoerschke]

https://issues.apache.org/jira/browse/SOLR-15440

[cpoerschke]

This builds on the #123 changes, so tagging both @tomglk and @alessandrobenedetti as potential reviewers, thank you!

[tomglk]

Hi @cpoerschke ,

thank you for continuing the work here!

The use of useDocValuesForStored to switch between the behaviours looks good to me, it seems concise.
As for my understanding you plan to remove the LegacyFieldValueFeature and useDocValuesForStored in 9.X or 10, correct?

Looking at the code, I think that people would have to copy quite a bit of code if they want to still use the legacy behaviour if useDocValuesForStored is gone. But I think that's fine.
As long as all the FieldValueFeatures are separated from each other and we do not have the prefetching, the DocValues approach is most efficient, I think.

I refactored the tests a bit, to make it easier to see what test_storedDvIsTrendy_FieldValueFeatureScorer_className_differs wants to test and added some helper functions for testThatCorrectFieldValueFeatureIsUsedForDocValueTypes to (subjectively) improve readability.

What do you think about those changes?

Other than that all good for me. :)

[cpoerschke]

Hi @tomgl - thank you very much for your inputs!

As for my understanding you plan to remove the LegacyFieldValueFeature and useDocValuesForStored in 9.X or 10, correct?

Yes, the plan is to remove LegacyFieldValueFeature in a future version and the plan was to remove useDocValuesForStored too, however ...

Looking at the code, I think that people would have to copy quite a bit of code if they want to still use the legacy behaviour if useDocValuesForStored is gone. ...

You raise a good point w.r.t. useDocValuesForStored removal being restrictive and actually retaining it adds near-zero code complexity and would facilitate anyone's custom retention of the legacy behaviour. And perhaps even a protected flag is clearer than a variant FieldValueFeature constructor? So 31eff54 is to retain the flag then.

I refactored the tests a bit, to make it easier to see what test_storedDvIsTrendy_FieldValueFeatureScorer_className_differs wants to test and added some helper functions for testThatCorrectFieldValueFeatureIsUsedForDocValueTypes to (subjectively) improve readability.

Thanks for that! Have added it to the branch and built on it with the 1a5b7b7 commit - WDYT?

[cpoerschke]

https://lists.apache.org/thread/ybmp5xz3c9549opqgktdb4cxlpmdmwy4 (I think) is the dev list thread w.r.t. Crave.io CI patch application.

[tomglk]

I agree to your reasoning and like your changes in 31eff54 !

Moving the setup of doc 21 into the helper function makes total sense. :)

The assert for the used scorer is a bit hidden now. I fear that for others it would not be obvious at first glance what the tests want to test.
I moved it out of the helper function in ffea1a8.
Does that make sense to you?

[cpoerschke]

The assert for the used scorer is a bit hidden now. I fear that for others it would not be obvious at first glance what the tests want to test. ...

Yes, I agree that the assert in the helper method reduces test comprehension, and also the extra expectedUsedScorerClass parameter seemed a bit tedious.

cf1ed8f builds on your change i.e. the assert is moved but the ObservingFieldValueFeature.usedScorerClass reading is retained near the ObservingFieldValueFeature.usedScorerClass = null clearing - WDYT?

[tomglk]

Oh yes, keeping the reading near the clearing is a good call. Nice!

I like it and do not see any more todos. :)

[cpoerschke]

https://lists.apache.org/thread/ybmp5xz3c9549opqgktdb4cxlpmdmwy4 (I think) is the dev list thread w.r.t. Crave.io CI patch application.

./gradlew -p solr/modules/ltr test passes locally.