Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-10155

Clarify logic for term filters on numeric types

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 6.4.1
    • Fix Version/s: 6.5, 7.0
    • Component/s: faceting
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      The following code has been found to be confusing to multiple folks working in SimpleFacets.java (see SOLR-10132)

      if (termFilter != null) {
                    // TODO: understand this logic... what is the case for supporting an empty string
                    // for contains on numeric facets? What does that achieve?
                    // The exception message is misleading in the case of an excludeTerms filter in any case...
                    // Also maybe vulnerable to NPE on isEmpty test?
                    final boolean supportedOperation = (termFilter instanceof SubstringBytesRefFilter) && ((SubstringBytesRefFilter) termFilter).substring().isEmpty();
                    if (!supportedOperation) {
                      throw new SolrException(ErrorCode.BAD_REQUEST, FacetParams.FACET_CONTAINS + " is not supported on numeric types");
                    }
                  }
      

      This is found around line 482 or so. The comment in the code above is mine, and won't be found in the codebase. This ticket can be resolved by eliminating the complex check and just denying all termFilters with a better exception message not specific to contains filters (and perhaps consolidated with the proceeding check for about prefix filters?), or adding a comment to the code base explaining why we need to allow a term filter with an empty, non-null string to be processed, and why this isn't an NPE waiting to happen.

      1. SOLR-10155.patch
        2 kB
        Christine Poerschke
      2. SOLR-10155.patch
        1 kB
        Christine Poerschke

        Issue Links

          Activity

          Hide
          cpoerschke Christine Poerschke added a comment -

          Attaching proposed patch.

          Alan Woodward - would you have any thoughts on this change? SOLR-1387 added facet.contains way back in March 2015. Thanks.

          Show
          cpoerschke Christine Poerschke added a comment - Attaching proposed patch. Alan Woodward - would you have any thoughts on this change? SOLR-1387 added facet.contains way back in March 2015. Thanks.
          Hide
          romseygeek Alan Woodward added a comment -

          +1, regex matching on numeric facets doesn't really make sense

          Show
          romseygeek Alan Woodward added a comment - +1, regex matching on numeric facets doesn't really make sense
          Hide
          gus_heck Gus Heck added a comment - - edited

          I think the pattern actually started with facet.prefix at the time DocValues was added by Adrien Grand in SOLR-3855 in 2013...

          https://github.com/apache/lucene-solr/commit/e61398084d3f1ca0f28c5c35d3318645d7a401ec#diff-5ac9dc7b128b4dd99b764060759222b2R381

          The only question I have is whether there's a use case for passing blanks through... perhaps some situation in which facet.prefix or facet.contains would be robotically added and supplying a blank is the means of "turning it off" without blowing up? Maybe some component might do such a thing?

          Show
          gus_heck Gus Heck added a comment - - edited I think the pattern actually started with facet.prefix at the time DocValues was added by Adrien Grand in SOLR-3855 in 2013... https://github.com/apache/lucene-solr/commit/e61398084d3f1ca0f28c5c35d3318645d7a401ec#diff-5ac9dc7b128b4dd99b764060759222b2R381 The only question I have is whether there's a use case for passing blanks through... perhaps some situation in which facet.prefix or facet.contains would be robotically added and supplying a blank is the means of "turning it off" without blowing up? Maybe some component might do such a thing?
          Hide
          cpoerschke Christine Poerschke added a comment -

          ... whether there's a use case for passing blanks through ... supplying a blank is the means of "turning it off" without blowing up ...

          That's a fair point, yes, the change in behaviour would have to be documented clearly in the CHANGES.txt e.g. something along the lines of "facet.contains= is now rejected for numeric types".

          So then, yes, would it make sense to apply the same change to facet.prefix with a joint "facet.contains= and facet.prefix= are now rejected for numeric types" CHANGES.txt note?

          Adrien Grand - would you have any thoughts on this, following on from the (long time ago) SOLR-3855 commit Gus mentioned above?

          Show
          cpoerschke Christine Poerschke added a comment - ... whether there's a use case for passing blanks through ... supplying a blank is the means of "turning it off" without blowing up ... That's a fair point, yes, the change in behaviour would have to be documented clearly in the CHANGES.txt e.g. something along the lines of "facet.contains= is now rejected for numeric types" . So then, yes, would it make sense to apply the same change to facet.prefix with a joint "facet.contains= and facet.prefix= are now rejected for numeric types" CHANGES.txt note? Adrien Grand - would you have any thoughts on this, following on from the (long time ago) SOLR-3855 commit Gus mentioned above?
          Hide
          jpountz Adrien Grand added a comment -

          +1 to explicitly reject facet.contains and facet.prefix on numerics with a clear error message

          Show
          jpountz Adrien Grand added a comment - +1 to explicitly reject facet.contains and facet.prefix on numerics with a clear error message
          Hide
          cpoerschke Christine Poerschke added a comment -

          Revised patch to include facet.prefix rejection for numerics.

          Show
          cpoerschke Christine Poerschke added a comment - Revised patch to include facet.prefix rejection for numerics.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 43474312eb2b66df4102bd653b9546e7eff47363 in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4347431 ]

          SOLR-10155: For numeric types facet.contains= and facet.prefix= are now rejected.
          (Gus Heck, Christine Poerschke)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 43474312eb2b66df4102bd653b9546e7eff47363 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4347431 ] SOLR-10155 : For numeric types facet.contains= and facet.prefix= are now rejected. (Gus Heck, Christine Poerschke)
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 54731019085cef2fb9499d4c872bd7aa29456ff3 in lucene-solr's branch refs/heads/branch_6x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5473101 ]

          SOLR-10155: For numeric types facet.contains= and facet.prefix= are now rejected.
          (Gus Heck, Christine Poerschke)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 54731019085cef2fb9499d4c872bd7aa29456ff3 in lucene-solr's branch refs/heads/branch_6x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5473101 ] SOLR-10155 : For numeric types facet.contains= and facet.prefix= are now rejected. (Gus Heck, Christine Poerschke)
          Hide
          cpoerschke Christine Poerschke added a comment -

          Thanks everyone!

          Show
          cpoerschke Christine Poerschke added a comment - Thanks everyone!

            People

            • Assignee:
              cpoerschke Christine Poerschke
              Reporter:
              gus_heck Gus Heck
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development