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 2020/05/28 13:41:08 UTC
[GitHub] [lucene-solr] markharwood opened a new pull request #1541: RegExp - add case insensitive matching option
markharwood opened a new pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541
Relates to Jira issue [9386](https://issues.apache.org/jira/projects/LUCENE/issues/LUCENE-9386)
Added a new CASE_INSENSITIVE option to the existing flags.
The RegExp class is a little strange because instances represent either the parser or the parsed objects it nests in a tree. The `flags` field is only relevant to the root parser and was left blank in all parsed nodes. This PR's changes require that the flags int is propagated to all nodes so that they can see if it includes the case insensitive option (all other bits in the flag represent parsing options so there was no need to propagate before).
----------------------------------------------------------------
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.
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-solr] markharwood commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r444182510
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -743,6 +792,30 @@ private Automaton toAutomatonInternal(Map<String,Automaton> automata,
}
return a;
}
+ private Automaton toCaseInsensitiveChar(int codepoint, int maxDeterminizedStates) {
+ Automaton case1 = Automata.makeChar(codepoint);
+ int altCase = Character.isLowerCase(codepoint) ? Character.toUpperCase(codepoint) : Character.toLowerCase(codepoint);
+ Automaton result;
+ if (altCase != codepoint) {
+ result = Operations.union(case1, Automata.makeChar(altCase));
+ result = MinimizationOperations.minimize(result, maxDeterminizedStates);
+ } else {
+ result = case1;
+ }
+ return result;
+ }
Review comment:
Yes, that's what I was thinking
----------------------------------------------------------------
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.
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-solr] markharwood edited a comment on pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood edited a comment on pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#issuecomment-636710038
On reflection, you're right - the single flag is trappy.
I'd like to refactor this class to make this simpler. The root problem we have is propagating parser state (flags/options) down to the objects that represent clauses in the parse tree. This is made difficult by the fact that RegExp is a single class representing both the parser and the parsed nodes.
I suggest refactoring so that :
1) RegExp remains the user-facing class with the public constructor and has the parsing logic
2) We use a new private class RegExpClause to hold clause state, but being an inner class it has access to the flags in the outer RegExp instance that contains it.
This should solve the problem of propagating settings and give us a sounder footing to build on.
Should I do this refactor as part of this PR or another @jpountz ?
One issue is that this would technically be a breaking change as https://issues.apache.org/jira/projects/LUCENE/issues/LUCENE-9371 opened up the internal state of the parser and we will change the class of nodes.
----------------------------------------------------------------
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.
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-solr] jimczi commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jimczi commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r445476294
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -439,12 +441,19 @@
/**
* Syntax flag, enables all optional regexp syntax.
*/
- public static final int ALL = 0xffff;
-
+ public static final int ALL = 0xff;
+
/**
* Syntax flag, enables no optional regexp syntax.
*/
public static final int NONE = 0x0000;
+
+ //----- Matching flags ( > 0xff ) ------
+
+ /**
+ * Allows case insensitive matching of ASCII characters.
+ */
+ public static final int ASCII_CASE_INSENSITIVE = 0x0100;
Review comment:
Let's call it CASE_INSENSITIVE since we want to leave the door for another flag that would control if unicode should be handled fully ?
----------------------------------------------------------------
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.
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-solr] jpountz commented on pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jpountz commented on pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#issuecomment-635616067
The flags that can be passed to the constructor are about the supported operators. Case insentivity is not an operator so it feels wrong to ask users to configure it this way too? I think it's fine to record this information in the flags _internally_, but maybe we should make the constructor take an additional boolean instead of expecting users to configure case insensitivity via a syntax flag?
----------------------------------------------------------------
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.
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-solr] jimczi commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jimczi commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r431877760
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -215,11 +238,13 @@ protected String checkRandomExpression(String docValue) {
String regexPattern = result.toString();
// Assert our randomly generated regex actually matches the provided raw input using java's expression matcher
- Pattern pattern = Pattern.compile(regexPattern);
+ Pattern pattern = caseSensitiveQuery ? Pattern.compile(regexPattern):
+ Pattern.compile(regexPattern, Pattern.CASE_INSENSITIVE);
+ ;
Matcher matcher = pattern.matcher(docValue);
assertTrue("Java regex " + regexPattern + " did not match doc value " + docValue, matcher.matches());
- RegExp regex = new RegExp(regexPattern);
+ RegExp regex = caseSensitiveQuery ? new RegExp(regexPattern) : new RegExp(regexPattern, RegExp.CASE_INSENSITIVE);
Review comment:
Should it be `new RegExp(regexPattern, RegExp.ALL | RegExp.CASE_INSENSITIVE)` ?
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -437,9 +437,15 @@
public static final int INTERVAL = 0x0020;
/**
- * Syntax flag, enables all optional regexp syntax.
+ * Allows case insensitive matching.
*/
- public static final int ALL = 0xffff;
+ public static final int CASE_INSENSITIVE = 0x0040;
+
+ /**
+ * Syntax flag, enables all optional regexp syntax
+ * but preserves default setting of case sensitive matching.
+ */
+ public static final int ALL = 0xffff ^ CASE_INSENSITIVE ;
Review comment:
Maybe add a `ALL_AND_CASE_INSENSITIVE` ?
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -88,10 +88,14 @@ public void testRepeatWithEmptyLanguage() throws Exception {
assertTrue(a.toString().length() > 0);
}
+
+ boolean caseSensitiveQuery = true;
+
public void testCoreJavaParity() {
// Generate random doc values and random regular expressions
// and check for same matching behaviour as Java's Pattern class.
for (int i = 0; i < 1000; i++) {
+ caseSensitiveQuery = true;
Review comment:
should use randomization ?
----------------------------------------------------------------
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.
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-solr] jimczi commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jimczi commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r445505526
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -439,12 +441,19 @@
/**
* Syntax flag, enables all optional regexp syntax.
*/
- public static final int ALL = 0xffff;
-
+ public static final int ALL = 0xff;
+
/**
* Syntax flag, enables no optional regexp syntax.
*/
public static final int NONE = 0x0000;
+
+ //----- Matching flags ( > 0xff ) ------
+
+ /**
+ * Allows case insensitive matching of ASCII characters.
+ */
+ public static final int ASCII_CASE_INSENSITIVE = 0x0100;
Review comment:
should we ensure that the flag is not used in the syntax_flags since we merge the two internally ?
----------------------------------------------------------------
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.
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-solr] markharwood commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r451581237
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -499,10 +508,29 @@ public RegExp(String s) throws IllegalArgumentException {
* regular expression
*/
public RegExp(String s, int syntax_flags) throws IllegalArgumentException {
+ this(s, syntax_flags, 0);
+ }
+ /**
+ * Constructs new <code>RegExp</code> from a string.
+ *
+ * @param s regexp string
+ * @param syntax_flags boolean 'or' of optional syntax constructs to be
+ * enabled
+ * @param match_flags boolean 'or' of match behavior options such as case insensitivity
+ * @exception IllegalArgumentException if an error occurred while parsing the
+ * regular expression
+ */
+ public RegExp(String s, int syntax_flags, int match_flags) throws IllegalArgumentException {
+ // (for BWC reasons we don't validate invalid bits, just trim instead)
+ syntax_flags = syntax_flags & 0xff;
Review comment:
As far as I can see, no. The change would be to remove that line and replace with
if (syntax_flags > ALL) {
throw new IllegalArgumentException("Illegal syntax flag");
}
----------------------------------------------------------------
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.
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-solr] markharwood commented on pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#issuecomment-649413439
Thanks for the reviews @jpountz and @jimczi
I think I've addressed all the review comments now if you have a chance to take another look
----------------------------------------------------------------
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.
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-solr] markharwood commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r445538877
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -439,12 +441,19 @@
/**
* Syntax flag, enables all optional regexp syntax.
*/
- public static final int ALL = 0xffff;
-
+ public static final int ALL = 0xff;
+
/**
* Syntax flag, enables no optional regexp syntax.
*/
public static final int NONE = 0x0000;
+
+ //----- Matching flags ( > 0xff ) ------
+
+ /**
+ * Allows case insensitive matching of ASCII characters.
+ */
+ public static final int ASCII_CASE_INSENSITIVE = 0x0100;
Review comment:
My assumption was this class was lenient to syntax flags > 0xff before this change so should remain so for BWC reasons
----------------------------------------------------------------
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.
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-solr] markharwood merged pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood merged pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541
----------------------------------------------------------------
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.
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-solr] jpountz commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r450944716
##########
File path: lucene/core/src/java/org/apache/lucene/search/RegexpQuery.java
##########
@@ -96,16 +96,46 @@ public RegexpQuery(Term term, int flags, int maxDeterminizedStates) {
* Constructs a query for terms matching <code>term</code>.
*
* @param term regular expression.
- * @param flags optional RegExp features from {@link RegExp}
+ * @param maxDeterminizedStates maximum number of states that compiling the
+ * @param syntax_flags optional RegExp syntax features from {@link RegExp}
+ * automaton for the regexp can result in. Set higher to allow more complex
+ * queries and lower to prevent memory exhaustion.
+ * @param match_flags boolean 'or' of match behavior options such as case insensitivity
+ */
+ public RegexpQuery(Term term, int maxDeterminizedStates, int syntax_flags, int match_flags) {
+ this(term, defaultProvider, maxDeterminizedStates, syntax_flags, match_flags);
+ }
+
+ /**
+ * Constructs a query for terms matching <code>term</code>.
+ *
+ * @param term regular expression.
+ * @param syntax_flags optional RegExp features from {@link RegExp}
* @param provider custom AutomatonProvider for named automata
* @param maxDeterminizedStates maximum number of states that compiling the
* automaton for the regexp can result in. Set higher to allow more complex
* queries and lower to prevent memory exhaustion.
*/
- public RegexpQuery(Term term, int flags, AutomatonProvider provider,
+ public RegexpQuery(Term term, int syntax_flags, AutomatonProvider provider,
int maxDeterminizedStates) {
+ this(term, provider, maxDeterminizedStates, syntax_flags, 0);
+ }
+
+ /**
+ * Constructs a query for terms matching <code>term</code>.
+ *
+ * @param term regular expression.
+ * @param syntax_flags optional RegExp features from {@link RegExp}
+ * @param provider custom AutomatonProvider for named automata
+ * @param maxDeterminizedStates maximum number of states that compiling the
+ * automaton for the regexp can result in. Set higher to allow more complex
+ * queries and lower to prevent memory exhaustion.
+ * @param match_flags boolean 'or' of match behavior options such as case insensitivity
+ */
+ public RegexpQuery(Term term, AutomatonProvider provider,
+ int maxDeterminizedStates, int syntax_flags, int match_flags) {
Review comment:
In my opinion, all constructors should always take parameters in the same order. The current longest constructor does `RegexpQuery(Term term, int syntaxFlags, AutomatonProvider provider, int maxDeterminizedStates)`, so I think that this one should be `RegexpQuery(Term term, int syntaxFlags, int matchFlags, AutomatonProvider provider, int maxDeterminizedStates)`.
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -499,10 +508,29 @@ public RegExp(String s) throws IllegalArgumentException {
* regular expression
*/
public RegExp(String s, int syntax_flags) throws IllegalArgumentException {
+ this(s, syntax_flags, 0);
+ }
+ /**
+ * Constructs new <code>RegExp</code> from a string.
+ *
+ * @param s regexp string
+ * @param syntax_flags boolean 'or' of optional syntax constructs to be
+ * enabled
+ * @param match_flags boolean 'or' of match behavior options such as case insensitivity
+ * @exception IllegalArgumentException if an error occurred while parsing the
+ * regular expression
+ */
+ public RegExp(String s, int syntax_flags, int match_flags) throws IllegalArgumentException {
+ // (for BWC reasons we don't validate invalid bits, just trim instead)
+ syntax_flags = syntax_flags & 0xff;
Review comment:
I don't think we need to maintain bw compat for this, is there any test that fails if you remove this line?
##########
File path: lucene/core/src/java/org/apache/lucene/search/RegexpQuery.java
##########
@@ -96,16 +96,46 @@ public RegexpQuery(Term term, int flags, int maxDeterminizedStates) {
* Constructs a query for terms matching <code>term</code>.
*
* @param term regular expression.
- * @param flags optional RegExp features from {@link RegExp}
+ * @param maxDeterminizedStates maximum number of states that compiling the
+ * @param syntax_flags optional RegExp syntax features from {@link RegExp}
+ * automaton for the regexp can result in. Set higher to allow more complex
+ * queries and lower to prevent memory exhaustion.
+ * @param match_flags boolean 'or' of match behavior options such as case insensitivity
+ */
+ public RegexpQuery(Term term, int maxDeterminizedStates, int syntax_flags, int match_flags) {
Review comment:
I'd keep maxDeterminizedStates last so that all constructors take parameters in the same order.
----------------------------------------------------------------
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.
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-solr] markharwood commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r445490501
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -439,12 +441,19 @@
/**
* Syntax flag, enables all optional regexp syntax.
*/
- public static final int ALL = 0xffff;
-
+ public static final int ALL = 0xff;
+
/**
* Syntax flag, enables no optional regexp syntax.
*/
public static final int NONE = 0x0000;
+
+ //----- Matching flags ( > 0xff ) ------
+
+ /**
+ * Allows case insensitive matching of ASCII characters.
+ */
+ public static final int ASCII_CASE_INSENSITIVE = 0x0100;
Review comment:
I thought it might be useful if the flag name reflected the current limitations?
----------------------------------------------------------------
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.
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-solr] jimczi commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jimczi commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r444163411
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -743,6 +792,30 @@ private Automaton toAutomatonInternal(Map<String,Automaton> automata,
}
return a;
}
+ private Automaton toCaseInsensitiveChar(int codepoint, int maxDeterminizedStates) {
+ Automaton case1 = Automata.makeChar(codepoint);
+ int altCase = Character.isLowerCase(codepoint) ? Character.toUpperCase(codepoint) : Character.toLowerCase(codepoint);
+ Automaton result;
+ if (altCase != codepoint) {
+ result = Operations.union(case1, Automata.makeChar(altCase));
+ result = MinimizationOperations.minimize(result, maxDeterminizedStates);
+ } else {
+ result = case1;
+ }
+ return result;
+ }
Review comment:
So you mean having two bit mask flags, one for the operators and one for the matching behavior ? I think that makes sense and could avoid complicating the ctrs in the future so +1.
----------------------------------------------------------------
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.
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-solr] markharwood commented on pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#issuecomment-638776739
I updated the constructor on this @jpountz - good to go?
----------------------------------------------------------------
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.
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-solr] jpountz commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r440900284
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -489,6 +497,19 @@ public RegExp(String s) throws IllegalArgumentException {
this(s, ALL);
}
+ /**
+ * Constructs new <code>RegExp</code> from a string. Same as
+ * <code>RegExp(s, ALL)</code>.
+ *
+ * @param s regexp string
+ * @param caseSensitive case sensitive matching
+ * @exception IllegalArgumentException if an error occurred while parsing the
+ * regular expression
+ */
+ public RegExp(String s, boolean caseSensitive) throws IllegalArgumentException {
+ this(s, ALL, caseSensitive);
+ }
Review comment:
same here, maybe it's fine to require passing syntax flags when you want to configure case sensitivity?
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -499,10 +520,30 @@ public RegExp(String s) throws IllegalArgumentException {
* regular expression
*/
public RegExp(String s, int syntax_flags) throws IllegalArgumentException {
+ this(s, syntax_flags, true);
+ }
+ /**
+ * Constructs new <code>RegExp</code> from a string.
+ *
+ * @param s regexp string
+ * @param syntax_flags boolean 'or' of optional syntax constructs to be
+ * enabled
+ * @param caseSensitive case sensitive matching
+ * @exception IllegalArgumentException if an error occurred while parsing the
+ * regular expression
+ */
+ public RegExp(String s, int syntax_flags, boolean caseSensitive) throws IllegalArgumentException {
originalString = s;
- flags = syntax_flags;
+ // Trim any bits unrelated to syntax flags
+ syntax_flags = syntax_flags & 0xff;
+ if (caseSensitive) {
+ flags = syntax_flags;
+ } else {
+ // Add in the case-insensitive setting
+ flags = syntax_flags | UNICODE_CASE_INSENSITIVE;
Review comment:
```suggestion
flags = syntax_flags | UNICODE_CASE_INSENSITIVE;
```
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -743,6 +792,30 @@ private Automaton toAutomatonInternal(Map<String,Automaton> automata,
}
return a;
}
+ private Automaton toCaseInsensitiveChar(int codepoint, int maxDeterminizedStates) {
+ Automaton case1 = Automata.makeChar(codepoint);
+ int altCase = Character.isLowerCase(codepoint) ? Character.toUpperCase(codepoint) : Character.toLowerCase(codepoint);
+ Automaton result;
+ if (altCase != codepoint) {
+ result = Operations.union(case1, Automata.makeChar(altCase));
+ result = MinimizationOperations.minimize(result, maxDeterminizedStates);
+ } else {
+ result = case1;
+ }
+ return result;
+ }
+
+ private Automaton toCaseInsensitiveString(int maxDeterminizedStates) {
+ List<Automaton> list = new ArrayList<>();
+ s.codePoints().forEach(
+ p -> {
+ list.add(toCaseInsensitiveChar(p, maxDeterminizedStates));
+ }
+ );
Review comment:
I'd rather like a regular for loop, this is a bit abusing lambdas to my taste. :)
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -743,6 +792,30 @@ private Automaton toAutomatonInternal(Map<String,Automaton> automata,
}
return a;
}
+ private Automaton toCaseInsensitiveChar(int codepoint, int maxDeterminizedStates) {
+ Automaton case1 = Automata.makeChar(codepoint);
+ int altCase = Character.isLowerCase(codepoint) ? Character.toUpperCase(codepoint) : Character.toLowerCase(codepoint);
+ Automaton result;
+ if (altCase != codepoint) {
+ result = Operations.union(case1, Automata.makeChar(altCase));
+ result = MinimizationOperations.minimize(result, maxDeterminizedStates);
+ } else {
+ result = case1;
+ }
+ return result;
+ }
Review comment:
I think that this is incorrect as there is no 1:1 mapping between lowercase and uppercase letters, for instance `ς` and `σ` both have `Σ` as their uppercase variant. And if someone uses `Σ` in their regexes, `ς` wouldn't match as `toLowerCase(Σ)` returns `σ`.
Should we make this only about ASCII for now, like Java's Pattern class? https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/util/regex/Pattern.html#CASE_INSENSITIVE We could add support for full Unicode later but this doesn't look like a low hanging fruit to me?
##########
File path: lucene/core/src/java/org/apache/lucene/search/RegexpQuery.java
##########
@@ -68,6 +68,19 @@ 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.
+ * </p>
+ *
+ * @param term regular expression.
+ * @param caseSensitive set to false for case insensitive matching
+ */
+ public RegexpQuery(Term term, boolean caseSensitive) {
+ this(term, RegExp.ALL, caseSensitive);
+ }
Review comment:
In order not to have a combinatorial explosion of the number of ctors, I think we could consider dropping this one: I think it's fine to require users to provide flags if they also want to configure case sensitivity?
----------------------------------------------------------------
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.
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-solr] jpountz commented on pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jpountz commented on pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#issuecomment-637503848
@markharwood Java's case-insensitivity flag is indeed a bit mask flag passed to the constructor, but my comment was more about the fact that the current flags on RegExp are about what operators are supported, while the bit you are adding controls how matching works. So in my opinion, it should be a different constructor argument (boolean or other bit set depending on whether we're seeing other desirable ways to control how matching works). We could merge both bitsets internally if that makes things easier, my concern is only about the API.
I don't have an opinion about the RegExp split, but I don't feel bad about propagating information recursively like in your PR.
----------------------------------------------------------------
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.
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-solr] markharwood commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r444134039
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -743,6 +792,30 @@ private Automaton toAutomatonInternal(Map<String,Automaton> automata,
}
return a;
}
+ private Automaton toCaseInsensitiveChar(int codepoint, int maxDeterminizedStates) {
+ Automaton case1 = Automata.makeChar(codepoint);
+ int altCase = Character.isLowerCase(codepoint) ? Character.toUpperCase(codepoint) : Character.toLowerCase(codepoint);
+ Automaton result;
+ if (altCase != codepoint) {
+ result = Operations.union(case1, Automata.makeChar(altCase));
+ result = MinimizationOperations.minimize(result, maxDeterminizedStates);
+ } else {
+ result = case1;
+ }
+ return result;
+ }
Review comment:
Would that be an argument for having a bit mask for case sensitivity flags instead of a Boolean? Java has the UNICODE_CASE flag which differs from the ASCII one and it might be an idea to leave something in the API open to future work.
----------------------------------------------------------------
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.
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-solr] markharwood commented on pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#issuecomment-654215190
OK to merge this @jpountz ?
I had one last [comment](https://github.com/apache/lucene-solr/pull/1541#discussion_r445538877) re flag validation
----------------------------------------------------------------
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.
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-solr] markharwood commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r445497621
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -439,12 +441,19 @@
/**
* Syntax flag, enables all optional regexp syntax.
*/
- public static final int ALL = 0xffff;
-
+ public static final int ALL = 0xff;
+
/**
* Syntax flag, enables no optional regexp syntax.
*/
public static final int NONE = 0x0000;
+
+ //----- Matching flags ( > 0xff ) ------
+
+ /**
+ * Allows case insensitive matching of ASCII characters.
+ */
+ public static final int ASCII_CASE_INSENSITIVE = 0x0100;
Review comment:
UNICODE_CASE_INSENSITIVE or just CASE_INSENSITIVE?
Either way it would cover ASCII and all other UNICODE characters.
Admittedly slightly odd that the two flags overlap but the alternative is people may assume that a non-qualified name like "CASE_INSENSITIVE" would cover all the bases when we only currently support ASCII.
----------------------------------------------------------------
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.
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-solr] jimczi commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jimczi commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r445504910
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -439,12 +441,19 @@
/**
* Syntax flag, enables all optional regexp syntax.
*/
- public static final int ALL = 0xffff;
-
+ public static final int ALL = 0xff;
+
/**
* Syntax flag, enables no optional regexp syntax.
*/
public static final int NONE = 0x0000;
+
+ //----- Matching flags ( > 0xff ) ------
+
+ /**
+ * Allows case insensitive matching of ASCII characters.
+ */
+ public static final int ASCII_CASE_INSENSITIVE = 0x0100;
Review comment:
ok fine by me.
----------------------------------------------------------------
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.
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-solr] jimczi commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jimczi commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r441813998
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -743,6 +792,30 @@ private Automaton toAutomatonInternal(Map<String,Automaton> automata,
}
return a;
}
+ private Automaton toCaseInsensitiveChar(int codepoint, int maxDeterminizedStates) {
+ Automaton case1 = Automata.makeChar(codepoint);
+ int altCase = Character.isLowerCase(codepoint) ? Character.toUpperCase(codepoint) : Character.toLowerCase(codepoint);
+ Automaton result;
+ if (altCase != codepoint) {
+ result = Operations.union(case1, Automata.makeChar(altCase));
+ result = MinimizationOperations.minimize(result, maxDeterminizedStates);
+ } else {
+ result = case1;
+ }
+ return result;
+ }
Review comment:
good catch, +1 for ASCII only for now, I guess it was too ambitious to handle unicode in the first run
----------------------------------------------------------------
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.
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-solr] markharwood commented on pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#issuecomment-635913889
>maybe we should make the constructor take an additional boolean instead of expecting users to configure case insensitivity via a syntax flag?
Does it change things if we consider Java's case insensitivity is also [a bit mask flag passed to the constructor](https://docs.oracle.com/javase/7/docs/api/java/util/regex/Pattern.html#compile(java.lang.String,%20int))?
----------------------------------------------------------------
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.
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-solr] markharwood commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r431891105
##########
File path: lucene/core/src/test/org/apache/lucene/util/automaton/TestRegExp.java
##########
@@ -88,10 +88,14 @@ public void testRepeatWithEmptyLanguage() throws Exception {
assertTrue(a.toString().length() > 0);
}
+
+ boolean caseSensitiveQuery = true;
+
public void testCoreJavaParity() {
// Generate random doc values and random regular expressions
// and check for same matching behaviour as Java's Pattern class.
for (int i = 0; i < 1000; i++) {
+ caseSensitiveQuery = true;
Review comment:
The randomisation comes from the choice of string mutation later that produces the regex - this is just initialising the variable.
----------------------------------------------------------------
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.
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-solr] markharwood commented on pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#issuecomment-636710038
On reflection, you're right - the single flag is trappy.
I'd like to refactor this class to make this simpler. The root problem we have is propagating parser state (flags/options) down to the objects that represent clauses in the parse tree. This is made difficult by the fact that RegExp is a single class representing both the parser and the parsed nodes.
I suggest refactoring so that :
1) RegExp remains the user-facing class with the public constructor and has the parsing logic
2) We use a new private class RegExpClause to hold clause state, but being an inner class it has access to the flags in the outer RegExp instance that contains it.
This should solve the problem of propagating settings and give us a sounder footing to build on.
Should I do this refactor as part of this PR or another @jpountz ?
----------------------------------------------------------------
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.
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-solr] markharwood commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r444201265
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -743,6 +792,30 @@ private Automaton toAutomatonInternal(Map<String,Automaton> automata,
}
return a;
}
+ private Automaton toCaseInsensitiveChar(int codepoint, int maxDeterminizedStates) {
+ Automaton case1 = Automata.makeChar(codepoint);
+ int altCase = Character.isLowerCase(codepoint) ? Character.toUpperCase(codepoint) : Character.toLowerCase(codepoint);
+ Automaton result;
+ if (altCase != codepoint) {
+ result = Operations.union(case1, Automata.makeChar(altCase));
+ result = MinimizationOperations.minimize(result, maxDeterminizedStates);
+ } else {
+ result = case1;
+ }
+ return result;
+ }
Review comment:
The one downside is RegExpQuery already has a (Term, int, int) constructor for syntax flags and maxDeterminizedStates so I added a (int, int, Term) variation for passing the syntax and match flags.
----------------------------------------------------------------
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.
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-solr] jimczi commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jimczi commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r444817529
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -743,6 +792,30 @@ private Automaton toAutomatonInternal(Map<String,Automaton> automata,
}
return a;
}
+ private Automaton toCaseInsensitiveChar(int codepoint, int maxDeterminizedStates) {
+ Automaton case1 = Automata.makeChar(codepoint);
+ int altCase = Character.isLowerCase(codepoint) ? Character.toUpperCase(codepoint) : Character.toLowerCase(codepoint);
+ Automaton result;
+ if (altCase != codepoint) {
+ result = Operations.union(case1, Automata.makeChar(altCase));
+ result = MinimizationOperations.minimize(result, maxDeterminizedStates);
+ } else {
+ result = case1;
+ }
+ return result;
+ }
Review comment:
> so I added a (int, int, Term) variation for passing the syntax and match flags.
I am not sure this is clearer ;). It's not ideal but I prefer adding only one ctr `(Term, int, int, int)`.
----------------------------------------------------------------
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.
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-solr] markharwood commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
markharwood commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r444284423
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -743,6 +792,30 @@ private Automaton toAutomatonInternal(Map<String,Automaton> automata,
}
return a;
}
+ private Automaton toCaseInsensitiveChar(int codepoint, int maxDeterminizedStates) {
+ Automaton case1 = Automata.makeChar(codepoint);
+ int altCase = Character.isLowerCase(codepoint) ? Character.toUpperCase(codepoint) : Character.toLowerCase(codepoint);
+ Automaton result;
+ if (altCase != codepoint) {
+ result = Operations.union(case1, Automata.makeChar(altCase));
+ result = MinimizationOperations.minimize(result, maxDeterminizedStates);
+ } else {
+ result = case1;
+ }
+ return result;
+ }
Review comment:
An alternative would be an overhaul of RegExp.
* Introducing a Builder class for the parser with named properties for settings
* separating the RegExp parser logic from the parsed objects (currently they are the same class).
* separating rendering functions (toString, to Automaton, toStringTree) from the parsed objects.
I'm not sure if we're at the tipping point where all of that would make sense.
----------------------------------------------------------------
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.
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-solr] jimczi commented on a change in pull request #1541: RegExp - add case insensitive matching option
Posted by GitBox <gi...@apache.org>.
jimczi commented on a change in pull request #1541:
URL: https://github.com/apache/lucene-solr/pull/1541#discussion_r445491309
##########
File path: lucene/core/src/java/org/apache/lucene/util/automaton/RegExp.java
##########
@@ -439,12 +441,19 @@
/**
* Syntax flag, enables all optional regexp syntax.
*/
- public static final int ALL = 0xffff;
-
+ public static final int ALL = 0xff;
+
/**
* Syntax flag, enables no optional regexp syntax.
*/
public static final int NONE = 0x0000;
+
+ //----- Matching flags ( > 0xff ) ------
+
+ /**
+ * Allows case insensitive matching of ASCII characters.
+ */
+ public static final int ASCII_CASE_INSENSITIVE = 0x0100;
Review comment:
But then what would be the other flag to allow unicode support ?
----------------------------------------------------------------
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.
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