Lucene - Core
  1. Lucene - Core
  2. LUCENE-1789

getDocValues should provide a MultiReader DocValues abstraction

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      When scoring a ValueSourceQuery, the scoring code calls ValueSource.getValues(reader) on each leaf level subreader – so DocValue instances are backed by the individual FieldCache entries of the subreaders – but if Client code were to inadvertently called getValues() on a MultiReader (or DirectoryReader) they would wind up using the "outer" FieldCache.

      Since getValues(IndexReader) returns DocValues, we have an advantage here that we don't have with FieldCache API (which is required to provide direct array access). getValues(IndexReader) could be implimented so that IF some a caller inadvertently passes in a reader with non-null subReaders, getValues could generate a DocValues instance for each of the subReaders, and then wrap them in a composite "MultiDocValues".

      1. LUCENE-1789.patch
        16 kB
        Michael McCandless

        Activity

        Hide
        Hoss Man added a comment - - edited

        This idea orriginated in LUCENE-1749, see these comments...

        https://issues.apache.org/jira/browse/LUCENE-1749?focusedCommentId=12740155#action_12740155
        https://issues.apache.org/jira/browse/LUCENE-1749?focusedCommentId=12740256#action_12740256
        https://issues.apache.org/jira/browse/LUCENE-1749?focusedCommentId=12740278#action_12740278

        I've marked this for 2.9 for now .... i think it's a "nice to have" in 2.9, because unlike general FieldCache usage, the API is abstract enough we can protect our users from mistakes; but i don't personally think it's critical that we do this if no one else wants to take a stab at it.

        (EDIT: shorter versions of URLs to prevent horizontal scroll)

        Show
        Hoss Man added a comment - - edited This idea orriginated in LUCENE-1749 , see these comments... https://issues.apache.org/jira/browse/LUCENE-1749?focusedCommentId=12740155#action_12740155 https://issues.apache.org/jira/browse/LUCENE-1749?focusedCommentId=12740256#action_12740256 https://issues.apache.org/jira/browse/LUCENE-1749?focusedCommentId=12740278#action_12740278 I've marked this for 2.9 for now .... i think it's a "nice to have" in 2.9, because unlike general FieldCache usage, the API is abstract enough we can protect our users from mistakes; but i don't personally think it's critical that we do this if no one else wants to take a stab at it. (EDIT: shorter versions of URLs to prevent horizontal scroll)
        Hide
        Mark Miller added a comment -

        Its basically what I did as a first attempt at 1771 actually (you have a glimpse into how hectic my brain is in that I didn't remember that 30 minutes ago) :

        (with some of this in ReaderUtil now, it can be written in half the length)

        +    // constructor
        +    private ValueSourceScorer(Similarity similarity, IndexReader reader, ValueSourceWeight w, boolean valuesFromSubReaders) throws IOException {
               super(similarity);
        +      if(!valuesFromSubReaders) {
        +        this.weight = w;
        +        this.qWeight = w.getValue();
        +        // this is when/where the values are first created.
        +        vals = valSrc.getValues(reader);
        +        termDocs = reader.termDocs(null);
        +        return;
        +      }
        +      
               this.weight = w;
               this.qWeight = w.getValue();
        -      // this is when/where the values are first created.
        -      vals = valSrc.getValues(reader);
        +      List subReadersList = new ArrayList();
        +      ReaderUtil.gatherSubReaders(subReadersList, reader);
        +      subReaders = (IndexReader[]) subReadersList.toArray(new IndexReader[subReadersList.size()]);
        +      valsArray = new DocValues[subReaders.length];
        +      docStarts = new int[subReaders.length];
        +      int maxDoc = 0;
        +      for (int i = 0; i < subReaders.length; i++) {
        +        docStarts[i] = maxDoc;
        +        maxDoc += subReaders[i].maxDoc();
        +        valsArray[i] = valSrc.getValues(subReaders[i]);
        +      }
        +      
        +      vals = new DocValues() {
        +
        +        //@Override
        +        public float floatVal(int doc) {
        +          int n = ReaderUtil.subSearcher(doc, subReaders.length, docStarts);
        +          return valsArray[n].floatVal(doc);
        +        }
        +
        +        //@Override
        +        public String toString(int doc) {
        +          return Float.toString(floatVal(doc));
        +        }
        +        
        +      };
               termDocs = reader.termDocs(null);
             }
        
        
        Show
        Mark Miller added a comment - Its basically what I did as a first attempt at 1771 actually (you have a glimpse into how hectic my brain is in that I didn't remember that 30 minutes ago) : (with some of this in ReaderUtil now, it can be written in half the length) + // constructor + private ValueSourceScorer(Similarity similarity, IndexReader reader, ValueSourceWeight w, boolean valuesFromSubReaders) throws IOException { super (similarity); + if (!valuesFromSubReaders) { + this .weight = w; + this .qWeight = w.getValue(); + // this is when/where the values are first created. + vals = valSrc.getValues(reader); + termDocs = reader.termDocs( null ); + return ; + } + this .weight = w; this .qWeight = w.getValue(); - // this is when/where the values are first created. - vals = valSrc.getValues(reader); + List subReadersList = new ArrayList(); + ReaderUtil.gatherSubReaders(subReadersList, reader); + subReaders = (IndexReader[]) subReadersList.toArray( new IndexReader[subReadersList.size()]); + valsArray = new DocValues[subReaders.length]; + docStarts = new int [subReaders.length]; + int maxDoc = 0; + for ( int i = 0; i < subReaders.length; i++) { + docStarts[i] = maxDoc; + maxDoc += subReaders[i].maxDoc(); + valsArray[i] = valSrc.getValues(subReaders[i]); + } + + vals = new DocValues() { + + //@Override + public float floatVal( int doc) { + int n = ReaderUtil.subSearcher(doc, subReaders.length, docStarts); + return valsArray[n].floatVal(doc); + } + + //@Override + public String toString( int doc) { + return Float .toString(floatVal(doc)); + } + + }; termDocs = reader.termDocs( null ); }
        Hide
        Michael McCandless added a comment -

        It is nice that DocValues gives us the freedom to do this, but.... I'm
        not sure we should, because it's a sizable performance trap.

        Ie, we'll be silently inserting a call to ReaderUtil.subSearcher on
        every doc value lookup (vs previously when it was a single top-level
        array lookup).

        While client code that has relied on this in the past will nicely
        continue to function properly, if we make this change, its performance
        is going to silently take a [possibly sizable] hit.

        In general, with Lucene, we can do the per-segment switching "up high"
        (which is what the core now does, exclusively), or we can do it "down
        low" (creating MultiTermDocs, MultiTermEnum, MultiTermPositions,
        MultiDocValues, etc.), which has sizable performance costs. It's also
        costly for us because we'll have N different places where we must
        create & maintain a MultiXXX class. I would love to someday deprecate
        all of the "down low" switching classes

        In the core I think we should always switch "up high". We've already
        done this w/ searching and collection/sorting. In LUCENE-1771 we're
        fixing IndexSearcher.explain to do so as well.

        With external code, I'd like over time to strongly encourage only
        switching "up high" as well.

        Maybe it'd be best if we could somehow allow this "down low" switching
        for 2.9, but 1) warn that you'll see a performance hit right off, 2)
        deprecate it, and 3) and somehow state that in 3.0 you'll have to send
        only a SegmentReader to this API, instead.

        EG, imagine an app that created an external custom HitCollector that
        calls say FloatFieldSource on the top reader in order to use of a
        float value per doc in each collect() call. On upgrading to 2.9, this
        app will already have to make the switch to the Collector API, which'd
        be a great time for them to also then switch to pulling these float
        values per-segment. But, if we make the proposed change here, the app
        could in fact just keep working off the top-level values (eg if the
        ctor in their class is pulling these values), thinking everything is
        fine when in fact there is a sizable, silent perf hit. I'd prefer in
        2.9 for them to also switch their DocValues lookup to be per segment.

        [Aside: once we gain clarity on LUCENE-831, hopefully we can do away
        with oal.search.function.FieldCacheSource,

        {Byte,Short,Int,Ord,ReverseOrd}

        FieldSource, etc. Ie these classes
        basically copy what FieldCache does, but expose a per-doc method call
        instead of a fixed array lookup.]

        Show
        Michael McCandless added a comment - It is nice that DocValues gives us the freedom to do this, but.... I'm not sure we should, because it's a sizable performance trap. Ie, we'll be silently inserting a call to ReaderUtil.subSearcher on every doc value lookup (vs previously when it was a single top-level array lookup). While client code that has relied on this in the past will nicely continue to function properly, if we make this change, its performance is going to silently take a [possibly sizable] hit. In general, with Lucene, we can do the per-segment switching "up high" (which is what the core now does, exclusively), or we can do it "down low" (creating MultiTermDocs, MultiTermEnum, MultiTermPositions, MultiDocValues, etc.), which has sizable performance costs. It's also costly for us because we'll have N different places where we must create & maintain a MultiXXX class. I would love to someday deprecate all of the "down low" switching classes In the core I think we should always switch "up high". We've already done this w/ searching and collection/sorting. In LUCENE-1771 we're fixing IndexSearcher.explain to do so as well. With external code, I'd like over time to strongly encourage only switching "up high" as well. Maybe it'd be best if we could somehow allow this "down low" switching for 2.9, but 1) warn that you'll see a performance hit right off, 2) deprecate it, and 3) and somehow state that in 3.0 you'll have to send only a SegmentReader to this API, instead. EG, imagine an app that created an external custom HitCollector that calls say FloatFieldSource on the top reader in order to use of a float value per doc in each collect() call. On upgrading to 2.9, this app will already have to make the switch to the Collector API, which'd be a great time for them to also then switch to pulling these float values per-segment. But, if we make the proposed change here, the app could in fact just keep working off the top-level values (eg if the ctor in their class is pulling these values), thinking everything is fine when in fact there is a sizable, silent perf hit. I'd prefer in 2.9 for them to also switch their DocValues lookup to be per segment. [Aside: once we gain clarity on LUCENE-831 , hopefully we can do away with oal.search.function.FieldCacheSource, {Byte,Short,Int,Ord,ReverseOrd} FieldSource, etc. Ie these classes basically copy what FieldCache does, but expose a per-doc method call instead of a fixed array lookup.]
        Hide
        Michael McCandless added a comment -

        Or... how about if we made a separate "helper" class, whose purpose
        was to accept a top-level reader and do "down low" switching to this
        new MultiDocValues class. This class would be deprecated, ie, exist
        only in 2.9 to help external usage of the DocValues API migrate to "up
        high" switching.

        However, you'd have to explicitly create this class. EG, in the
        normal DocValues classes we throw an exception if you pass in a
        top-level reader, noting clearly that you could 1) switch to this
        helper class (at a sizable per-lookup performance hit), or 2) switch
        to looking up your values per-segment?

        This way at least it'd be much clearer to the external consumer the
        cost of using the "down low" switching class. It'd make the decision
        explicit, not silent, on upgrading to 2.9.

        Show
        Michael McCandless added a comment - Or... how about if we made a separate "helper" class, whose purpose was to accept a top-level reader and do "down low" switching to this new MultiDocValues class. This class would be deprecated, ie, exist only in 2.9 to help external usage of the DocValues API migrate to "up high" switching. However, you'd have to explicitly create this class. EG, in the normal DocValues classes we throw an exception if you pass in a top-level reader, noting clearly that you could 1) switch to this helper class (at a sizable per-lookup performance hit), or 2) switch to looking up your values per-segment? This way at least it'd be much clearer to the external consumer the cost of using the "down low" switching class. It'd make the decision explicit, not silent, on upgrading to 2.9.
        Hide
        Hoss Man added a comment -

        While client code that has relied on this in the past will nicely
        continue to function properly, if we make this change, its performance
        is going to silently take a [possibly sizable] hit.

        Correct: a change like this could cause 2.9 to introduce a time based performance hit from the added method call to resolve the sub(reader|docvalue) on each method call ... but if we don't have a change like this, 2.9 could introduce a memory based performance hit from the other FieldCache changes as it client code accessing DocValues for the top level reader will create a duplication of the whole array.

        Incidently: I'm willing to believe you that the time based perf hit would be high, but my instinct is that it wouldn't be that bad: the DocValues API already introduces at least one method call per doc lookup (two depending on datatype). adding a second method call to delegate to a sub-DocValues isntance doesn't seem that bad (especially since a new MultDocValues class could get the subReader list and compute the docId offsets on init, and then reuse them on each method call)

        In the core I think we should always switch "up high".

        (In case there is any confusion: wasn't suggesting that we stop using "up high" switching on DocValues in code included in the Lucene dist, i was suggesting that if someone uses DocValues directly in their code (against a top level reader) then we help them out by giving them the "down low" switching ... so "expected" usages wouldn't pay the added time based hit, just "unexpected" usages (which would be saved from the memory hit))

        Maybe it'd be best if we could somehow allow this "down low" switching
        for 2.9, but 1) warn that you'll see a performance hit right off, 2)
        deprecate it, and 3) and somehow state that in 3.0 you'll have to send
        only a SegmentReader to this API, instead.

        that would get into really sticky territory for people writting custom IndexReaders (or using FilteredIndexReader)

        But, if we make the proposed change here, the app could in fact just keep working off the top-level values (eg if the ctor in their class is pulling these values), thinking everything is fine when in fact there is a sizable, silent perf hit.

        I agree ... but unless i'm missing something about the code on the trunk, that situation already exists: the developer might switch to using the Collector API, but nothing about the current trunk will prevent/warn him that this...

        ValueSource vs = new ValueSource("aFieldIAlsoSortOn");
        IndexReader r = getCurrentReaderThatCouldBeAMultiReader();
        DocValues vals = vs.getDocValues(r);
        

        ...could have a sizable, silent, memory perf hit in 2.9

        (ValueSource.getValues has a javadoc indicating that caching will be done on the IndexReader passed in, but your comment suggests that if 2.9 were released today (with hte current trunk) people upgrading would have some obvious way of noticing that they need to pass a sub reader to getValues)

        Show
        Hoss Man added a comment - While client code that has relied on this in the past will nicely continue to function properly, if we make this change, its performance is going to silently take a [possibly sizable] hit. Correct: a change like this could cause 2.9 to introduce a time based performance hit from the added method call to resolve the sub(reader|docvalue) on each method call ... but if we don't have a change like this, 2.9 could introduce a memory based performance hit from the other FieldCache changes as it client code accessing DocValues for the top level reader will create a duplication of the whole array. Incidently: I'm willing to believe you that the time based perf hit would be high, but my instinct is that it wouldn't be that bad: the DocValues API already introduces at least one method call per doc lookup (two depending on datatype). adding a second method call to delegate to a sub-DocValues isntance doesn't seem that bad (especially since a new MultDocValues class could get the subReader list and compute the docId offsets on init, and then reuse them on each method call) In the core I think we should always switch "up high". (In case there is any confusion: wasn't suggesting that we stop using "up high" switching on DocValues in code included in the Lucene dist, i was suggesting that if someone uses DocValues directly in their code (against a top level reader) then we help them out by giving them the "down low" switching ... so "expected" usages wouldn't pay the added time based hit, just "unexpected" usages (which would be saved from the memory hit)) Maybe it'd be best if we could somehow allow this "down low" switching for 2.9, but 1) warn that you'll see a performance hit right off, 2) deprecate it, and 3) and somehow state that in 3.0 you'll have to send only a SegmentReader to this API, instead. that would get into really sticky territory for people writting custom IndexReaders (or using FilteredIndexReader) But, if we make the proposed change here, the app could in fact just keep working off the top-level values (eg if the ctor in their class is pulling these values), thinking everything is fine when in fact there is a sizable, silent perf hit. I agree ... but unless i'm missing something about the code on the trunk, that situation already exists: the developer might switch to using the Collector API, but nothing about the current trunk will prevent/warn him that this... ValueSource vs = new ValueSource( "aFieldIAlsoSortOn" ); IndexReader r = getCurrentReaderThatCouldBeAMultiReader(); DocValues vals = vs.getDocValues(r); ...could have a sizable, silent, memory perf hit in 2.9 (ValueSource.getValues has a javadoc indicating that caching will be done on the IndexReader passed in, but your comment suggests that if 2.9 were released today (with hte current trunk) people upgrading would have some obvious way of noticing that they need to pass a sub reader to getValues)
        Hide
        Michael McCandless added a comment -

        Correct: a change like this could cause 2.9 to introduce a time based performance hit from the added method call to resolve the sub(reader|docvalue) on each method call ... but if we don't have a change like this, 2.9 could introduce a memory based performance hit from the other FieldCache changes as it client code accessing DocValues for the top level reader will create a duplication of the whole array.

        True, and of the two, I agree a hidden time cost is the lesser evil.

        But I'd prefer to not hide the cost, ie, encourage/force an explicit
        choice when users upgrade to 2.9. If we can't think of some realistic
        way to do that, then I agree we should go forward with the current
        approach...

        Incidently: I'm willing to believe you that the time based perf hit would be high, but my instinct is that it wouldn't be that bad: the DocValues API already introduces at least one method call per doc lookup (two depending on datatype). adding a second method call to delegate to a sub-DocValues isntance doesn't seem that bad (especially since a new MultDocValues class could get the subReader list and compute the docId offsets on init, and then reuse them on each method call)

        It's the added binary search in ReaderUtil.subSearcher that worries
        me.

        In the core I think we should always switch "up high".

        (In case there is any confusion: wasn't suggesting that we stop using "up high" switching on DocValues in code included in the Lucene dist, i was suggesting that if someone uses DocValues directly in their code (against a top level reader) then we help them out by giving them the "down low" switching ... so "expected" usages wouldn't pay the added time based hit, just "unexpected" usages (which would be saved from the memory hit))

        Understood. We are only talking about external usages of these APIs,
        and even then, exceptionally advance usage. Ie, users who make their
        own ValueSourceQuery and then run it against an IndexSearcher will be
        fine. It's only people who directly invoke getValues, w/ some random
        reader, that hit the hidden cost.

        But, if we make the proposed change here, the app could in fact just keep working off the top-level values (eg if the ctor in their class is pulling these values), thinking everything is fine when in fact there is a sizable, silent perf hit.

        I agree ... but unless i'm missing something about the code on the trunk, that situation already exists: the developer might switch to using the Collector API, but nothing about the current trunk will prevent/warn him that this...

        ValueSource vs = new ValueSource("aFieldIAlsoSortOn");
        IndexReader r = getCurrentReaderThatCouldBeAMultiReader();
        DocValues vals = vs.getDocValues(r);
        ...could have a sizable, silent, memory perf hit in 2.9

        (ValueSource.getValues has a javadoc indicating that caching will be done on the IndexReader passed in, but your comment suggests that if 2.9 were released today (with hte current trunk) people upgrading would have some obvious way of noticing that they need to pass a sub reader to getValues)

        How about this: we add a new param to the ctors of the value sources,
        called (say) acceptMultiReader. It has 3 values:

        • NO means an exception is thrown on seeing a top reader (where "top
          reader" means any reader whose getSequentialSubReaders is
          non-null)
        • YES_BURN_TIME means accept the top reader and make a
          MultiDocValues
        • YES_BURN_MEMORY means use the top reader against the field cache

        We deprecate the existing ctors, so on moving to 3.0 you have to make
        an explicit choice, but default it to YES_BURN_TIME.

        One benefit of making the choice explicit is for those apps that have
        memory to burn they may in fact choose to burn it.

        Would this give a clean migration path forward?

        Show
        Michael McCandless added a comment - Correct: a change like this could cause 2.9 to introduce a time based performance hit from the added method call to resolve the sub(reader|docvalue) on each method call ... but if we don't have a change like this, 2.9 could introduce a memory based performance hit from the other FieldCache changes as it client code accessing DocValues for the top level reader will create a duplication of the whole array. True, and of the two, I agree a hidden time cost is the lesser evil. But I'd prefer to not hide the cost, ie, encourage/force an explicit choice when users upgrade to 2.9. If we can't think of some realistic way to do that, then I agree we should go forward with the current approach... Incidently: I'm willing to believe you that the time based perf hit would be high, but my instinct is that it wouldn't be that bad: the DocValues API already introduces at least one method call per doc lookup (two depending on datatype). adding a second method call to delegate to a sub-DocValues isntance doesn't seem that bad (especially since a new MultDocValues class could get the subReader list and compute the docId offsets on init, and then reuse them on each method call) It's the added binary search in ReaderUtil.subSearcher that worries me. In the core I think we should always switch "up high". (In case there is any confusion: wasn't suggesting that we stop using "up high" switching on DocValues in code included in the Lucene dist, i was suggesting that if someone uses DocValues directly in their code (against a top level reader) then we help them out by giving them the "down low" switching ... so "expected" usages wouldn't pay the added time based hit, just "unexpected" usages (which would be saved from the memory hit)) Understood. We are only talking about external usages of these APIs, and even then, exceptionally advance usage. Ie, users who make their own ValueSourceQuery and then run it against an IndexSearcher will be fine. It's only people who directly invoke getValues, w/ some random reader, that hit the hidden cost. But, if we make the proposed change here, the app could in fact just keep working off the top-level values (eg if the ctor in their class is pulling these values), thinking everything is fine when in fact there is a sizable, silent perf hit. I agree ... but unless i'm missing something about the code on the trunk, that situation already exists: the developer might switch to using the Collector API, but nothing about the current trunk will prevent/warn him that this... ValueSource vs = new ValueSource("aFieldIAlsoSortOn"); IndexReader r = getCurrentReaderThatCouldBeAMultiReader(); DocValues vals = vs.getDocValues(r); ...could have a sizable, silent, memory perf hit in 2.9 (ValueSource.getValues has a javadoc indicating that caching will be done on the IndexReader passed in, but your comment suggests that if 2.9 were released today (with hte current trunk) people upgrading would have some obvious way of noticing that they need to pass a sub reader to getValues) How about this: we add a new param to the ctors of the value sources, called (say) acceptMultiReader. It has 3 values: NO means an exception is thrown on seeing a top reader (where "top reader" means any reader whose getSequentialSubReaders is non-null) YES_BURN_TIME means accept the top reader and make a MultiDocValues YES_BURN_MEMORY means use the top reader against the field cache We deprecate the existing ctors, so on moving to 3.0 you have to make an explicit choice, but default it to YES_BURN_TIME. One benefit of making the choice explicit is for those apps that have memory to burn they may in fact choose to burn it. Would this give a clean migration path forward?
        Hide
        Hoss Man added a comment -

        How about this: we add a new param to the ctors of the value sources, called (say) acceptMultiReader. It has 3 values:

        ...that would work ... but i feel like there may be a cleaner API possible here...

        What if we just added a new MultiValueSource wrapper class, that acted as a proxy around another ValueSource so that the only non-transparent behavior is that MultiValueSource.getDocValues returns an instance of the new MultiDocValues we've been talking about.

        If you use something like FloatFieldSource directly in your code, you get what you ask for: the FieldCache is fetched agaisnt the exact reader you supply (ie: YES_BURN_MEMORY). If you want to use a FieldSource directly in your code, and you want to get good cache reuse, and you don't want to sorry about the subreaders yourself, you wrap your FieldSource in a new MultiValueSource(myFieldSource) (YES_BURN_TIME)

        The only thing this wouldn't get us is an obvious warning to developers on upgrading (like the deprecation warnings htat would come from your suggested API) ... but since nothing about backwards compatibility is actually breaking here, that doesn't seem like the end of the world – we can document it in CHANGES.txt (we're going to need a nice big section there about all the FieldCache usage changes anyway) drawing their attention to the new MultiValueSource they should consider using.

        My thinking is this: anybody who is constructing new ValueSOurces directly is pretty deep into the code, odds are if they're using that type of code, they might be mucking with the FieldCache directly in other ways as well – we can't solve all their problems, but we can give them helper code to make the transition easier)

        Show
        Hoss Man added a comment - How about this: we add a new param to the ctors of the value sources, called (say) acceptMultiReader. It has 3 values: ...that would work ... but i feel like there may be a cleaner API possible here... What if we just added a new MultiValueSource wrapper class, that acted as a proxy around another ValueSource so that the only non-transparent behavior is that MultiValueSource.getDocValues returns an instance of the new MultiDocValues we've been talking about. If you use something like FloatFieldSource directly in your code, you get what you ask for: the FieldCache is fetched agaisnt the exact reader you supply (ie: YES_BURN_MEMORY). If you want to use a FieldSource directly in your code, and you want to get good cache reuse, and you don't want to sorry about the subreaders yourself, you wrap your FieldSource in a new MultiValueSource(myFieldSource) (YES_BURN_TIME) The only thing this wouldn't get us is an obvious warning to developers on upgrading (like the deprecation warnings htat would come from your suggested API) ... but since nothing about backwards compatibility is actually breaking here, that doesn't seem like the end of the world – we can document it in CHANGES.txt (we're going to need a nice big section there about all the FieldCache usage changes anyway) drawing their attention to the new MultiValueSource they should consider using. My thinking is this: anybody who is constructing new ValueSOurces directly is pretty deep into the code, odds are if they're using that type of code, they might be mucking with the FieldCache directly in other ways as well – we can't solve all their problems, but we can give them helper code to make the transition easier)
        Hide
        Michael McCandless added a comment -

        What if we just added a new MultiValueSource wrapper class, that acted as a proxy around another ValueSource

        OK I like this solution!

        Show
        Michael McCandless added a comment - What if we just added a new MultiValueSource wrapper class, that acted as a proxy around another ValueSource OK I like this solution!
        Hide
        Hoss Man added a comment -

        Cool... i don't suppose you have time to work on a patch?

        (what's the emoticon for fingers crossed?)

        Show
        Hoss Man added a comment - Cool... i don't suppose you have time to work on a patch? (what's the emoticon for fingers crossed?)
        Hide
        Michael McCandless added a comment -

        OK, I'll take a crack at this!

        Show
        Michael McCandless added a comment - OK, I'll take a crack at this!
        Hide
        Michael McCandless added a comment -

        Attached patch.

        Show
        Michael McCandless added a comment - Attached patch.

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Hoss Man
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development