Move ValueSource and FunctionValues under core/ [LUCENE-5325]

[asfimport]

Spinoff from #6362: ValueSource and FunctionValues are abstract APIs which exist under the queries/ module. That causes any module which wants to depend on these APIs (but not necessarily on any of their actual implementations!), to depend on the queries/ module. If we move these APIs under core/, we can eliminate these dependencies and add some mock impls for testing purposes.

Quoting Robert from #6362:

we should eliminate the suggest/ dependencies on expressions and queries, the expressions/ on queries, the grouping/ dependency on queries, the spatial/ dependency on queries, its a mess.

To add to that list, facet/ should not depend on queries too.

---

Migrated from LUCENE-5325 by Shai Erera (@shaie), resolved Dec 29 2016
Attachments: LUCENE-5325.patch (versions: 7), LUCENE-5325-6x.patch, LUCENE-5325-6x-matchingbits.patch
Linked issues:

• SOLR-6575

Sub-tasks:

• Change raw Map type to Map<String,Object> for ValueSource context [LUCENE-5335] #6399

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

I'm +1 for having these APIs in core, but I'd like us to do some cleanup before integrating them into core. In particular:

• could we remove byteVal, shortVal, intVal and floatVal and only have longVal and doubleVal to deal with numerics?
• could we improve the multi-valued APIs? Maybe something more like SortedSetDocValues (setDocument/nextOrd)?

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

+1 to move ValueSource/FunctionValues to Core. I've certainly viewed it as a core part of Lucene.

Adrien, by "improve the multi-valued APIs", are you perhaps referring ValueSource/FunctionValue's rather odd set of methods that take a 2nd parameter which is an array to dump multiple values out into? MultiValueSource goes along with this. I rather hate MultiValueSource and all those overloaded methods – I look forward to seeing a better design. The main problem with MultiValueSource is that it's design is limited to all documents having a constant number of values, it doesn't vary per document. Notice MultiValueSource's dimension() takes no arguments – it's result is effectively constant.

[asfimport]

Ryan Ernst (@rjernst) (migrated from JIRA)

+1 for the APIs in the core.

@shaie, have you already started on this? If not I'd be happy to take it on.

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

Adrien, by "improve the multi-valued APIs", are you perhaps referring ValueSource/FunctionValue's rather odd set of methods that take a 2nd parameter which is an array to dump multiple values out into? MultiValueSource goes along with this. I rather hate MultiValueSource and all those overloaded methods – I look forward to seeing a better design. The main problem with MultiValueSource is that it's design is limited to all documents having a constant number of values, it doesn't vary per document. Notice MultiValueSource's dimension() takes no arguments – it's result is effectively constant.

Exactly. :-)

[asfimport]

Ryan Ernst (@rjernst) (migrated from JIRA)

I began doing the move and refactorings discussed here, but this turned into a very large change. After some discussion, I'm going to try creating smaller subtasks. The first example is #6399. There are a bunch more changes like this we should make before doing the actual move.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Shai Erera, have you already started on this? If not I'd be happy to take it on.

No, I haven't started any work yet and won't be able to work on it in the next few weeks, so feel free to take it!

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

Some stuff I've been working on has touched this, and I think I have a possible way forward. One of the issues I have with ValueSource is that it's inherently type-unsafe - you have no way of saying what sort of value you want from a ValueSource input. However, pretty much all the VS consumers we have only actually use a single type: expressions uses doubleVal(), facet uses longVal() for LongRange and doubleVal() for DoubleRange, function queries use floatVal(), etc.

I propose we add a new set of abstract classes to the core search package, exposing per-document Long, Double and BytesRef values. We add some simple wrapper methods to the queries module to convert a ValueSource to a LongValuesSource/DoubleValuesSource/BytesRefValuesSource. Then we can go through each module and convert it to use the core classes instead of ValueSource, one by one. This breaks things up into the subtasks Ryan was talking about, and allows us to defer worrying about some of the wartier aspects of the current ValueSource API (quite a few of which are only used by a single module, and can be narrowed down to a module-specific subclass or implementation).

I have the start of a patch for this, which I can upload if people think this sounds like a good idea, cutting over expressions and facets.

[asfimport]

Ryan Ernst (@rjernst) (migrated from JIRA)

@romseygeek sounds like an interesting idea, a prototype patch sounds good.

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

Definitely sounds great. I know what you mean by this having a lot of warts. Like the multi-value aspect, which is in turn only used by a small piece of Solr.

[asfimport]

Yonik Seeley (@yonik) (migrated from JIRA)

Like the multi-value aspect, which is in turn only used by a small piece of Solr.

Indeed, multi-value VS needs to change!

[asfimport]

Chris M. Hostetter (@hossman) (migrated from JIRA)

One of the issues I have with ValueSource is that it's inherently type-unsafe - you have no way of saying what sort of value you want from a ValueSource input.

This comment reminded me of a discussion I remember having a long time ago that i started typing up a summary of in case it inspires folks, but then realized I already filed as SOLR-6575 the last time someone mentioned something that reminded me of it.

Might be some helpful ideas there if folks are looking to revamp & improve the type specificity of the ValueSource APIs

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

Patch.

To keep it simple, I've just added DoubleValues / DoubleValuesSource, and cut over the expressions module to use it. Still needs some work:

• the ValueSource.toDoubleValuesSource() method needs a test
• I'm not quite sure how to deal with exceptions throw by Scorer.score() in DoubleValuesSource.SCORES
• I've copied the exists() method from FunctionValues to DoubleValues, but I'm not convinced we need it. The expressions module doesn't actually use it, at any rate, but in some cases I can see it's useful to distinguish between a zero-value and a non-existent value. Maybe I should remove it here, and it can be added in later if it's needed for a specific function?
• DoubleValues is an abstract class rather than an interface, simply because I couldn't get the ASM stuff in JavascriptCompiler to work with interfaces.

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

I've copied the exists() method from FunctionValues to DoubleValues, but I'm not convinced we need it.

I think it is needed. Consider a ValueSource that wants to pick between the value from a wrapped ValueSource, or if when that VS has no value, then return the value from another (perhaps a constant default but could be anything). The only way to get this and remove a FunctionValues.exists() method I see is having some sort of BooleanValueSource that has implementations to check if a DocValue's value exists. Then you could address the same use-cases, I think.

The patch looked alright but I didn't apply it to get a more thorough impression. I certainly like the simplicity of it. And I like this API/functionality moving to Core from Queries module – it's very generic & foundational.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

is there a way to move out the setScorer method? it seems like this one really does not belong, e.g. should be at a higher level and this api should just have the per-document get ? I like David's idea about separate boolean exists (or simply have method to get the Bits for this at a higher level).

to me that is better, because then its up to the thing using it to determine how null values should be treated. For example expressions/ could treat them as NaN rather than 0, maybe that is more intuitive (especially with respect to sorting).

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

Updated patch, nuking setScorer() and exists().

There's a bit of hinky logic around creating sorts now, because FieldComparator separates out setNextReader() and setScorer(), but it is at least contained. We still need to pass in a Scorer somewhere though, so that scores are available to the leaf values.

Another option would be to pull the Scorer logic back into the expressions module itself, but I think having a generic DoubleValues.SCORES instance is going to be useful in lots of situations; plus it means that all of the sorting logic is in DoubleValuesProducer itself.

Also adds a test for converting a DoubleValuesSource into a generic ValueSource (not the other way round, which I had originally put in and isn't needed yet!)