Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 7.0, 6.5
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      TermsQuery current sits in the queries module, but it's used in both spatial-extras and in facets, and currently is the only reason that the facets module has a dependency on queries. I think it's a generally useful query, and would fit in perfectly well in core.

      This would also allow us to explore rewriting BooleanQuery to TermsQuery to avoid the max-clauses limit

      1. LUCENE-7624.patch
        54 kB
        Alan Woodward

        Issue Links

          Activity

          Hide
          cpoerschke Christine Poerschke added a comment -

          ... I think it's a generally useful query, and would fit in perfectly well in core.

          I agree. Already (singular) TermQuery is in core and it would be logical for (plural) TermsQuery to be in the same module. Historically (pre LUCENE-6270) TermsQuery used to be TermsFilter which perhaps explains its location in the queries rather than the core module.

          Show
          cpoerschke Christine Poerschke added a comment - ... I think it's a generally useful query, and would fit in perfectly well in core. I agree. Already (singular) TermQuery is in core and it would be logical for (plural) TermsQuery to be in the same module. Historically (pre LUCENE-6270 ) TermsQuery used to be TermsFilter which perhaps explains its location in the queries rather than the core module.
          Hide
          rcmuir Robert Muir added a comment -

          It would be logical if it really behaved the same, but it doesnt.

          Please, don't put this in core with this name right beside TermQuery when it scores differently. That is such a trap.

          Show
          rcmuir Robert Muir added a comment - It would be logical if it really behaved the same, but it doesnt. Please, don't put this in core with this name right beside TermQuery when it scores differently. That is such a trap.
          Hide
          romseygeek Alan Woodward added a comment - - edited

          How about TermInSetQuery, by analogy with PointInSetQuery? The latter is also constant-scoring.

          Show
          romseygeek Alan Woodward added a comment - - edited How about TermInSetQuery, by analogy with PointInSetQuery? The latter is also constant-scoring.
          Hide
          dsmiley David Smiley added a comment -

          +1 to TermInSetQuery

          Show
          dsmiley David Smiley added a comment - +1 to TermInSetQuery
          Hide
          thetaphi Uwe Schindler added a comment -

          +1 for TermInSetQuery. And leave a deprecated stub in the queries module.

          Show
          thetaphi Uwe Schindler added a comment - +1 for TermInSetQuery. And leave a deprecated stub in the queries module.
          Hide
          thetaphi Uwe Schindler added a comment -

          This would also allow us to explore rewriting BooleanQuery to TermsQuery to avoid the max-clauses limit

          Can't work unless the BooleanQuery has no scoring enabled (this can be checked when Weight is created, but not during rewrite).

          Show
          thetaphi Uwe Schindler added a comment - This would also allow us to explore rewriting BooleanQuery to TermsQuery to avoid the max-clauses limit Can't work unless the BooleanQuery has no scoring enabled (this can be checked when Weight is created, but not during rewrite).
          Hide
          rcmuir Robert Muir added a comment -

          +1 for the much better name.

          Show
          rcmuir Robert Muir added a comment - +1 for the much better name.
          Hide
          mikemccand Michael McCandless added a comment -

          +1 for TermInSetQuery.

          Show
          mikemccand Michael McCandless added a comment - +1 for TermInSetQuery .
          Hide
          romseygeek Alan Woodward added a comment -

          Here's a patch, will commit shortly.

          Show
          romseygeek Alan Woodward added a comment - Here's a patch, will commit shortly.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 8511f9e6991679f71e7a82c6ef9cf1b774d090aa in lucene-solr's branch refs/heads/branch_6x from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8511f9e ]

          LUCENE-7624: Move TermsQuery into core as TermInSetQuery

          Show
          jira-bot ASF subversion and git services added a comment - Commit 8511f9e6991679f71e7a82c6ef9cf1b774d090aa in lucene-solr's branch refs/heads/branch_6x from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8511f9e ] LUCENE-7624 : Move TermsQuery into core as TermInSetQuery
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 22940f5c49297b606d710c6775309d67ff064f2f in lucene-solr's branch refs/heads/master from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=22940f5 ]

          LUCENE-7624: Move TermsQuery into core as TermInSetQuery

          Show
          jira-bot ASF subversion and git services added a comment - Commit 22940f5c49297b606d710c6775309d67ff064f2f in lucene-solr's branch refs/heads/master from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=22940f5 ] LUCENE-7624 : Move TermsQuery into core as TermInSetQuery
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 17cd0f00cc1a7bce647eedfe56c860a02aa22654 in lucene-solr's branch refs/heads/master from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=17cd0f0 ]

          LUCENE-7624: Remove deprecated TermsQuery

          Show
          jira-bot ASF subversion and git services added a comment - Commit 17cd0f00cc1a7bce647eedfe56c860a02aa22654 in lucene-solr's branch refs/heads/master from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=17cd0f0 ] LUCENE-7624 : Remove deprecated TermsQuery
          Hide
          romseygeek Alan Woodward added a comment -

          Thanks all

          Show
          romseygeek Alan Woodward added a comment - Thanks all
          Hide
          jpountz Adrien Grand added a comment -

          Should we take advantage of this move to require that all terms are in the same field? Using this query across multiple fields disables an optimization, and I don't think it is very useful etither?

          Show
          jpountz Adrien Grand added a comment - Should we take advantage of this move to require that all terms are in the same field? Using this query across multiple fields disables an optimization, and I don't think it is very useful etither?
          Hide
          mikemccand Michael McCandless added a comment -

          Should we take advantage of this move to require that all terms are in the same field? Using this query across multiple fields disables an optimization, and I don't think it is very useful etither?

          +1

          Show
          mikemccand Michael McCandless added a comment - Should we take advantage of this move to require that all terms are in the same field? Using this query across multiple fields disables an optimization, and I don't think it is very useful etither? +1
          Hide
          dsmiley David Smiley added a comment -

          Should we take advantage of this move to require that all terms are in the same field? Using this query across multiple fields disables an optimization, and I don't think it is very useful etither?

          +1 I agree

          Show
          dsmiley David Smiley added a comment - Should we take advantage of this move to require that all terms are in the same field? Using this query across multiple fields disables an optimization, and I don't think it is very useful etither? +1 I agree
          Hide
          paul.elschot@xs4all.nl Paul Elschot added a comment -

          This one is an interesting target for surround, so I had a look.

          Allowing more than one field for the terms also has an advantage in that only one doc id set will be built for all the terms.

          As to the code:
          There is a small javadoc mistake in line 54 using both "@{" and "{@".
          When constructing a Term a deep copy of the given BytesRef is taken, so the deep copy in line 154 is superfluous.
          (The deep copy in line 222 of the termEnum.term() is needed there.)

          Show
          paul.elschot@xs4all.nl Paul Elschot added a comment - This one is an interesting target for surround, so I had a look. Allowing more than one field for the terms also has an advantage in that only one doc id set will be built for all the terms. As to the code: There is a small javadoc mistake in line 54 using both "@{" and "{@". When constructing a Term a deep copy of the given BytesRef is taken, so the deep copy in line 154 is superfluous. (The deep copy in line 222 of the termEnum.term() is needed there.)
          Hide
          jpountz Adrien Grand added a comment -

          Allowing more than one field for the terms also has an advantage in that only one doc id set will be built for all the terms.

          This is correct, but I think this is a very rare use-case? On the other end, requiring a single field would simplify things I think. It would be more consistent with other queries, eg. PointInSetQuery or would allow to rewrite to an AutomatonQuery if that proves helpful in some cases (which also works on a single field).

          Show
          jpountz Adrien Grand added a comment - Allowing more than one field for the terms also has an advantage in that only one doc id set will be built for all the terms. This is correct, but I think this is a very rare use-case? On the other end, requiring a single field would simplify things I think. It would be more consistent with other queries, eg. PointInSetQuery or would allow to rewrite to an AutomatonQuery if that proves helpful in some cases (which also works on a single field).
          Hide
          jira-bot ASF subversion and git services added a comment -

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

          Revert "LUCENE-7624: Move TermsQuery into core as TermInSetQuery"

          This reverts commit 8511f9e6991679f71e7a82c6ef9cf1b774d090aa.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 929fba34d95a9d3b168b38839203f2835be1060f in lucene-solr's branch refs/heads/branch_6_4 from Adrien Grand [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=929fba3 ] Revert " LUCENE-7624 : Move TermsQuery into core as TermInSetQuery" This reverts commit 8511f9e6991679f71e7a82c6ef9cf1b774d090aa.
          Hide
          jpountz Adrien Grand added a comment -

          I just reverted from the 6.4 branch as discussed on the dev list in order to have a bit more time to discuss whether this query should allow terms that come from multiple fields.

          Show
          jpountz Adrien Grand added a comment - I just reverted from the 6.4 branch as discussed on the dev list in order to have a bit more time to discuss whether this query should allow terms that come from multiple fields.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user PaulElschot opened a pull request:

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

          Minor corrections, see also LUCENE-7624

          LUCENE-7637

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

          $ git pull https://github.com/PaulElschot/lucene-solr lucene7637

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

          https://github.com/apache/lucene-solr/pull/141.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 #141


          commit 59d8bd27df50f280e8530e757e946fceaa1cc37a
          Author: Paul Elschot <paul.j.elschot@gmail.com>
          Date: 2017-01-21T14:32:02Z

          Minor corrections, see also LUCENE-7624


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user PaulElschot opened a pull request: https://github.com/apache/lucene-solr/pull/141 Minor corrections, see also LUCENE-7624 LUCENE-7637 You can merge this pull request into a Git repository by running: $ git pull https://github.com/PaulElschot/lucene-solr lucene7637 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/lucene-solr/pull/141.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 #141 commit 59d8bd27df50f280e8530e757e946fceaa1cc37a Author: Paul Elschot <paul.j.elschot@gmail.com> Date: 2017-01-21T14:32:02Z Minor corrections, see also LUCENE-7624

            People

            • Assignee:
              romseygeek Alan Woodward
              Reporter:
              romseygeek Alan Woodward
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development