Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-5325

Move ValueSource and FunctionValues under core/

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: master (7.0), 6.4
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Spinoff from LUCENE-5298: 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 LUCENE-5298:

      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.

      1. LUCENE-5325.patch
        33 kB
        Alan Woodward
      2. LUCENE-5325.patch
        34 kB
        Alan Woodward
      3. LUCENE-5325.patch
        34 kB
        Alan Woodward
      4. LUCENE-5325.patch
        122 kB
        Alan Woodward
      5. LUCENE-5325.patch
        56 kB
        Alan Woodward
      6. LUCENE-5325.patch
        62 kB
        Alan Woodward
      7. LUCENE-5325.patch
        57 kB
        Alan Woodward
      8. LUCENE-5325-6x.patch
        33 kB
        Alan Woodward
      9. LUCENE-5325-6x-matchingbits.patch
        3 kB
        Alan Woodward

        Issue Links

          Activity

          Hide
          jpountz Adrien Grand added a comment -

          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)?
          Show
          jpountz Adrien Grand added a comment - 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)?
          Hide
          dsmiley David Smiley added a comment -

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

          Show
          dsmiley David Smiley added a comment - +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.
          Hide
          rjernst Ryan Ernst added a comment -

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

          Show
          rjernst Ryan Ernst added a comment - +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.
          Hide
          jpountz Adrien Grand added a comment -

          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.

          Show
          jpountz Adrien Grand added a comment - 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.
          Hide
          rjernst Ryan Ernst added a comment -

          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.

          Show
          rjernst Ryan Ernst added a comment - 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.
          Hide
          shaie Shai Erera added a comment -

          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!

          Show
          shaie Shai Erera added a comment - 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!
          Hide
          romseygeek Alan Woodward added a comment -

          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.

          Show
          romseygeek Alan Woodward added a comment - 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.
          Hide
          rjernst Ryan Ernst added a comment -

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

          Show
          rjernst Ryan Ernst added a comment - Alan Woodward sounds like an interesting idea, a prototype patch sounds good.
          Hide
          dsmiley David Smiley added a comment -

          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.

          Show
          dsmiley David Smiley added a comment - 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.
          Hide
          yseeley@gmail.com Yonik Seeley added a comment -

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

          Indeed, multi-value VS needs to change!

          Show
          yseeley@gmail.com Yonik Seeley added a comment - Like the multi-value aspect, which is in turn only used by a small piece of Solr. Indeed, multi-value VS needs to change!
          Hide
          hossman Hoss Man added a comment -

          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

          Show
          hossman Hoss Man added a comment - 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
          Hide
          romseygeek Alan Woodward added a comment -

          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.
          Show
          romseygeek Alan Woodward added a comment - 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.
          Hide
          dsmiley David Smiley added a comment -

          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.

          Show
          dsmiley David Smiley added a comment - 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.
          Hide
          rcmuir Robert Muir added a comment -

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

          Show
          rcmuir Robert Muir added a comment - 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).
          Hide
          romseygeek Alan Woodward added a comment -

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

          Show
          romseygeek Alan Woodward added a comment - 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!)
          Hide
          romseygeek Alan Woodward added a comment -

          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
          Show
          romseygeek Alan Woodward added a comment - 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
          Hide
          romseygeek Alan Woodward added a comment -

          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.

          Show
          romseygeek Alan Woodward added a comment - 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.
          Hide
          rcmuir Robert Muir added a comment -

          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?

          Show
          rcmuir Robert Muir added a comment - 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?
          Hide
          romseygeek Alan Woodward added a comment -

          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.

          Show
          romseygeek Alan Woodward added a comment - 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.
          Hide
          romseygeek Alan Woodward added a comment -

          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.

          Show
          romseygeek Alan Woodward added a comment - 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.
          Hide
          romseygeek Alan Woodward added a comment -

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

          Show
          romseygeek Alan Woodward added a comment - Any comments on the patch? I'll commit in the next couple of days if everybody's happy.
          Hide
          jpountz Adrien Grand added a comment -

          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.

          Show
          jpountz Adrien Grand added a comment - 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.
          Hide
          romseygeek Alan Woodward added a comment -

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

          Show
          romseygeek Alan Woodward added a comment - 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().
          Hide
          jpountz Adrien Grand added a comment -

          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

          Show
          jpountz Adrien Grand added a comment - 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
          Hide
          romseygeek Alan Woodward added a comment -

          Final patch, to apply after LUCENE-7607.

          Show
          romseygeek Alan Woodward added a comment - Final patch, to apply after LUCENE-7607 .
          Hide
          jpountz Adrien Grand added a comment -

          +1

          Show
          jpountz Adrien Grand added a comment - +1
          Hide
          romseygeek Alan Woodward added a comment -

          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

          Show
          romseygeek Alan Woodward added a comment - 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
          Hide
          jira-bot ASF subversion and git services added a comment -

          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

          Show
          jira-bot ASF subversion and git services added a comment - 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
          Hide
          jira-bot ASF subversion and git services added a comment -

          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

          Show
          jira-bot ASF subversion and git services added a comment - 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
          Hide
          dsmiley David Smiley added a comment -

          Yay; thanks so much for this Alan Woodward!

          Show
          dsmiley David Smiley added a comment - Yay; thanks so much for this Alan Woodward !
          Hide
          jpountz Adrien Grand added a comment -

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

          Show
          jpountz Adrien Grand added a comment - Alan Woodward On the 6.x branch, I am confused why FieldValuesSource always returns true for advanceExact, shouldn't it check docsWithField?
          Hide
          romseygeek Alan Woodward added a comment -

          shouldn't it check docsWithField?

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

          Show
          romseygeek Alan Woodward added a comment - shouldn't it check docsWithField? Hm, you're quite right. Here's a patch fixing that - will commit shortly.
          Hide
          dsmiley David Smiley added a comment -

          A test for that would be good.

          Show
          dsmiley David Smiley added a comment - A test for that would be good.
          Hide
          jira-bot ASF subversion and git services added a comment -

          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

          Show
          jira-bot ASF subversion and git services added a comment - 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
          Hide
          jira-bot ASF subversion and git services added a comment -

          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

          Show
          jira-bot ASF subversion and git services added a comment - 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
          Hide
          romseygeek Alan Woodward added a comment -

          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.

          Show
          romseygeek Alan Woodward added a comment - 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.

            People

            • Assignee:
              romseygeek Alan Woodward
              Reporter:
              shaie Shai Erera
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development