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)?

+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.

+1 for the APIs in the core.

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

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.

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 LUCENE-5335. There are a bunch more changes like this we should make before doing the actual move.

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!

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.

Alan Woodward sounds like an interesting idea, a prototype patch sounds good.

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.

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

Indeed, multi-value VS needs to change!

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

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.

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.

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).

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!)

Another iteration, this time pushing access to Scores entirely back into the expressions code. The API is pretty minimal now:

• DoubleValues just has ```double get(int)```
• DoubleValuesSource just has ```DoubleValues getValues(LeafReaderContext)```, plus some factory methods for access to DV fields and a SortField implementation

I'm going back and forth a bit on where scores should live. TaxonomyFacetSumValueSource also allows access to scores, so we need to be able to set a Scorer on an arbitrary DoubleValues instance there. However, once setScorer appears on DoubleValues, that means we need to add it to the long-valued equivalent (because we'll want to wrap one with another), so we can't just use NumericDocValues.

I dont understand the problem, it seems to be unrelated to the api at hand. Whether these double values come from scores, from the ABV of beers in the refridgerator, etc, seems none of its concern. It should just be a per-doc api for double values and nothing more?

You're right, it is unrelated, and there's a way around it, I just needed to get away from thinking in the mode of the previous API. So thanks for the push-back

Here's a new patch which cuts over expressions, facets and suggest. There's one new method on DoubleValuesSource and LongValuesSource, for retrieving a bitset marking which docs actually have a value here, which is required by the facet counting code. Field wrappers just forward to DocValues.getDocsWithField(), and everything else returns a MatchAllBits for now. Ideally, expressions would AND together the bitsets from all child variables, but that can be done in a followup once I've found the methods to actually do that efficiently...

I think this is good enough to start with? Grouping, join and queries modules are going to be more complicated I think, so should be done in a separate JIRA. And this patch is already pretty big, and can probably be broken up into separate commits for each module.

Waking this one up again. Here's a patch that takes into account the new doc values iterator API. The new classes look like this:

DoubleValuesSource

• getDoubleValues(LeafReaderContext, Scorer) which returns a DoubleValues

DoubleValues

• advanceTo(doc) : returns true if doc has a value
• doubleValue()

And similar for LongValuesSource/LongValues. There is already a LongValues class in lucene.util, so this probably needs a better name. The XValuesSource classes have a bunch of static methods for generating sources from doc values fields or from a Scorer.

I also have patches that convert to and from the old ValueSource API, and that migrate the facet, expression and suggest modules, but I think they should be kept for follow-up issues.

Any comments on the patch? I'll commit in the next couple of days if everybody's happy.

I am +1 in general, I think it's great that we are getting a cleaner values source API! I left some comments about the current patch below:

• Should we rename advanceTo to advanceExact to be more consistent with the doc values API? These methods seem to have the same contract.
• It seems to be that fromIntField could just delegate to fromLongField all the time?
• Can we use LongUnaryOperator.identity() rather than (v) -> v
• // TODO make setScorer throw IOException? +1
• LongValuesSource.fromDoubleField looks a bit trappy to me since it implicitly casts. Should we instead expect callers to call DoubleValuesSource.fromDoubleField and then do the casting explicitly using toLongValuesSource?
• Maybe we should not have the DoubleValues getValues(LeafReaderContext) method (without a Scorer). I'm wondering it might be better to require callers to pass null themselves when they know scores are not needed.

About the way we have access to scores, I'm wondering that things might be a bit cleaner if we avoided mixing the values source and Scorer APIs by replacing LongValues getValues(LeafReaderContext ctx, Scorer scorer) with LongValues getValues(LeafReaderContext ctx, DoubleValues scores) in addition to something like a DoubleValues fromScorer(Scorer scorer) method on DoubleValuesSource?

In general I think we should also better document the contract of these APIs, like what the expectations are for the scorer/scores when needsScores returns false and what toDoubleValues and toLongValues do since not all doubles can be represented as a long and vice-versa.

Thanks for the review, Adrien. Here's an updated patch:

• advanceTo renamed to advanceExact
• fromIntField delegates to fromLongField in both Long and DoubleValuesSource
• I ended up removing the automatic wrapping of doubles to longs in LongValuesSource entirely, in favour of calling DoubleValuesSource.toLongValuesSource(). The javadocs on the latter now explicitly say that the conversion is done via casting.
• Javadocs generally are more explanatory, particularly around scores
• getValues() now takes a DoubleValues object rather than a Scorer - this ended up being a very nice change, and will allow us to remove several of the FakeScorer implementations that the current ValueSource API forces us to use in things like expressions or facets.

I'll open a separate issue for adding the throws clause for FieldComparator.setScorer().

Thanks, it looks great. Some minor comments:

• Can you make the docs of advanceExact more explicit about the fact it cannot go backwards?
• Can you remove the @Test annotations from the test case?
• The // force in order comment is outdated, scoring always goes in order since 5.0. I think we can remove all this testing on BooleanQuery since it is now no different from a TermQuery for sorting.

Otherwise +1

Final patch, to apply after LUCENE-7607.

+1

The patch for 6x is slightly different, as the DocValues iterator API hasn't been backported, so the interaction with the FieldComparators has to take into account missing values

Commit e2aa2b638468f8ed7555533be25f4dc86780b840 in lucene-solr's branch refs/heads/branch_6x from Alan Woodward
[ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e2aa2b6 ]

LUCENE-5325: Add LongValuesSource and DoubleValuesSource in core

Commit 3f24fd81c836982be96b9b60082b53177fffe504 in lucene-solr's branch refs/heads/master from Alan Woodward
[ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3f24fd8 ]

LUCENE-5325: Add LongValuesSource and DoubleValuesSource in core

Yay; thanks so much for this Alan Woodward!

Alan Woodward On the 6.x branch, I am confused why FieldValuesSource always returns true for advanceExact, shouldn't it check docsWithField?

shouldn't it check docsWithField?

Hm, you're quite right. Here's a patch fixing that - will commit shortly.

A test for that would be good.

Commit 4e5a62140f4e90fc41fe91350c7787c8455f2887 in lucene-solr's branch refs/heads/branch_6x from Alan Woodward
[ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4e5a621 ]

LUCENE-5325: Check for matching bits in NumericDocValues to XValues converter

Commit a4335c0e9f01275c7d6e807813d9818b6e59d76e in lucene-solr's branch refs/heads/master from Alan Woodward
[ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a4335c0 ]

LUCENE-5325: Add test for missing values in sorts

I pushed the fix for 6.x and added a test there and on master. Thanks for the reviews! I'll open up some follow-up issues now.