Solr
  1. Solr
  2. SOLR-3595

Currency types do not support range queries when multiValued

    Details

      Description

      You can define the currency type as multiValued. However, if you do (and have more than one value), range queries, at least, do not work. See the thread titled "Filtering a query by range returning unexpected results".

      I'm not at all sure that currency type should support multivalued. For instance, how would one handle storing multiple values for a currency type in different currencies (e.g. USD and EUR)? I don't know enough about the internals to understand if it's possible, this JIRA is the result of a question on the users list.

      If we decide that currency should not support multiValued, it seems a check at startup is in order on the "fail early, fail loudly" principle.

        Activity

        Hide
        Uwe Schindler added a comment -

        Multivalued does not work with fields that consist of more than one Lucene field behind, because the field vales cannot be corelated. We should disallow multi value and throw exception. This is similar to LatLong fields I think.

        Show
        Uwe Schindler added a comment - Multivalued does not work with fields that consist of more than one Lucene field behind, because the field vales cannot be corelated. We should disallow multi value and throw exception. This is similar to LatLong fields I think.
        Hide
        Jan Høydahl added a comment -

        +1
        Disallowing multivalue sounds like the right approach, then if anyone figures out how to support it in the future it can be done then, not now.

        Show
        Jan Høydahl added a comment - +1 Disallowing multivalue sounds like the right approach, then if anyone figures out how to support it in the future it can be done then, not now.
        Hide
        Hoss Man added a comment -

        bulk fixing the version info for 4.0-ALPHA and 4.0 all affected issues have "hoss20120711-bulk-40-change" in comment

        Show
        Hoss Man added a comment - bulk fixing the version info for 4.0-ALPHA and 4.0 all affected issues have "hoss20120711-bulk-40-change" in comment
        Hide
        Erick Erickson added a comment -

        This can be done pretty easily by overriding isMultiValued in CurrencyField and hard-coding returning false. Seems a little harsh and is confusing. I can define a field with multiValued="true" and then get an error saying "multiple values encountered for non multiValued field....." Huh?.

        One way to handle this would be to also provide a default method in FieldType, something about "getNonMultivaluedMsg()" (what a bad name!) and overriding that too. Kludgy but minimal code.

        We can also just change the test around line 241 in DocumentBuilder to check for isPolyField, but I'd have to wash my hands afterward...
        replace
        if( sfield!=null && !sfield.multiValued() && field.getValueCount() > 1 ) {
        with
        if( sfield!=null && (!sfield.multiValued() || sfield.isPolyField()) && field.getValueCount() > 1 ) {
        This kind of awful hack would probably be really bad for polygons in spatial, but I haven't looked.

        I'd prefer to "fail early, fail often" and catch this when the schema file was being parsed, but I don't see a convenient place to do that. Am I overlooking the obvious again?

        Or is there another way to approach this altogether?

        Show
        Erick Erickson added a comment - This can be done pretty easily by overriding isMultiValued in CurrencyField and hard-coding returning false. Seems a little harsh and is confusing. I can define a field with multiValued="true" and then get an error saying "multiple values encountered for non multiValued field....." Huh?. One way to handle this would be to also provide a default method in FieldType, something about "getNonMultivaluedMsg()" (what a bad name!) and overriding that too. Kludgy but minimal code. We can also just change the test around line 241 in DocumentBuilder to check for isPolyField, but I'd have to wash my hands afterward... replace if( sfield!=null && !sfield.multiValued() && field.getValueCount() > 1 ) { with if( sfield!=null && (!sfield.multiValued() || sfield.isPolyField()) && field.getValueCount() > 1 ) { This kind of awful hack would probably be really bad for polygons in spatial, but I haven't looked. I'd prefer to "fail early, fail often" and catch this when the schema file was being parsed, but I don't see a convenient place to do that. Am I overlooking the obvious again? Or is there another way to approach this altogether?
        Hide
        Hoss Man added a comment -

        I'd prefer to "fail early, fail often" and catch this when the schema file was being parsed, but I don't see a convenient place to do that. Am I overlooking the obvious again?

        can't we just put hte logic in CurrencyField.init(), after the call to super.init() ...

        if (this.isMultiValued()) { 
         throw new SolrException("CurrencyField's can not be multivalued: " + this.typeName) 
        } 
        

        ?

        (see BadIndexSchemaTest for an example of how to test that it fails properly)

        Show
        Hoss Man added a comment - I'd prefer to "fail early, fail often" and catch this when the schema file was being parsed, but I don't see a convenient place to do that. Am I overlooking the obvious again? can't we just put hte logic in CurrencyField.init(), after the call to super.init() ... if ( this .isMultiValued()) { throw new SolrException( "CurrencyField's can not be multivalued: " + this .typeName) } ? (see BadIndexSchemaTest for an example of how to test that it fails properly)
        Hide
        Robert Muir added a comment -

        rmuir20120906-bulk-40-change

        Show
        Robert Muir added a comment - rmuir20120906-bulk-40-change
        Hide
        Hoss Man added a comment -

        assigning to myself to try my suggestion when i have some more time, anyone with an itch to deal with this should not hesitate to take it from me

        Show
        Hoss Man added a comment - assigning to myself to try my suggestion when i have some more time, anyone with an itch to deal with this should not hesitate to take it from me
        Hide
        Hoss Man added a comment -

        My suggestion addressed the situation of trying to decalre the fieldType as multiValued, but did nothing to address fields & dynamic fields that were explicitly declared multiValued.

        So in the attached patch i introduce a new method into the FieldType api called "checkSchemaField" which the SchemaField constructor uses to let the FieldType check the instance for errors that violate any invariants about hte field type. the default impl is a NOOP and CurrencyField overrides this.

        Would like someone to sanity check that this approach is a good idea (seems like it would definitely have other uses moving forward) otherwise i think it's feasible solution to the currency multivalued problem and we should get it in for 4.0

        Show
        Hoss Man added a comment - My suggestion addressed the situation of trying to decalre the fieldType as multiValued, but did nothing to address fields & dynamic fields that were explicitly declared multiValued. So in the attached patch i introduce a new method into the FieldType api called "checkSchemaField" which the SchemaField constructor uses to let the FieldType check the instance for errors that violate any invariants about hte field type. the default impl is a NOOP and CurrencyField overrides this. Would like someone to sanity check that this approach is a good idea (seems like it would definitely have other uses moving forward) otherwise i think it's feasible solution to the currency multivalued problem and we should get it in for 4.0
        Hide
        Yonik Seeley added a comment -

        Relatively unobtrusive... seems fine to me.

        Show
        Yonik Seeley added a comment - Relatively unobtrusive... seems fine to me.
        Hide
        Hoss Man added a comment -

        r1383648 - 4x
        r1383653 - trunk

        Show
        Hoss Man added a comment - r1383648 - 4x r1383653 - trunk
        Hide
        Hoss Man added a comment -

        Annnnnnnnd i had the wrong issue# in changes, now fixed...

        Committed revision 1383654.
        Committed revision 1383655.

        Show
        Hoss Man added a comment - Annnnnnnnd i had the wrong issue# in changes, now fixed... Committed revision 1383654. Committed revision 1383655.
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Chris M. Hostetter
        http://svn.apache.org/viewvc?view=revision&revision=1383655

        SOLR-3595: fix incorrect issue number in CHANGES.txt (merge r1383654)

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Chris M. Hostetter http://svn.apache.org/viewvc?view=revision&revision=1383655 SOLR-3595 : fix incorrect issue number in CHANGES.txt (merge r1383654)
        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.

          People

          • Assignee:
            Hoss Man
            Reporter:
            Erick Erickson
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development