You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/03/02 12:48:54 UTC
[GitHub] [lucene] romseygeek opened a new pull request #722: LUCENE-10431: Deprecate MultiTermQuery.setRewriteMethod()
romseygeek opened a new pull request #722:
URL: https://github.com/apache/lucene/pull/722
Allowing users to mutate MultiTermQuery can give rise to odd bugs, for example
in wrapper queries such as BooleanQuery which lazily calculate their hashcodes
and then cache the result. This commit deprecates the `setRewriteMethod()`
method on `MultiTermQuery`, in preparation for removing it entirely, and adds
constructor parameters to the various MTQ implementations as a preferred
way to set the rewrite method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jpountz commented on a change in pull request #722: LUCENE-10431: Deprecate MultiTermQuery.setRewriteMethod()
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #722:
URL: https://github.com/apache/lucene/pull/722#discussion_r817784359
##########
File path: lucene/core/src/java/org/apache/lucene/search/FuzzyQuery.java
##########
@@ -56,6 +56,10 @@
public static final int defaultMaxExpansions = 50;
public static final boolean defaultTranspositions = true;
+ public static RewriteMethod defaultRewriteMethod(int maxExpansions) {
Review comment:
Let's add javadocs?
##########
File path: lucene/core/src/java/org/apache/lucene/search/FuzzyQuery.java
##########
@@ -113,11 +138,40 @@ public FuzzyQuery(Term term, int maxEdits) {
this(term, maxEdits, defaultPrefixLength);
}
+ /**
+ * Calls {@link #FuzzyQuery(Term, int, int, int, boolean,
+ * org.apache.lucene.search.MultiTermQuery.RewriteMethod) FuzzyQuery(term, maxEdits,
+ * defaultPrefixLength, defaultMaxExpansions, defaultTransitions, defaultRewriteMethod)}.
+ */
+ public FuzzyQuery(Term term, int maxEdits, RewriteMethod rewriteMethod) {
+ this(
+ term,
+ maxEdits,
+ defaultPrefixLength,
+ defaultMaxExpansions,
+ defaultTranspositions,
+ rewriteMethod);
+ }
+
/** Calls {@link #FuzzyQuery(Term, int) FuzzyQuery(term, defaultMaxEdits)}. */
public FuzzyQuery(Term term) {
this(term, defaultMaxEdits);
}
+ /**
+ * Creates a new FuzzyQuery with default max edits, prefix length, max expansions and
+ * transpositions
+ */
+ public FuzzyQuery(Term term, RewriteMethod rewriteMethod) {
+ this(
+ term,
+ defaultMaxEdits,
+ defaultPrefixLength,
+ defaultMaxExpansions,
+ defaultTranspositions,
+ rewriteMethod);
+ }
Review comment:
I wonder if we should add a builder to avoid the explosion of the number of ctors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #722: LUCENE-10431: Deprecate MultiTermQuery.setRewriteMethod()
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #722:
URL: https://github.com/apache/lucene/pull/722#discussion_r817991640
##########
File path: lucene/core/src/java/org/apache/lucene/search/FuzzyQuery.java
##########
@@ -113,11 +138,40 @@ public FuzzyQuery(Term term, int maxEdits) {
this(term, maxEdits, defaultPrefixLength);
}
+ /**
+ * Calls {@link #FuzzyQuery(Term, int, int, int, boolean,
+ * org.apache.lucene.search.MultiTermQuery.RewriteMethod) FuzzyQuery(term, maxEdits,
+ * defaultPrefixLength, defaultMaxExpansions, defaultTransitions, defaultRewriteMethod)}.
+ */
+ public FuzzyQuery(Term term, int maxEdits, RewriteMethod rewriteMethod) {
+ this(
+ term,
+ maxEdits,
+ defaultPrefixLength,
+ defaultMaxExpansions,
+ defaultTranspositions,
+ rewriteMethod);
+ }
+
/** Calls {@link #FuzzyQuery(Term, int) FuzzyQuery(term, defaultMaxEdits)}. */
public FuzzyQuery(Term term) {
this(term, defaultMaxEdits);
}
+ /**
+ * Creates a new FuzzyQuery with default max edits, prefix length, max expansions and
+ * transpositions
+ */
+ public FuzzyQuery(Term term, RewriteMethod rewriteMethod) {
+ this(
+ term,
+ defaultMaxEdits,
+ defaultPrefixLength,
+ defaultMaxExpansions,
+ defaultTranspositions,
+ rewriteMethod);
+ }
Review comment:
Why do we need a builder? Why not just only add `RewriteMethod` parameter to the "full ctor". Don't add any more "sugar" ctors, we already have enough. This change currently wants to add something like 4 but none are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] romseygeek merged pull request #722: LUCENE-10431: Deprecate MultiTermQuery.setRewriteMethod()
Posted by GitBox <gi...@apache.org>.
romseygeek merged pull request #722:
URL: https://github.com/apache/lucene/pull/722
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on a change in pull request #722: LUCENE-10431: Deprecate MultiTermQuery.setRewriteMethod()
Posted by GitBox <gi...@apache.org>.
rmuir commented on a change in pull request #722:
URL: https://github.com/apache/lucene/pull/722#discussion_r817992391
##########
File path: lucene/core/src/java/org/apache/lucene/search/RegexpQuery.java
##########
@@ -62,6 +62,24 @@ public RegexpQuery(Term term) {
this(term, RegExp.ALL);
}
+ /**
+ * Constructs a query for terms matching <code>term</code>.
+ *
+ * <p>By default, all regular expression features are enabled.
+ *
+ * @param term regular expression.
+ * @param rewriteMethod the rewrite method used to build the final query
+ */
+ public RegexpQuery(Term term, RewriteMethod rewriteMethod) {
+ this(
+ term,
+ RegExp.ALL,
+ 0,
+ defaultProvider,
+ Operations.DEFAULT_DETERMINIZE_WORK_LIMIT,
+ rewriteMethod);
+ }
+
Review comment:
same thing here. let's not add it to all these "sugar" ctors. Its enough to add it to the one "full" ctor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] romseygeek commented on a change in pull request #722: LUCENE-10431: Deprecate MultiTermQuery.setRewriteMethod()
Posted by GitBox <gi...@apache.org>.
romseygeek commented on a change in pull request #722:
URL: https://github.com/apache/lucene/pull/722#discussion_r818496740
##########
File path: lucene/core/src/java/org/apache/lucene/search/FuzzyQuery.java
##########
@@ -113,11 +138,40 @@ public FuzzyQuery(Term term, int maxEdits) {
this(term, maxEdits, defaultPrefixLength);
}
+ /**
+ * Calls {@link #FuzzyQuery(Term, int, int, int, boolean,
+ * org.apache.lucene.search.MultiTermQuery.RewriteMethod) FuzzyQuery(term, maxEdits,
+ * defaultPrefixLength, defaultMaxExpansions, defaultTransitions, defaultRewriteMethod)}.
+ */
+ public FuzzyQuery(Term term, int maxEdits, RewriteMethod rewriteMethod) {
+ this(
+ term,
+ maxEdits,
+ defaultPrefixLength,
+ defaultMaxExpansions,
+ defaultTranspositions,
+ rewriteMethod);
+ }
+
/** Calls {@link #FuzzyQuery(Term, int) FuzzyQuery(term, defaultMaxEdits)}. */
public FuzzyQuery(Term term) {
this(term, defaultMaxEdits);
}
+ /**
+ * Creates a new FuzzyQuery with default max edits, prefix length, max expansions and
+ * transpositions
+ */
+ public FuzzyQuery(Term term, RewriteMethod rewriteMethod) {
+ this(
+ term,
+ defaultMaxEdits,
+ defaultPrefixLength,
+ defaultMaxExpansions,
+ defaultTranspositions,
+ rewriteMethod);
+ }
Review comment:
I'm adding about four but there are already about eight :) I quite like the Builder idea but we can argue over that separately, for now I'll just extend the full constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a change in pull request #722: LUCENE-10431: Deprecate MultiTermQuery.setRewriteMethod()
Posted by GitBox <gi...@apache.org>.
uschindler commented on a change in pull request #722:
URL: https://github.com/apache/lucene/pull/722#discussion_r818194470
##########
File path: lucene/core/src/java/org/apache/lucene/search/MultiTermQuery.java
##########
@@ -52,7 +52,7 @@
*/
public abstract class MultiTermQuery extends Query {
protected final String field;
- protected RewriteMethod rewriteMethod = CONSTANT_SCORE_REWRITE;
+ protected RewriteMethod rewriteMethod;
Review comment:
We should add a reminder here to make it final after the depreciation is gone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org