Replace ValueSource context Map with a more concrete data type [LUCENE-6232]

[asfimport]

Inspired by #5046

The context object used by ValueSource and friends is a raw Map that provides no type safety guarantees. In our current state, there are lots of warnings about unchecked casts, raw types, and generally unsafe code from the compiler's perspective.

There are several common patterns and types of Objects that we store in the context. It would be beneficial to instead use a class with typed methods for get/set of Scorer, Weights, etc.

---

Migrated from LUCENE-6232 by Mike Drob (@madrob), resolved Jun 22 2020
Attachments: LUCENE-6232.patch (versions: 3)
Linked issues:

• Change raw Map type to Map<String,Object> for ValueSource context [LUCENE-5335] #6399
• Fix compiler warnings for ant clean compile [LUCENE-7602] #8653

[asfimport]

Mike Drob (@madrob) (migrated from JIRA)

What are our guarantees about backwards compatability? This would touch a lot of code across both Solr and Lucene.

Do we need to leave the existing methods, and add deprecation annotations? When can we excise them?

[asfimport]

Ryan Ernst (@rjernst) (migrated from JIRA)

@madrob See #6399 for some previous work here. IIRC, most of the warnings here were because of the raw map, which that issue tried to address by changing it to a String->Object map. The content of the map is pretty arbitrary.

[asfimport]

Terry Smith (migrated from JIRA)

I have custom code that injects objects into this map. If you refactor this to be concrete class could you leave it non-final so a custom FunctionQuery could provide it's own subclassed instance of this context?

[asfimport]

Mike Drob (@madrob) (migrated from JIRA)

@rjernst - I saw that JIRA, but like the comments said I don't think it solves all the problems. That's maybe a good start, but we need a much more serious refactoring so I thought maybe a separate issue would be more appropriate.

[asfimport]

Mike Drob (@madrob) (migrated from JIRA)

Terry Smith - Leaving it non-final is probably fine. There is also the possibility of leaving some map-like methods on the class (i.e. put(K,V) and get(K) with explicit documentation that they are not safe.

[asfimport]

Terry Smith (migrated from JIRA)

@madrob Thanks, either would work.

[asfimport]

Mike Drob (@madrob) (migrated from JIRA)

Attaching a patch that doesn't clean up all of the raw type warnings, but certainly gets a lot of them with a new class ValueSourceContext

CollapseScore stuff in here would introduce a circular dependency, so maybe that gets subclassed.

By my count there are still 7 calls left to put(), so it's at least a manageable number. At this point we might be able to add docs that say this is obviously unsafe, and supress the warnings if we can't think of anything better.

[asfimport]

Mike Drob (@madrob) (migrated from JIRA)

New patch, updated to the current trunk. Previous one had some issues with import statements and was missing a map initialization.

[asfimport]

Yonik Seeley (@yonik) (migrated from JIRA)

Here's the one I came up with in Heliosearch:
https://github.com/Heliosearch/heliosearch/blob/b7fa35c519d9facc3860dd78d28797542422080b/solr/core/src/java/org/apache/solr/search/QueryContext.java

It's called QueryContext since it seemed it could be more generic than just for value sources.

If we switch from IdentityHashMap (which is currently used) to HashMap, it might be good to make sure any heavy-weight equals implementations check for other==this.

Regarding the current patch on this issue,
https://github.com/Heliosearch/heliosearch/blob/b7fa35c519d9facc3860dd78d28797542422080b/solr/core/src/java/org/apache/solr/search/QueryContext.java

• public Map<String,FunctionValues> getValuesCache() {

That seems rather specific to a particular kind of value source and probably doesn't belong at this level?

[asfimport]

Mike Drob (@madrob) (migrated from JIRA)

It's called QueryContext since it seemed it could be more generic than just for value sources.

Naming things is one of Computer Science's greatest unsolved problems. I initially called mine QueryContext but decided it might be too broad. No objections to changing it back.

If we switch from IdentityHashMap (which is currently used) to HashMap, it might be good to make sure any heavy-weight equals implementations check for other==this.

This is a good idea regardless.

That seems rather specific to a particular kind of value source and probably doesn't belong at this level?

Yes, this is only used in one place. Easy enough to remove, but then we will have more usage of the unsafe put(). I have toyed around with creating subclasses for the context that can safely store a CollapseScore or other common patterns, but have yet t ocome up with something satisfactory. Maybe we just put this off for now.

Here's the one I came up with in Heliosearch

I'm glad it looks like we went down similar paths. I'll borrow some ideas from here and put up a new patch.

[asfimport]

Mike Drob (@madrob) (migrated from JIRA)

Patch incorporating Yonik's feedback.

[asfimport]

Mike Drob (@madrob) (migrated from JIRA)

Ping. Has anybody had a chance to look at the latest patch? Thanks!

[asfimport]

Mike Drob (@madrob) (migrated from JIRA)

@yonik - since you're backporting a bunch of other HelioSearch features already, do you think this will make it in transitively as well?

[asfimport]

Mike Drob (@madrob) (migrated from JIRA)

This was cleaned up by the recent work on removing warnings.