Solr
  1. Solr
  2. SOLR-3218

Range faceting support for CurrencyField

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 4.9, 5.0
    • Component/s: Schema and Analysis
    • Labels:
      None

      Description

      Spinoff from SOLR-2202. Need to add range faceting capabilities for CurrencyField

      1. SOLR-3218-1.patch
        26 kB
        Andrew Morrison
      2. SOLR-3218-2.patch
        27 kB
        Andrew Morrison
      3. SOLR-3218.patch
        27 kB
        Andrew Morrison
      4. SOLR-3218.patch
        24 kB
        Andrew Morrison
      5. SOLR-3218.patch
        29 kB
        Jan Høydahl
      6. SOLR-3218.patch
        29 kB
        Vitaliy Zhovtyuk
      7. SOLR-3218.patch
        29 kB
        Vitaliy Zhovtyuk

        Issue Links

          Activity

          Hide
          Andrew Morrison added a comment -

          Hoss Mann had posted the following on SOLR-2202
          -----------------------------------------------

          a) CurrencyField (and by extension "CurrencyValue") gets my vote

          b) i really only reviewed the facet stuff in SOLR-2202-solr-10.patch (i know Jan has already been reviewing the more core stuff about the type) ... it makes me realize that we really need to refactor the range faceting code to be easier to do in custom FieldTypes, but that's certainly no fault of this issue and can be done later.

          The facet code itself looks correct but my one concern is that (if i'm understanding all of this MoneyValue conversion stuff correctly) it should be possible to facet with start/end/gap values specified in any currency, as long as they are all consistent – but there is not test of this situation. the negative test only looks at using an inconsistent gap, and the positive tests only use USD, or the "default" which is also USD. We should have at least one test that uses something like EUR for start/end/gap and verifies that the counts are correct given the conversion rates used in the test.

          incidentally: I don't see anything actually enforcing that start/end are in the same currency – just that gap is in the same currency as the values it's being added to, so essentially that start and gap use hte same currenty. But I'm actually not at all clear on why there is any attempt to enforce that the currencies used are the same, since the whole point of the type (as i understand it) is that you can do conversions on the fly – it may seem silly for someone to say facet.range.start=0,USD & facet.range.gap=200,EUR & facet.range.end=1000,YEN but is there any technical reason why we can't let them do that?

          Show
          Andrew Morrison added a comment - Hoss Mann had posted the following on SOLR-2202 ----------------------------------------------- a) CurrencyField (and by extension "CurrencyValue") gets my vote b) i really only reviewed the facet stuff in SOLR-2202 -solr-10.patch (i know Jan has already been reviewing the more core stuff about the type) ... it makes me realize that we really need to refactor the range faceting code to be easier to do in custom FieldTypes, but that's certainly no fault of this issue and can be done later. The facet code itself looks correct but my one concern is that (if i'm understanding all of this MoneyValue conversion stuff correctly) it should be possible to facet with start/end/gap values specified in any currency, as long as they are all consistent – but there is not test of this situation. the negative test only looks at using an inconsistent gap, and the positive tests only use USD, or the "default" which is also USD. We should have at least one test that uses something like EUR for start/end/gap and verifies that the counts are correct given the conversion rates used in the test. incidentally: I don't see anything actually enforcing that start/end are in the same currency – just that gap is in the same currency as the values it's being added to, so essentially that start and gap use hte same currenty. But I'm actually not at all clear on why there is any attempt to enforce that the currencies used are the same, since the whole point of the type (as i understand it) is that you can do conversions on the fly – it may seem silly for someone to say facet.range.start=0,USD & facet.range.gap=200,EUR & facet.range.end=1000,YEN but is there any technical reason why we can't let them do that?
          Hide
          Andrew Morrison added a comment -

          Hoss,

          The attached patch (SOLR-3218-1.patch) adds additional tests for EUR and GBP.

          I believe start/end currency equality is enforced by MoneyType.compareTo which will throw an exception when end is compared to the first (start+gap).

          As far as enforcing currency equality being a good idea or not, it would make sense and I would prefer if start/end/gap currencies didn't need to be equal. This patch doesn't allow for that given the tradeoff of the utility of being able to use different currencies versus the annoyance of keeping a handle open to an ExchangeRateProvider in the places we'd need it.

          I'd be happy to take a look at making different currencies possible if there is enough interest.

          It'll be good to add a test for start/end currency mismatches. I'll upload SOLR-3218-2.patch in a moment.

          Show
          Andrew Morrison added a comment - Hoss, The attached patch ( SOLR-3218 -1.patch) adds additional tests for EUR and GBP. I believe start/end currency equality is enforced by MoneyType.compareTo which will throw an exception when end is compared to the first (start+gap). As far as enforcing currency equality being a good idea or not, it would make sense and I would prefer if start/end/gap currencies didn't need to be equal. This patch doesn't allow for that given the tradeoff of the utility of being able to use different currencies versus the annoyance of keeping a handle open to an ExchangeRateProvider in the places we'd need it. I'd be happy to take a look at making different currencies possible if there is enough interest. It'll be good to add a test for start/end currency mismatches. I'll upload SOLR-3218 -2.patch in a moment.
          Hide
          Andrew Morrison added a comment -

          Jan,

          Once SOLR-2202 is trunked I'll update this patch to work with CurrencyField.

          Show
          Andrew Morrison added a comment - Jan, Once SOLR-2202 is trunked I'll update this patch to work with CurrencyField.
          Hide
          Hoss Man added a comment -

          I believe start/end currency equality is enforced by MoneyType.compareTo which will throw an exception when end is compared to the first (start+gap).

          Ah ..ok. and then ultimately start+gap is compared to end (even if hardend is false) so you'll get a exception then. ok fair enough.

          As far as enforcing currency equality being a good idea or not, it would make sense and I would prefer if start/end/gap currencies didn't need to be equal. This patch doesn't allow for that given the tradeoff of the utility of being able to use different currencies versus the annoyance of keeping a handle open to an ExchangeRateProvider in the places we'd need it.

          I'm not completley understanding, but i don't need to: If it's easier/simpler (for now) to require that start/end/gap are all in the same currency that's fine – we should just test/document that clearly .. we can alwasy relax that restriction later if you think of a clean/easy way to do it.

          like i said before: it's probably silly to do it anyway, i just didn't understand if/what the complication was

          Show
          Hoss Man added a comment - I believe start/end currency equality is enforced by MoneyType.compareTo which will throw an exception when end is compared to the first (start+gap). Ah ..ok. and then ultimately start+gap is compared to end (even if hardend is false) so you'll get a exception then. ok fair enough. As far as enforcing currency equality being a good idea or not, it would make sense and I would prefer if start/end/gap currencies didn't need to be equal. This patch doesn't allow for that given the tradeoff of the utility of being able to use different currencies versus the annoyance of keeping a handle open to an ExchangeRateProvider in the places we'd need it. I'm not completley understanding, but i don't need to: If it's easier/simpler (for now) to require that start/end/gap are all in the same currency that's fine – we should just test/document that clearly .. we can alwasy relax that restriction later if you think of a clean/easy way to do it. like i said before: it's probably silly to do it anyway, i just didn't understand if/what the complication was
          Hide
          Jan Høydahl added a comment -

          Thanks for your work Andrew.

          Just a small comment about pathces. We prefer that you name the patch simply SOLR-3218.patch every time. JIRA takes care of greying out the older versions. Also, is it possible to convert your GIT patch to svn compatible patch?

          Good plan to jump over to CurrencyField once it is in trunk.

          Show
          Jan Høydahl added a comment - Thanks for your work Andrew. Just a small comment about pathces. We prefer that you name the patch simply SOLR-3218 .patch every time. JIRA takes care of greying out the older versions. Also, is it possible to convert your GIT patch to svn compatible patch? Good plan to jump over to CurrencyField once it is in trunk.
          Hide
          Jan Høydahl added a comment -

          CurrencyField is now in trunk

          Show
          Jan Høydahl added a comment - CurrencyField is now in trunk
          Hide
          Andrew Morrison added a comment -

          That was fast. I'll submit an updated patch soon.

          Show
          Andrew Morrison added a comment - That was fast. I'll submit an updated patch soon.
          Hide
          Andrew Morrison added a comment -

          The latest patch is updated to account for the CurrencyField name.

          I updated CurrencyValue.toString() to return "3.14,USD" for $3.14 rather than "314,USD". My feeling is that it's more straight forward to return strings that look like the values that were passed in to parse(). If we don't want toString to act as such, I can move that logic to CurrencyRangeEndpointCalculator.formatValue(). Also of interest, I'm injecting a '.' into the toString value of the long amount rather than using any existing floating point number formatters which might use a ',' rather than a '.' causing issues with the existing comma delimiter between the amount and currencyCode.

          Hoss,

          It was easy to allow the gap to be in any currency for which we have a conversion, so we're now allowing that. Start and end must still be of the same currency. I worry that relaxing the restriction on the gap may just be confusing without adding any real value. We may want to consider forcing gap to be the same as start and end so that things are more conceptually straight forward.

          Show
          Andrew Morrison added a comment - The latest patch is updated to account for the CurrencyField name. I updated CurrencyValue.toString() to return "3.14,USD" for $3.14 rather than "314,USD". My feeling is that it's more straight forward to return strings that look like the values that were passed in to parse(). If we don't want toString to act as such, I can move that logic to CurrencyRangeEndpointCalculator.formatValue(). Also of interest, I'm injecting a '.' into the toString value of the long amount rather than using any existing floating point number formatters which might use a ',' rather than a '.' causing issues with the existing comma delimiter between the amount and currencyCode. Hoss, It was easy to allow the gap to be in any currency for which we have a conversion, so we're now allowing that. Start and end must still be of the same currency. I worry that relaxing the restriction on the gap may just be confusing without adding any real value. We may want to consider forcing gap to be the same as start and end so that things are more conceptually straight forward.
          Hide
          Hoss Man added a comment -

          I updated CurrencyValue.toString() to return "3.14,USD" for $3.14 rather than "314,USD". My feeling is that it's more straight forward to return strings that look like the values that were passed in to parse().

          that sounds right.

          the most important thing is that in the response from range faceting, where it gives you a (str) lower bound about a count, that lower bound should be a legal value when building a query against that field (ie: you can use it in a range query) ... i'm pretty sure (if i understand correctly) that for CurrencyField that means "3.14,USD"

          I worry that relaxing the restriction on the gap may just be confusing without adding any real value. We may want to consider forcing gap to be the same as start and end so that things are more conceptually straight forward.

          I believe you – i've got no objection to locking that down, i just want to make sure that if we doc "you can't do this" that: a) the code actually fails if you try; and b) we have a test proving that the code will fail if you try.

          (and if we decide later that it makes sense, we can relax things and change the test & docs)

          Show
          Hoss Man added a comment - I updated CurrencyValue.toString() to return "3.14,USD" for $3.14 rather than "314,USD". My feeling is that it's more straight forward to return strings that look like the values that were passed in to parse(). that sounds right. the most important thing is that in the response from range faceting, where it gives you a (str) lower bound about a count, that lower bound should be a legal value when building a query against that field (ie: you can use it in a range query) ... i'm pretty sure (if i understand correctly) that for CurrencyField that means "3.14,USD" I worry that relaxing the restriction on the gap may just be confusing without adding any real value. We may want to consider forcing gap to be the same as start and end so that things are more conceptually straight forward. I believe you – i've got no objection to locking that down, i just want to make sure that if we doc "you can't do this" that: a) the code actually fails if you try; and b) we have a test proving that the code will fail if you try. (and if we decide later that it makes sense, we can relax things and change the test & docs)
          Hide
          Jan Høydahl added a comment -

          Updated patch

          • Patch is in correct format, starting form project root, not from solr
          • Fixes indexOutOfRange bug for start=0
          • Velocity GUI support
          • SolrJ support
          • Now puts plain String's in the NamedList instead of CurrencyValue's - easier to transport around

          The alternative to using Strings is to move CurrencyValue to solrj package, what do you think?

          Still needs many more test cases

          Show
          Jan Høydahl added a comment - Updated patch Patch is in correct format, starting form project root, not from solr Fixes indexOutOfRange bug for start=0 Velocity GUI support SolrJ support Now puts plain String's in the NamedList instead of CurrencyValue's - easier to transport around The alternative to using Strings is to move CurrencyValue to solrj package, what do you think? Still needs many more test cases
          Hide
          Yonik Seeley added a comment -

          The alternative to using Strings is to move CurrencyValue to solrj package, what do you think?

          Strings are good since it doesn't seem like we should push dependencies like this into solrj.

          Show
          Yonik Seeley added a comment - The alternative to using Strings is to move CurrencyValue to solrj package, what do you think? Strings are good since it doesn't seem like we should push dependencies like this into solrj.
          Hide
          Jan Høydahl added a comment -

          The conversion to String from CurrencyValue is a bit of a hack with instanceof tests, since I was not allowed to return String from getValue in CurrencyValue. Not very object oriented..

          Show
          Jan Høydahl added a comment - The conversion to String from CurrencyValue is a bit of a hack with instanceof tests, since I was not allowed to return String from getValue in CurrencyValue. Not very object oriented..
          Hide
          Lance Norskog added a comment -

          +1 for this feature. It makes the currency type 10x more compelling.

          Show
          Lance Norskog added a comment - +1 for this feature. It makes the currency type 10x more compelling.
          Hide
          Jan Høydahl added a comment -

          Hmm, this has been dormant for some while. Have anyone given the patch a spin? Anyone want to bring it up to date and get it closer to committing? This feature would probably benefit from Randomized testing, but if anyone else feels it's good enough for a first commit then such things could be added later.

          Show
          Jan Høydahl added a comment - Hmm, this has been dormant for some while. Have anyone given the patch a spin? Anyone want to bring it up to date and get it closer to committing? This feature would probably benefit from Randomized testing, but if anyone else feels it's good enough for a first commit then such things could be added later.
          Hide
          Andrew Morrison added a comment -

          I'm still around. If you think it'll get committed I can try and find some time to get it up to date.

          Show
          Andrew Morrison added a comment - I'm still around. If you think it'll get committed I can try and find some time to get it up to date.
          Hide
          Jan Høydahl added a comment -

          Great. I'll be gone for 3 weeks now, so if not anyone else finalizes+commits it in the meantime for 4.4, I'll help get it into 4.5.

          Show
          Jan Høydahl added a comment - Great. I'll be gone for 3 weeks now, so if not anyone else finalizes+commits it in the meantime for 4.4, I'll help get it into 4.5.
          Hide
          Steve Rowe added a comment -

          Bulk move 4.4 issues to 4.5 and 5.0

          Show
          Steve Rowe added a comment - Bulk move 4.4 issues to 4.5 and 5.0
          Hide
          Vitaliy Zhovtyuk added a comment -

          Updated to latest trunk. Added range facet tests to org.apache.solr.schema.AbstractCurrencyFieldTest, Moved org.apache.solr.schema.CurrencyValue back to separate class from nested class org.apache.solr.schema.CurrencyField since CurrencyValue used outside in org.apache.solr.request.SimpleFacets and other classes. Probably worth for wrap and encapsulate in org.apache.solr.schema.CurrencyField

          Show
          Vitaliy Zhovtyuk added a comment - Updated to latest trunk. Added range facet tests to org.apache.solr.schema.AbstractCurrencyFieldTest, Moved org.apache.solr.schema.CurrencyValue back to separate class from nested class org.apache.solr.schema.CurrencyField since CurrencyValue used outside in org.apache.solr.request.SimpleFacets and other classes. Probably worth for wrap and encapsulate in org.apache.solr.schema.CurrencyField
          Hide
          Vitaliy Zhovtyuk added a comment -

          Fixed some javadocs

          Show
          Vitaliy Zhovtyuk added a comment - Fixed some javadocs
          Hide
          Uwe Schindler added a comment -

          Move issue to Solr 4.9.

          Show
          Uwe Schindler added a comment - Move issue to Solr 4.9.

            People

            • Assignee:
              Unassigned
              Reporter:
              Jan Høydahl
            • Votes:
              4 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:

                Development