Solr
  1. Solr
  2. SOLR-7164

BBoxFieldType should not store values for subfields (by default)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.1, 6.0
    • Component/s: spatial
    • Labels:
      None

      Description

      When the bbox field creates the subfields, it uses the schema for 'double' and 'boolean' types.

      As is, we can specify these field types as indexed, not stored – but that is a bit trappy.

      Lets add a property to the field definition:

       storeSubFields="false"
      

      and register the subfields appropriatly

        Issue Links

          Activity

          Hide
          David Smiley added a comment -

          Did you mean to put this into the Solr project? Lets say you did. I don't think this requires a new attribute; we can simply honor the 'stored' of the subfield type.

          Just curious; what is the use-case?

          Show
          David Smiley added a comment - Did you mean to put this into the Solr project? Lets say you did. I don't think this requires a new attribute; we can simply honor the 'stored' of the subfield type. Just curious; what is the use-case?
          Hide
          Ryan McKinley added a comment -

          here is a patch

          the use case is to make users be explicit about wanting to store the sub fields. If you do store the fields, weird things happen with the 'update' feature – since it treats them as explicitly set values.

          As is, you can avoid this by creating a special field type that is not stored. but i think most users (at least most schemas I have seen) set the indexed/stored properties on fields, not field types

          Show
          Ryan McKinley added a comment - here is a patch the use case is to make users be explicit about wanting to store the sub fields. If you do store the fields, weird things happen with the 'update' feature – since it treats them as explicitly set values. As is, you can avoid this by creating a special field type that is not stored. but i think most users (at least most schemas I have seen) set the indexed/stored properties on fields, not field types
          Hide
          Ryan McKinley added a comment -

          Did you mean to put this into the Solr project?

          ya – moved it to solr

          Show
          Ryan McKinley added a comment - Did you mean to put this into the Solr project? ya – moved it to solr
          Hide
          David Smiley added a comment -

          If you do store the fields, weird things happen with the 'update' feature – since it treats them as explicitly set values.

          Before committing the patch; I'd like to better appreciate this problem – I don't understand it. Can you write a test that demonstrates the problem? Perhaps there is a fix that can address the root cause.

          I guess LatLonType & PointType would be similarly affected since they have sub-fields. Do you think so?

          Show
          David Smiley added a comment - If you do store the fields, weird things happen with the 'update' feature – since it treats them as explicitly set values. Before committing the patch; I'd like to better appreciate this problem – I don't understand it. Can you write a test that demonstrates the problem? Perhaps there is a fix that can address the root cause. I guess LatLonType & PointType would be similarly affected since they have sub-fields. Do you think so?
          Hide
          Ryan McKinley added a comment -

          the test that would fail is include in the patch:

          @Test
            public void testSpatialConfig() throws Exception {
              IndexSchema schema = h.getCoreInc().getLatestSchema();
          
              // BBox Config
              // Make sure the subfields are not stored
              SchemaField sub = schema.getField("bbox"+BBoxStrategy.SUFFIX_MINX);
              assertFalse(sub.stored());
              assertTrue(sub.indexed());
            }
          

          The out of the box 'fix' is just to define a 'doubleType' and 'booleanType' that is indexed but not stored. Totally possible, but easy to miss. I'm just suggesting we make it hard to miss, explicitly say "i want the values stored also"

          LatLonType & PointType use AbstractSubTypeFieldType... I don't fully understand that class, but I think it takes this into account. it looks like the subfields are forced to:

          Map<String, String> props = new HashMap<>();
              //Just set these, delegate everything else to the field type
              props.put("indexed", "true");
              props.put("stored", "false");
              props.put("multiValued", "false");
              int p = SchemaField.calcProps(name, type, props);
          
          Show
          Ryan McKinley added a comment - the test that would fail is include in the patch: @Test public void testSpatialConfig() throws Exception { IndexSchema schema = h.getCoreInc().getLatestSchema(); // BBox Config // Make sure the subfields are not stored SchemaField sub = schema.getField( "bbox" +BBoxStrategy.SUFFIX_MINX); assertFalse(sub.stored()); assertTrue(sub.indexed()); } The out of the box 'fix' is just to define a 'doubleType' and 'booleanType' that is indexed but not stored. Totally possible, but easy to miss. I'm just suggesting we make it hard to miss, explicitly say "i want the values stored also" LatLonType & PointType use AbstractSubTypeFieldType... I don't fully understand that class, but I think it takes this into account. it looks like the subfields are forced to: Map< String , String > props = new HashMap<>(); //Just set these, delegate everything else to the field type props.put( "indexed" , " true " ); props.put( "stored" , " false " ); props.put( "multiValued" , " false " ); int p = SchemaField.calcProps(name, type, props);
          Hide
          Ryan McKinley added a comment -

          I'd like to better appreciate this problem – I don't understand it

          are you asking about the 'update' issue? If so, i can just explain it – my real concern is really that I don't want to accidentally create stored fields!

          with the update feature, it reads out all stored fields and then writes them back to the index... so if you update the bbox, that would write the various subfields as if the original request had them. I'm not sure the exact behavior, but it seem unpredictable and will depend on field ordering etc. If the field is multi-valued it may work, but that is also an incorrect assumption for the normal case

          Show
          Ryan McKinley added a comment - I'd like to better appreciate this problem – I don't understand it are you asking about the 'update' issue? If so, i can just explain it – my real concern is really that I don't want to accidentally create stored fields! with the update feature, it reads out all stored fields and then writes them back to the index... so if you update the bbox, that would write the various subfields as if the original request had them. I'm not sure the exact behavior, but it seem unpredictable and will depend on field ordering etc. If the field is multi-valued it may work, but that is also an incorrect assumption for the normal case
          Hide
          David Smiley added a comment -

          Okay, I understand now and I like what you propose as seen in the patch with one exception. You forced the sub-field to be indexed. I don't think we should force that; the user might want docValues and to use BBoxField for relevancy and not for search/filtering.

          Show
          David Smiley added a comment - Okay, I understand now and I like what you propose as seen in the patch with one exception. You forced the sub-field to be indexed. I don't think we should force that; the user might want docValues and to use BBoxField for relevancy and not for search/filtering.
          Hide
          ASF subversion and git services added a comment -

          Commit 1662354 from Ryan McKinley in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1662354 ]

          SOLR-7164: BBoxFieldType defaults sub fields to not-stored

          Show
          ASF subversion and git services added a comment - Commit 1662354 from Ryan McKinley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1662354 ] SOLR-7164 : BBoxFieldType defaults sub fields to not-stored
          Hide
          Ryan McKinley added a comment -

          i removed the 'indexed' flag, but made sure it is single valued (multi-valued would do weird things!)

          Show
          Ryan McKinley added a comment - i removed the 'indexed' flag, but made sure it is single valued (multi-valued would do weird things!)
          Hide
          ASF subversion and git services added a comment -

          Commit 1662355 from Ryan McKinley in branch 'dev/trunk'
          [ https://svn.apache.org/r1662355 ]

          SOLR-7164: BBoxFieldType defaults sub fields to not-stored

          Show
          ASF subversion and git services added a comment - Commit 1662355 from Ryan McKinley in branch 'dev/trunk' [ https://svn.apache.org/r1662355 ] SOLR-7164 : BBoxFieldType defaults sub fields to not-stored
          Hide
          David Smiley added a comment -

          I noticed you added this:

          props &= ~MULTIVALUED; // must not be multivalued
          

          But I think we should detect and throw an exception instead – this field type fundamentally doesn't support it. Maaaybe a warning and not an exception. But as-is it's misconfigured in the schema.

          Also, BBoxField doesn't have the "Type" suffix in its name. This issue title and the CHANGES.txt is wrong.

          Show
          David Smiley added a comment - I noticed you added this: props &= ~MULTIVALUED; // must not be multivalued But I think we should detect and throw an exception instead – this field type fundamentally doesn't support it. Maaaybe a warning and not an exception. But as-is it's misconfigured in the schema. Also, BBoxField doesn't have the "Type" suffix in its name. This issue title and the CHANGES.txt is wrong.
          Hide
          ASF subversion and git services added a comment -

          Commit 1662356 from Ryan McKinley in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1662356 ]

          SOLR-7164: fix README for BBoxFieldType

          Show
          ASF subversion and git services added a comment - Commit 1662356 from Ryan McKinley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1662356 ] SOLR-7164 : fix README for BBoxFieldType
          Hide
          ASF subversion and git services added a comment -

          Commit 1662357 from Ryan McKinley in branch 'dev/trunk'
          [ https://svn.apache.org/r1662357 ]

          SOLR-7164: fix README for BBoxFieldType

          Show
          ASF subversion and git services added a comment - Commit 1662357 from Ryan McKinley in branch 'dev/trunk' [ https://svn.apache.org/r1662357 ] SOLR-7164 : fix README for BBoxFieldType
          Hide
          ASF subversion and git services added a comment -

          Commit 1662368 from shalin@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1662368 ]

          SOLR-7164: Fix failing TestSolr4Spatial by closing SolrCore

          Show
          ASF subversion and git services added a comment - Commit 1662368 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1662368 ] SOLR-7164 : Fix failing TestSolr4Spatial by closing SolrCore
          Hide
          ASF subversion and git services added a comment -

          Commit 1662369 from shalin@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1662369 ]

          SOLR-7164: Fix failing TestSolr4Spatial by closing SolrCore

          Show
          ASF subversion and git services added a comment - Commit 1662369 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1662369 ] SOLR-7164 : Fix failing TestSolr4Spatial by closing SolrCore
          Hide
          Ryan McKinley added a comment -
          Show
          Ryan McKinley added a comment - thanks Shalin Shekhar Mangar !
          Hide
          ASF subversion and git services added a comment -

          Commit 1672518 from Ryan McKinley in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1672518 ]

          SOLR-7164: update lucene field also

          Show
          ASF subversion and git services added a comment - Commit 1672518 from Ryan McKinley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1672518 ] SOLR-7164 : update lucene field also
          Hide
          ASF subversion and git services added a comment -

          Commit 1672532 from Ryan McKinley in branch 'dev/trunk'
          [ https://svn.apache.org/r1672532 ]

          Merged revision(s) 1672518 from lucene/dev/branches/branch_5x:
          SOLR-7164: update lucene field also
          ........

          Show
          ASF subversion and git services added a comment - Commit 1672532 from Ryan McKinley in branch 'dev/trunk' [ https://svn.apache.org/r1672532 ] Merged revision(s) 1672518 from lucene/dev/branches/branch_5x: SOLR-7164 : update lucene field also ........
          Hide
          Timothy Potter added a comment -

          Bulk close after 5.1 release

          Show
          Timothy Potter added a comment - Bulk close after 5.1 release

            People

            • Assignee:
              Ryan McKinley
              Reporter:
              Ryan McKinley
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development