Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.0, 6.1, master (7.0)
    • Component/s: core/search
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      See LUCENE-6531
      Mutable queries are an issue for automatic filter caching since modifying a query after it has been put into the cache will corrupt the cache. We should make all queries immutable (up to the boost) to avoid this issue.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user LucVL opened a pull request:

          https://github.com/apache/lucene-solr/pull/19

          LUCENE-7064: Make MultiPhraseQuery immutable

          This PR is based at the highest commit point common to master, branch_6x and branch_6_0 so it should be easy to merge with any or all of them.

          My original mail to the dev list doesn't seem to get through, so I repeat it here:

          > While checking how to migrate my custom components from lucene/solr 5.1 to 6.x I stumbled upon the fact that oal.search.MultiPhraseQuery is not immutable like most other implementations (see e.g.: https://issues.apache.org/jira/browse/LUCENE-6531)
          >
          > Since it is part of the public API I would suggest splitting it in an immutable class and a builder like was done for most other Queries before releasing an official 6.x version.
          >
          > I did a quick scan through all derived classes of Query and I compiled the following list (ignoring sources in test or contrib folders)
          > Some of them are already marked as experimental (but should perhaps receive the "official" @lucene.experimental tag ?)
          > For some it's possibly not an issue since they should never end up in a filter cache (like MoreLikeThisQuery ?), but then a comment specifying the exception to the rule should perhaps be added.
          >
          > I'll probably already have a go at the MultiPhraseQuery case shortly and create a JIRA issue with a PR for it.
          >
          > Luc Vanlerberghe
          >
          > lucene/search:
          > - org.apache.lucene.search.MultiPhraseQuery
          >
          > lucene/queries:
          > - org.apache.lucene.queries.CommonTermsQuery
          > - org.apache.lucene.queries.CustomScoreQuery (marked as @lucene.experimental)
          > - org.apache.lucene.queries.mlt.MoreLikeThisQuery
          >
          > lucene/suggest:
          > - org.apache.lucene.search.suggest.document.ContextQuery (marked as @lucene.experimental)
          >
          > lucene/facet:
          > - org.apache.lucene.facet.DrillDownQuery (marked as @lucene.experimental)
          >
          > solr/core:
          > - org.apache.solr.search.ExtendedQueryBase
          > Several derived classes, among which:
          > - org.apache.solr.query.FilterQuery
          > - org.apache.solr.query.SolrRangeQuery (marked as @lucene.experimental)
          > - org.apache.solr.search.RankQuery (marked in comment as experimental, but not its derived classes)
          > - org.apache.solr.search.WrappedQuery
          > - org.apache.solr.search.join.GraphQuery (marked as @lucene.experimental)
          > - org.apache.solr.search.SolrConstantScoreQuery (marked in comment as experimental, but not the derived FunctionRangeQuery)
          >

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/LucVL/lucene-solr lucene-7064

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/lucene-solr/pull/19.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #19


          commit 223eaa2bcf74ea3b363abfb161b3771b09f48281
          Author: Luc Vanlerberghe <luc.vanlerberghe@bvdinfo.com>
          Date: 2016-03-03T10:52:14Z

          LUCENE-7064: Split MultiPhraseQuery into an immutable class and a Builder

          commit 1b4c0ed77fe8b22c7a04b2879b34514b0ac05d0d
          Author: Luc Vanlerberghe <luc.vanlerberghe@bvdinfo.com>
          Date: 2016-03-03T13:02:51Z

          LUCENE-7064: Updated tests


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user LucVL opened a pull request: https://github.com/apache/lucene-solr/pull/19 LUCENE-7064 : Make MultiPhraseQuery immutable This PR is based at the highest commit point common to master, branch_6x and branch_6_0 so it should be easy to merge with any or all of them. My original mail to the dev list doesn't seem to get through, so I repeat it here: > While checking how to migrate my custom components from lucene/solr 5.1 to 6.x I stumbled upon the fact that oal.search.MultiPhraseQuery is not immutable like most other implementations (see e.g.: https://issues.apache.org/jira/browse/LUCENE-6531 ) > > Since it is part of the public API I would suggest splitting it in an immutable class and a builder like was done for most other Queries before releasing an official 6.x version. > > I did a quick scan through all derived classes of Query and I compiled the following list (ignoring sources in test or contrib folders) > Some of them are already marked as experimental (but should perhaps receive the "official" @lucene.experimental tag ?) > For some it's possibly not an issue since they should never end up in a filter cache (like MoreLikeThisQuery ?), but then a comment specifying the exception to the rule should perhaps be added. > > I'll probably already have a go at the MultiPhraseQuery case shortly and create a JIRA issue with a PR for it. > > Luc Vanlerberghe > > lucene/search: > - org.apache.lucene.search.MultiPhraseQuery > > lucene/queries: > - org.apache.lucene.queries.CommonTermsQuery > - org.apache.lucene.queries.CustomScoreQuery (marked as @lucene.experimental) > - org.apache.lucene.queries.mlt.MoreLikeThisQuery > > lucene/suggest: > - org.apache.lucene.search.suggest.document.ContextQuery (marked as @lucene.experimental) > > lucene/facet: > - org.apache.lucene.facet.DrillDownQuery (marked as @lucene.experimental) > > solr/core: > - org.apache.solr.search.ExtendedQueryBase > Several derived classes, among which: > - org.apache.solr.query.FilterQuery > - org.apache.solr.query.SolrRangeQuery (marked as @lucene.experimental) > - org.apache.solr.search.RankQuery (marked in comment as experimental, but not its derived classes) > - org.apache.solr.search.WrappedQuery > - org.apache.solr.search.join.GraphQuery (marked as @lucene.experimental) > - org.apache.solr.search.SolrConstantScoreQuery (marked in comment as experimental, but not the derived FunctionRangeQuery) > You can merge this pull request into a Git repository by running: $ git pull https://github.com/LucVL/lucene-solr lucene-7064 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/lucene-solr/pull/19.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #19 commit 223eaa2bcf74ea3b363abfb161b3771b09f48281 Author: Luc Vanlerberghe <luc.vanlerberghe@bvdinfo.com> Date: 2016-03-03T10:52:14Z LUCENE-7064 : Split MultiPhraseQuery into an immutable class and a Builder commit 1b4c0ed77fe8b22c7a04b2879b34514b0ac05d0d Author: Luc Vanlerberghe <luc.vanlerberghe@bvdinfo.com> Date: 2016-03-03T13:02:51Z LUCENE-7064 : Updated tests
          Hide
          lvl Luc Vanlerberghe added a comment -

          I just realized I pass the arrays from the builder to the query as is without copying them, so any modification to the builder modifies the query as well.
          I will update the PR in a moment...

          Show
          lvl Luc Vanlerberghe added a comment - I just realized I pass the arrays from the builder to the query as is without copying them, so any modification to the builder modifies the query as well. I will update the PR in a moment...
          Hide
          lvl Luc Vanlerberghe added a comment -

          I updated the PR so modifying a Builder no longer affects already created queries
          I aligned the signatures of getTermArrays and getPositions with the ones in PhraseQuery.

          From Adrien Grand:

          Actually I think returning a list is better: with arrays you need to perform a deep copy if you want to make sure that the user cannot change the internal state of the query. We could keep arrays internally and call Collections.unmodifiableList(Arrays.asList(termArrays)) when returning the terms to the user?

          For the getTermArrays case that would involve creating wrappers of wrappers since it returns a two-dimensional array.

          For now I just added a "Do not modify!" in the doc comment...

          Show
          lvl Luc Vanlerberghe added a comment - I updated the PR so modifying a Builder no longer affects already created queries I aligned the signatures of getTermArrays and getPositions with the ones in PhraseQuery. From Adrien Grand: Actually I think returning a list is better: with arrays you need to perform a deep copy if you want to make sure that the user cannot change the internal state of the query. We could keep arrays internally and call Collections.unmodifiableList(Arrays.asList(termArrays)) when returning the terms to the user? For the getTermArrays case that would involve creating wrappers of wrappers since it returns a two-dimensional array. For now I just added a "Do not modify!" in the doc comment...
          Hide
          jpountz Adrien Grand added a comment -

          OK, I agree consistency with PhraseQuery is good.

          Show
          jpountz Adrien Grand added a comment - OK, I agree consistency with PhraseQuery is good.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 40aab73a4c997410fc2c1b8b919e5a30e49b1ee5 in lucene-solr's branch refs/heads/master from Luc Vanlerberghe
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=40aab73 ]

          LUCENE-7064: Split MultiPhraseQuery into an immutable class and a Builder

          This closes #19

          Show
          jira-bot ASF subversion and git services added a comment - Commit 40aab73a4c997410fc2c1b8b919e5a30e49b1ee5 in lucene-solr's branch refs/heads/master from Luc Vanlerberghe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=40aab73 ] LUCENE-7064 : Split MultiPhraseQuery into an immutable class and a Builder This closes #19
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/lucene-solr/pull/19

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/lucene-solr/pull/19
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit a78ef1584234a0a831f294f8ac2ebf9e8a7026fb in lucene-solr's branch refs/heads/master from Adrien Grand
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a78ef15 ]

          LUCENE-7064: Add attribution.

          Show
          jira-bot ASF subversion and git services added a comment - Commit a78ef1584234a0a831f294f8ac2ebf9e8a7026fb in lucene-solr's branch refs/heads/master from Adrien Grand [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a78ef15 ] LUCENE-7064 : Add attribution.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1407077092f3b6d50fbecf07eda3f739f135455d in lucene-solr's branch refs/heads/branch_6_0 from Luc Vanlerberghe
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1407077 ]

          LUCENE-7064: Split MultiPhraseQuery into an immutable class and a Builder

          This closes #19

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1407077092f3b6d50fbecf07eda3f739f135455d in lucene-solr's branch refs/heads/branch_6_0 from Luc Vanlerberghe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1407077 ] LUCENE-7064 : Split MultiPhraseQuery into an immutable class and a Builder This closes #19
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 948a388778d56cfa1c9fbd3c9ed44f93136bf943 in lucene-solr's branch refs/heads/branch_6x from Luc Vanlerberghe
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=948a388 ]

          LUCENE-7064: Split MultiPhraseQuery into an immutable class and a Builder

          This closes #19

          Show
          jira-bot ASF subversion and git services added a comment - Commit 948a388778d56cfa1c9fbd3c9ed44f93136bf943 in lucene-solr's branch refs/heads/branch_6x from Luc Vanlerberghe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=948a388 ] LUCENE-7064 : Split MultiPhraseQuery into an immutable class and a Builder This closes #19
          Hide
          jpountz Adrien Grand added a comment -

          Pushed. Thanks Luc!

          Show
          jpountz Adrien Grand added a comment - Pushed. Thanks Luc!
          Hide
          hossman Hoss Man added a comment -

          Manually correcting fixVersion per Step #S6 of LUCENE-7271

          Show
          hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S6 of LUCENE-7271

            People

            • Assignee:
              Unassigned
              Reporter:
              lvl Luc Vanlerberghe
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development