Solr
  1. Solr
  2. SOLR-4842

Field faceting with local params affects successive field faceting parameters

    Details

      Description

      SOLR-4717 introduced local param support for per-field faceting, allowing the same field to be faceted in various ways. There's a problem such that one fields local param setting will override the defaults of the next field. For example:

      http://localhost:8983/solr/query?q=*:*&facet=true&rows=0&facet.field={!facet.prefix=a}name&facet.field=features

      where the facet.prefix for name affects the features faceting.

      1. SOLR-4842.patch
        1 kB
        Erik Hatcher
      2. SOLR-4842__hoss_tests.patch
        4 kB
        Hoss Man
      3. SOLR-4842.patch
        2 kB
        Erik Hatcher
      4. SOLR-4842.patch
        6 kB
        Erik Hatcher
      5. SOLR-4842.patch
        7 kB
        Hoss Man

        Activity

        Hide
        Erik Hatcher added a comment -

        Here's a patch demonstrating a failing test case that should pass, demonstrating the corruption of one facet.field's local params into another facet.field.

        Show
        Erik Hatcher added a comment - Here's a patch demonstrating a failing test case that should pass, demonstrating the corruption of one facet.field's local params into another facet.field.
        Hide
        Erik Hatcher added a comment -

        I'm not sure my patch actually shows the problem clearly yet. facet.mincount and deprecated facet.zeros support still in there confuse things a bit. I'm still working through a test case showing the issue clearly.

        Show
        Erik Hatcher added a comment - I'm not sure my patch actually shows the problem clearly yet. facet.mincount and deprecated facet.zeros support still in there confuse things a bit. I'm still working through a test case showing the issue clearly.
        Hide
        Hoss Man added a comment -

        Erik: based on your followup comment, i ignored your patch and attempted to write a test to reproduce the general problem you described and could not do so – see attached SOLR-4842__hoss_tests.patch.

        if there is a bug, i suspect it must be something subtle in the way the defaults of a particular param are defined. if you're having trouble writing a test aptch to demonstrate the problme you are seeing, can you at least describe a specific example query where you observe a problem? even if you can't share the docs needed to seee the problem, knowing exactly what params may help narrow things down.

        Show
        Hoss Man added a comment - Erik: based on your followup comment, i ignored your patch and attempted to write a test to reproduce the general problem you described and could not do so – see attached SOLR-4842 __hoss_tests.patch. if there is a bug, i suspect it must be something subtle in the way the defaults of a particular param are defined. if you're having trouble writing a test aptch to demonstrate the problme you are seeing, can you at least describe a specific example query where you observe a problem? even if you can't share the docs needed to seee the problem, knowing exactly what params may help narrow things down.
        Hide
        Hoss Man added a comment -

        Erik: ok, now looking at your test, i think it's just flawed.

        Ignore for a minute the issue of faceting multiple ways, ignore the "foo" key in the assertQ your patch modifies, ignore everything about it, delete it from the test, and just consider a query using only the "bar" key like so...

              assertQ("ignore foo, look at bar",
                      req("q", "id:[42 TO 47]"
                          ,"facet", "true"
                          ,"facet.zeros", "false"
                          ,"fq", "id:[42 TO 45]"
                          ,"facet.field", "{!key=bar " +
                             "facet.missing=true "+
                          "}"+fname
                          )
                      ,"*[count(//doc)=4]"
                      ,"*[count(//lst[@name='bar']/int)=5]"
                      ,"//lst[@name='bar']/int[not(@name)][.='1']"
                      );
        

        That test is still going to fail because facet.zeros=false but you are asserting that there should be 5 terms for "bar". the only way there could be 5 terms is if you include the terms with a zero.

        I don't think the docs have never really specified what happens if you mix and match "facet.mincount" with the deprecated "facet.zeros" (ie: "facet.mincount=1&facet.zeros=true&facet.field=XXX"), let alone in the case of per-field overrides (ie: "facet.mincount=1&f.XXX.facet.zeros=true&facet.field=XXX") – i think it's fair game to say all bets are off in the new situation of localparams. but in this specific case, there's no way it makes sense to think that the "bar" key should have a mincount of "0".

        Show
        Hoss Man added a comment - Erik: ok, now looking at your test, i think it's just flawed. Ignore for a minute the issue of faceting multiple ways, ignore the "foo" key in the assertQ your patch modifies, ignore everything about it, delete it from the test, and just consider a query using only the "bar" key like so... assertQ("ignore foo, look at bar", req("q", "id:[42 TO 47]" ,"facet", "true" ,"facet.zeros", "false" ,"fq", "id:[42 TO 45]" ,"facet.field", "{!key=bar " + "facet.missing=true "+ "}"+fname ) ,"*[count(//doc)=4]" ,"*[count(//lst[@name='bar']/int)=5]" ,"//lst[@name='bar']/int[not(@name)][.='1']" ); That test is still going to fail because facet.zeros=false but you are asserting that there should be 5 terms for "bar". the only way there could be 5 terms is if you include the terms with a zero. I don't think the docs have never really specified what happens if you mix and match "facet.mincount" with the deprecated "facet.zeros" (ie: "facet.mincount=1&facet.zeros=true&facet.field=XXX"), let alone in the case of per-field overrides (ie: "facet.mincount=1&f.XXX.facet.zeros=true&facet.field=XXX") – i think it's fair game to say all bets are off in the new situation of localparams. but in this specific case, there's no way it makes sense to think that the "bar" key should have a mincount of "0".
        Hide
        Erik Hatcher added a comment -

        Here's a patch that demonstrates the issue clearly. The actual problem reported to me (via a customer) was about facet.prefix, but I tried to distill it to the mincount initially missing the weird mincount/zeros interaction.

        I've added two test cases, one that passes as-is, the second fails, and the only difference is the order of the two facet.field params, one using a localparam of facet.prefix, the other not.

        Show
        Erik Hatcher added a comment - Here's a patch that demonstrates the issue clearly. The actual problem reported to me (via a customer) was about facet.prefix, but I tried to distill it to the mincount initially missing the weird mincount/zeros interaction. I've added two test cases, one that passes as-is, the second fails, and the only difference is the order of the two facet.field params, one using a localparam of facet.prefix, the other not.
        Hide
        Erik Hatcher added a comment - - edited

        Here's a patch incorporating Hoss' additional tests as well as my failing test and a fix in SimpleFacets in #parseParams logic when localParams == null which caused carrying over of previous local params.

        I'm running the full test suite now and will await Hoss' review (and maybe Ryan's eyes too?) before committing.

        Show
        Erik Hatcher added a comment - - edited Here's a patch incorporating Hoss' additional tests as well as my failing test and a fix in SimpleFacets in #parseParams logic when localParams == null which caused carrying over of previous local params. I'm running the full test suite now and will await Hoss' review (and maybe Ryan's eyes too?) before committing.
        Hide
        Erik Hatcher added a comment -

        I'm running the full test suite now and will await Hoss' review (and maybe Ryan's eyes too?) before committing.

        Fresh branch_4x checkout with just this patch attached, "ant test" (from solr/ directory) gives me the following failures:

        [junit4:junit4] Tests with failures:
        [junit4:junit4]   - org.apache.solr.cloud.CollectionsAPIDistributedZkTest.testDistribSearch
        [junit4:junit4]   - org.apache.solr.cloud.ZkCLITest.testUpConfigLinkConfigClearZk
        

        I can only presume this is unrelated to this patch.

        Show
        Erik Hatcher added a comment - I'm running the full test suite now and will await Hoss' review (and maybe Ryan's eyes too?) before committing. Fresh branch_4x checkout with just this patch attached, "ant test" (from solr/ directory) gives me the following failures: [junit4:junit4] Tests with failures: [junit4:junit4] - org.apache.solr.cloud.CollectionsAPIDistributedZkTest.testDistribSearch [junit4:junit4] - org.apache.solr.cloud.ZkCLITest.testUpConfigLinkConfigClearZk I can only presume this is unrelated to this patch.
        Hide
        Yonik Seeley added a comment -

        Confirmed bug w/ simple test on example data:

        http://localhost:8983/solr/query?q=*:*&facet=true&rows=0&facet.field={!facet.prefix=a}name&facet.field=features
        

        The features constraints returned all start with "a" only.

        Show
        Yonik Seeley added a comment - Confirmed bug w/ simple test on example data: http: //localhost:8983/solr/query?q=*:*&facet= true &rows=0&facet.field={!facet.prefix=a}name&facet.field=features The features constraints returned all start with "a" only.
        Hide
        Erik Hatcher added a comment -

        adjusted example with Yonik's example since mincount is a red herring here.

        Show
        Erik Hatcher added a comment - adjusted example with Yonik's example since mincount is a red herring here.
        Hide
        Hoss Man added a comment -

        Ugh ... i didn't realize how stateful param parsing had gotten i nthe faceting code.

        Erik's last patch had at least one bug remaining that affected range & date faceting because of the need to reset the "required" params.

        attached patch seems to fix that ... i don't see any other obvious bugs.

        Show
        Hoss Man added a comment - Ugh ... i didn't realize how stateful param parsing had gotten i nthe faceting code. Erik's last patch had at least one bug remaining that affected range & date faceting because of the need to reset the "required" params. attached patch seems to fix that ... i don't see any other obvious bugs.
        Hide
        Yonik Seeley added a comment -

        Patch looks good, commit it!

        Show
        Yonik Seeley added a comment - Patch looks good, commit it!
        Hide
        Erik Hatcher added a comment -

        Ugh ... i didn't realize how stateful param parsing had gotten i nthe faceting code.

        Yeah, icky it is.

        Erik's last patch had at least one bug remaining that affected range & date faceting because of the need to reset the "required" params.

        Hmmm... did you see a test failure? I was wondering if that change you made was needed at the time but no tests were failing. I added your one-line tweak in there. If you added test cases in your patch I didn't catch that yet.

        Committed to branch_4x and trunk. Is there any other branch where this should go?

        Show
        Erik Hatcher added a comment - Ugh ... i didn't realize how stateful param parsing had gotten i nthe faceting code. Yeah, icky it is. Erik's last patch had at least one bug remaining that affected range & date faceting because of the need to reset the "required" params. Hmmm... did you see a test failure? I was wondering if that change you made was needed at the time but no tests were failing. I added your one-line tweak in there. If you added test cases in your patch I didn't catch that yet. Committed to branch_4x and trunk. Is there any other branch where this should go?
        Hide
        Steve Rowe added a comment -

        Committed to branch_4x and trunk. Is there any other branch where this should go?

        Erik, you set the fix version to 4.3.1, and in order for that to happen, this has to be committed on the lucene_solr_4_3 branch, no?

        Show
        Steve Rowe added a comment - Committed to branch_4x and trunk. Is there any other branch where this should go? Erik, you set the fix version to 4.3.1, and in order for that to happen, this has to be committed on the lucene_solr_4_3 branch, no?
        Hide
        Yonik Seeley added a comment -

        I set the fix version Erik, please do commit to the 43 branch.

        Show
        Yonik Seeley added a comment - I set the fix version Erik, please do commit to the 43 branch.
        Hide
        Steve Rowe added a comment - - edited

        I set the fix version Erik, please do commit to the 43 branch.

        - sorry guys, I looked at JIRA's All view and misinterpreted Erik's adding 5.0 as a fix version 2 hours ago as also adding 4.3.1, since it appears as if no fix version was previously set (empty left-hand column), and both 4.3.1 and 5.0 are in the right-hand column.

        Show
        Steve Rowe added a comment - - edited I set the fix version Erik, please do commit to the 43 branch. - sorry guys, I looked at JIRA's All view and misinterpreted Erik's adding 5.0 as a fix version 2 hours ago as also adding 4.3.1, since it appears as if no fix version was previously set (empty left-hand column), and both 4.3.1 and 5.0 are in the right-hand column.
        Hide
        Hoss Man added a comment -

        I was wondering if that change you made was needed at the time but no tests were failing.

        because there were no tests using range on the same field multiple ways mixed with local params on only one of hte range facets. ie: same bug you found with facet.prefix, but with any of hte facet.range.* params.

        I added your one-line tweak in there. If you added test cases in your patch I didn't catch that yet.

        yes, the patch had a additional tests demonstrating the reason why reseting "required" was needed.

        Show
        Hoss Man added a comment - I was wondering if that change you made was needed at the time but no tests were failing. because there were no tests using range on the same field multiple ways mixed with local params on only one of hte range facets. ie: same bug you found with facet.prefix, but with any of hte facet.range.* params. I added your one-line tweak in there. If you added test cases in your patch I didn't catch that yet. yes, the patch had a additional tests demonstrating the reason why reseting "required" was needed.
        Hide
        Erik Hatcher added a comment -

        Additional test case from Hoss picked up and committed. Committed to trunk branch_4x, and lucene_solr_4_3

        Show
        Erik Hatcher added a comment - Additional test case from Hoss picked up and committed. Committed to trunk branch_4x, and lucene_solr_4_3
        Hide
        Shalin Shekhar Mangar added a comment -

        Bulk close after 4.3.1 release

        Show
        Shalin Shekhar Mangar added a comment - Bulk close after 4.3.1 release

          People

          • Assignee:
            Erik Hatcher
            Reporter:
            Erik Hatcher
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development