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 2021/06/18 21:24:21 UTC
[GitHub] [lucene] vigyasharma opened a new pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
vigyasharma opened a new pull request #188:
URL: https://github.com/apache/lucene/pull/188
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Lucene:
* https://issues.apache.org/jira/projects/LUCENE
You will need to create an account in Jira in order to create an issue.
The title of the PR should reference the Jira issue number in the form:
* LUCENE-####: <short description of problem or changes>
LUCENE must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
## Description
`CommonGramsFilterFactory` should respect the ignoreCase flag passed in args even when the default stop word set is used. It currently ignores the flag if `commonWordFiles` are not specified.
## Solution
Ensure the flag is respected in even when default stop word set is used.
## Tests
Added test to ensure that bigrams get constructed with common words that are not in lower case, when `ignoreCase` is passed as `true` to the `CommonGramsFilterFactory`.
## Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/lucene/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Lucene maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
--
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] mikemccand commented on pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
mikemccand commented on pull request #188:
URL: https://github.com/apache/lucene/pull/188#issuecomment-903892694
Oh, I think this one can/should be backported to 8.10 as well, @vigyasharma could you please open a PR against `branch_8x` in the `lucene-solr` GitHub repo? Thanks!
--
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] vigyasharma commented on a change in pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
vigyasharma commented on a change in pull request #188:
URL: https://github.com/apache/lucene/pull/188#discussion_r688072266
##########
File path: lucene/analysis/common/src/java/org/apache/lucene/analysis/commongrams/CommonGramsFilterFactory.java
##########
@@ -40,57 +36,28 @@
* @since 3.1
* @lucene.spi {@value #NAME}
*/
-public class CommonGramsFilterFactory extends TokenFilterFactory implements ResourceLoaderAware {
+public class CommonGramsFilterFactory extends AbstractWordsFileFilterFactory {
/** SPI name */
public static final String NAME = "commonGrams";
- // TODO: shared base class for Stop/Keep/CommonGrams?
- private CharArraySet commonWords;
- private final String commonWordFiles;
- private final String format;
- private final boolean ignoreCase;
-
/** Creates a new CommonGramsFilterFactory */
public CommonGramsFilterFactory(Map<String, String> args) {
super(args);
- commonWordFiles = get(args, "words");
- format = get(args, "format");
- ignoreCase = getBoolean(args, "ignoreCase", false);
- if (!args.isEmpty()) {
- throw new IllegalArgumentException("Unknown parameters: " + args);
- }
}
/** Default ctor for compatibility with SPI */
public CommonGramsFilterFactory() {
throw defaultCtorException();
}
- @Override
- public void inform(ResourceLoader loader) throws IOException {
- if (commonWordFiles != null) {
- if ("snowball".equalsIgnoreCase(format)) {
- commonWords = getSnowballWordSet(loader, commonWordFiles, ignoreCase);
- } else {
- commonWords = getWordSet(loader, commonWordFiles, ignoreCase);
- }
- } else {
- commonWords = EnglishAnalyzer.ENGLISH_STOP_WORDS_SET;
Review comment:
That's right.
--
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] vigyasharma commented on pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
vigyasharma commented on pull request #188:
URL: https://github.com/apache/lucene/pull/188#issuecomment-897957207
Thanks for the review, Michael! I've updated this PR with suggested changes.
--
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] mikemccand commented on a change in pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #188:
URL: https://github.com/apache/lucene/pull/188#discussion_r688708518
##########
File path: lucene/analysis/common/src/java/org/apache/lucene/analysis/en/AbstractWordsFileFilterFactory.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.en;
+
+import java.io.IOException;
+import java.util.Map;
+import org.apache.lucene.analysis.CharArraySet;
+import org.apache.lucene.analysis.TokenFilterFactory;
+import org.apache.lucene.analysis.WordlistLoader;
+import org.apache.lucene.util.ResourceLoader;
+import org.apache.lucene.util.ResourceLoaderAware;
+
+/**
+ * Abstract parent class for analysis factories that accept a stopwords file as input.
+ *
+ * <p>Concrete implementations can leverage the following input attributes. All attributes are
+ * optional:
+ *
+ * <ul>
+ * <li><code>ignoreCase</code> defaults to <code>false</code>
+ * <li><code>words</code> should be the name of a stopwords file to parse, if not specified the
+ * factory will use the value provided by {@link #createDefaultWords()} implementation in
+ * concrete subclass.
+ * <li><code>format</code> defines how the <code>words</code> file will be parsed, and defaults to
+ * <code>wordset</code>. If <code>words</code> is not specified, then <code>format</code> must
+ * not be specified.
+ * </ul>
+ *
+ * <p>The valid values for the <code>format</code> option are:
+ *
+ * <ul>
+ * <li><code>wordset</code> - This is the default format, which supports one word per line
+ * (including any intra-word whitespace) and allows whole line comments beginning with the "#"
+ * character. Blank lines are ignored. See {@link WordlistLoader#getLines
+ * WordlistLoader.getLines} for details.
+ * <li><code>snowball</code> - This format allows for multiple words specified on each line, and
+ * trailing comments may be specified using the vertical line ("|"). Blank lines are
+ * ignored. See {@link WordlistLoader#getSnowballWordSet WordlistLoader.getSnowballWordSet}
+ * for details.
+ * </ul>
+ */
+public abstract class AbstractWordsFileFilterFactory extends TokenFilterFactory
+ implements ResourceLoaderAware {
+
+ public static final String FORMAT_WORDSET = "wordset";
+ public static final String FORMAT_SNOWBALL = "snowball";
+
+ private CharArraySet words;
+ private final String wordFiles;
+ private final String format;
+ private final boolean ignoreCase;
+
+ /** Default ctor for compatibility with SPI */
+ protected AbstractWordsFileFilterFactory() {
+ throw defaultCtorException();
+ }
+
+ /** Initialize this factory via a set of key-value pairs. */
+ public AbstractWordsFileFilterFactory(Map<String, String> args) {
+ super(args);
+ wordFiles = get(args, "words");
+ format = get(args, "format", (null == wordFiles ? null : FORMAT_WORDSET));
+ ignoreCase = getBoolean(args, "ignoreCase", false);
+ if (!args.isEmpty()) {
+ throw new IllegalArgumentException("Unknown parameters: " + args);
+ }
+ }
+
+ /** Initialize the set of stopwords provided via ResourceLoader, or using defaults. */
+ @Override
+ public void inform(ResourceLoader loader) throws IOException {
+ if (wordFiles != null) {
+ if (FORMAT_WORDSET.equalsIgnoreCase(format)) {
+ words = getWordSet(loader, wordFiles, ignoreCase);
+ } else if (FORMAT_SNOWBALL.equalsIgnoreCase(format)) {
+ words = getSnowballWordSet(loader, wordFiles, ignoreCase);
+ } else {
+ throw new IllegalArgumentException(
+ "Unknown 'format' specified for 'words' file: " + format);
+ }
+ } else {
+ if (null != format) {
+ throw new IllegalArgumentException(
+ "'format' can not be specified w/o an explicit 'words' file: " + format);
+ }
+ words = createDefaultWords();
+ }
+ }
+
+ /** Default word set implementation. */
+ protected abstract CharArraySet createDefaultWords();
Review comment:
Woot!
--
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] vigyasharma commented on a change in pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
vigyasharma commented on a change in pull request #188:
URL: https://github.com/apache/lucene/pull/188#discussion_r688073110
##########
File path: lucene/analysis/common/src/java/org/apache/lucene/analysis/en/AbstractWordsFileFilterFactory.java
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.en;
+
+import java.io.IOException;
+import java.util.Map;
+import org.apache.lucene.analysis.CharArraySet;
+import org.apache.lucene.analysis.TokenFilterFactory;
+import org.apache.lucene.analysis.WordlistLoader;
+import org.apache.lucene.util.ResourceLoader;
+import org.apache.lucene.util.ResourceLoaderAware;
+
+/**
+ * Abstract parent class for analysis factories that accept a stopwords file as input.
+ *
+ * <p>Concrete implementations can leverage the following input attributes. All attributes are
+ * optional:
+ *
+ * <ul>
+ * <li><code>ignoreCase</code> defaults to <code>false</code>
+ * <li><code>words</code> should be the name of a stopwords file to parse, if not specified the
+ * factory will use the value provided by {@link #createDefaultWords()} implementation in
+ * concrete subclass. Defaults to {@link EnglishAnalyzer#ENGLISH_STOP_WORDS_SET}
+ * <li><code>format</code> defines how the <code>words</code> file will be parsed, and defaults to
+ * <code>wordset</code>. If <code>words</code> is not specified, then <code>format</code> must
+ * not be specified.
+ * </ul>
+ *
+ * <p>The valid values for the <code>format</code> option are:
+ *
+ * <ul>
+ * <li><code>wordset</code> - This is the default format, which supports one word per line
+ * (including any intra-word whitespace) and allows whole line comments beginning with the "#"
+ * character. Blank lines are ignored. See {@link WordlistLoader#getLines
+ * WordlistLoader.getLines} for details.
+ * <li><code>snowball</code> - This format allows for multiple words specified on each line, and
+ * trailing comments may be specified using the vertical line ("|"). Blank lines are
+ * ignored. See {@link WordlistLoader#getSnowballWordSet WordlistLoader.getSnowballWordSet}
+ * for details.
+ * </ul>
+ */
+public abstract class AbstractWordsFileFilterFactory extends TokenFilterFactory
+ implements ResourceLoaderAware {
+
+ public static final String FORMAT_WORDSET = "wordset";
+ public static final String FORMAT_SNOWBALL = "snowball";
+
+ private CharArraySet words;
+ private final String wordFiles;
+ private final String format;
+ private final boolean ignoreCase;
+
+ /** Default ctor for compatibility with SPI */
+ protected AbstractWordsFileFilterFactory() {
+ throw defaultCtorException();
+ }
+
+ /** Initialize this factory via a set of key-value pairs. */
+ public AbstractWordsFileFilterFactory(Map<String, String> args) {
+ super(args);
+ wordFiles = get(args, "words");
+ format = get(args, "format", (null == wordFiles ? null : FORMAT_WORDSET));
+ ignoreCase = getBoolean(args, "ignoreCase", false);
+ if (!args.isEmpty()) {
+ throw new IllegalArgumentException("Unknown parameters: " + args);
+ }
+ }
+
+ /** Initialize the set of stopwords provided via ResourceLoader, or using defaults. */
+ @Override
+ public void inform(ResourceLoader loader) throws IOException {
+ if (wordFiles != null) {
+ if (FORMAT_WORDSET.equalsIgnoreCase(format)) {
+ words = getWordSet(loader, wordFiles, ignoreCase);
+ } else if (FORMAT_SNOWBALL.equalsIgnoreCase(format)) {
+ words = getSnowballWordSet(loader, wordFiles, ignoreCase);
+ } else {
+ throw new IllegalArgumentException(
+ "Unknown 'format' specified for 'words' file: " + format);
+ }
+ } else {
+ if (null != format) {
+ throw new IllegalArgumentException(
+ "'format' can not be specified w/o an explicit 'words' file: " + format);
+ }
+ words = createDefaultWords();
+ }
+ }
+
+ /** Default word set implementation. */
+ protected CharArraySet createDefaultWords() {
+ return new CharArraySet(EnglishAnalyzer.ENGLISH_STOP_WORDS_SET, ignoreCase);
Review comment:
Good point, I've moved the default stop word impl. to subclasses and made this method abstract.
--
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] mikemccand commented on pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
mikemccand commented on pull request #188:
URL: https://github.com/apache/lucene/pull/188#issuecomment-897534338
Thanks @vigyasharma, I'll try to have a look soon.
--
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] vigyasharma commented on a change in pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
vigyasharma commented on a change in pull request #188:
URL: https://github.com/apache/lucene/pull/188#discussion_r688072666
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/commongrams/TestCommonGramsFilterFactory.java
##########
@@ -98,6 +98,25 @@ public void testDefaults() throws Exception {
stream, new String[] {"testing", "testing_the", "the", "the_factory", "factory"});
}
+ /**
+ * Test that ignoreCase flag is honored when no words are provided and default stopwords are used.
+ */
+ public void testIgnoreCase() throws Exception {
Review comment:
Yes, it failed before the fix and refactor, and passes after.
--
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] vigyasharma commented on pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
vigyasharma commented on pull request #188:
URL: https://github.com/apache/lucene/pull/188#issuecomment-919395702
> Oh, I think this one can/should be backported to 8.10 as well, @vigyasharma could you please open a PR against `branch_8x` in the `lucene-solr` GitHub repo? Thanks!
Thanks for the review, @mikemccand. Created PR - https://github.com/apache/lucene-solr/pull/2573 to backport these changes.
--
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] mikemccand commented on a change in pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #188:
URL: https://github.com/apache/lucene/pull/188#discussion_r687651878
##########
File path: lucene/analysis/common/src/test/org/apache/lucene/analysis/commongrams/TestCommonGramsFilterFactory.java
##########
@@ -98,6 +98,25 @@ public void testDefaults() throws Exception {
stream, new String[] {"testing", "testing_the", "the", "the_factory", "factory"});
}
+ /**
+ * Test that ignoreCase flag is honored when no words are provided and default stopwords are used.
+ */
+ public void testIgnoreCase() throws Exception {
Review comment:
This test case failed before the refactoring? Perfect :)
##########
File path: lucene/analysis/common/src/java/org/apache/lucene/analysis/en/AbstractWordsFileFilterFactory.java
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.en;
+
+import java.io.IOException;
+import java.util.Map;
+import org.apache.lucene.analysis.CharArraySet;
+import org.apache.lucene.analysis.TokenFilterFactory;
+import org.apache.lucene.analysis.WordlistLoader;
+import org.apache.lucene.util.ResourceLoader;
+import org.apache.lucene.util.ResourceLoaderAware;
+
+/**
+ * Abstract parent class for analysis factories that accept a stopwords file as input.
+ *
+ * <p>Concrete implementations can leverage the following input attributes. All attributes are
+ * optional:
+ *
+ * <ul>
+ * <li><code>ignoreCase</code> defaults to <code>false</code>
+ * <li><code>words</code> should be the name of a stopwords file to parse, if not specified the
+ * factory will use the value provided by {@link #createDefaultWords()} implementation in
+ * concrete subclass. Defaults to {@link EnglishAnalyzer#ENGLISH_STOP_WORDS_SET}
+ * <li><code>format</code> defines how the <code>words</code> file will be parsed, and defaults to
+ * <code>wordset</code>. If <code>words</code> is not specified, then <code>format</code> must
+ * not be specified.
+ * </ul>
+ *
+ * <p>The valid values for the <code>format</code> option are:
+ *
+ * <ul>
+ * <li><code>wordset</code> - This is the default format, which supports one word per line
+ * (including any intra-word whitespace) and allows whole line comments beginning with the "#"
+ * character. Blank lines are ignored. See {@link WordlistLoader#getLines
+ * WordlistLoader.getLines} for details.
+ * <li><code>snowball</code> - This format allows for multiple words specified on each line, and
+ * trailing comments may be specified using the vertical line ("|"). Blank lines are
+ * ignored. See {@link WordlistLoader#getSnowballWordSet WordlistLoader.getSnowballWordSet}
+ * for details.
+ * </ul>
+ */
+public abstract class AbstractWordsFileFilterFactory extends TokenFilterFactory
+ implements ResourceLoaderAware {
+
+ public static final String FORMAT_WORDSET = "wordset";
+ public static final String FORMAT_SNOWBALL = "snowball";
+
+ private CharArraySet words;
+ private final String wordFiles;
+ private final String format;
+ private final boolean ignoreCase;
+
+ /** Default ctor for compatibility with SPI */
+ protected AbstractWordsFileFilterFactory() {
+ throw defaultCtorException();
+ }
+
+ /** Initialize this factory via a set of key-value pairs. */
+ public AbstractWordsFileFilterFactory(Map<String, String> args) {
+ super(args);
+ wordFiles = get(args, "words");
+ format = get(args, "format", (null == wordFiles ? null : FORMAT_WORDSET));
+ ignoreCase = getBoolean(args, "ignoreCase", false);
+ if (!args.isEmpty()) {
+ throw new IllegalArgumentException("Unknown parameters: " + args);
+ }
+ }
+
+ /** Initialize the set of stopwords provided via ResourceLoader, or using defaults. */
+ @Override
+ public void inform(ResourceLoader loader) throws IOException {
+ if (wordFiles != null) {
+ if (FORMAT_WORDSET.equalsIgnoreCase(format)) {
+ words = getWordSet(loader, wordFiles, ignoreCase);
+ } else if (FORMAT_SNOWBALL.equalsIgnoreCase(format)) {
+ words = getSnowballWordSet(loader, wordFiles, ignoreCase);
+ } else {
+ throw new IllegalArgumentException(
+ "Unknown 'format' specified for 'words' file: " + format);
+ }
+ } else {
+ if (null != format) {
+ throw new IllegalArgumentException(
+ "'format' can not be specified w/o an explicit 'words' file: " + format);
+ }
+ words = createDefaultWords();
+ }
+ }
+
+ /** Default word set implementation. */
+ protected CharArraySet createDefaultWords() {
+ return new CharArraySet(EnglishAnalyzer.ENGLISH_STOP_WORDS_SET, ignoreCase);
Review comment:
It's kinda weird to default to English stop words here? This base class is a generic "this thing needs to load words from a file" sort of deal ... maybe make this method abstract and force all subclasses to implement it and move this impl down to `StopFilterFactory`? It is separately weird that we default to English there too! English is just one (weird!) language! But we don't need to solve that one here.
And I guess `CommonGramsFilterFactory` would also default to English stop words, as it does already today.
##########
File path: lucene/analysis/common/src/java/org/apache/lucene/analysis/en/AbstractWordsFileFilterFactory.java
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.analysis.en;
+
+import java.io.IOException;
+import java.util.Map;
+import org.apache.lucene.analysis.CharArraySet;
+import org.apache.lucene.analysis.TokenFilterFactory;
+import org.apache.lucene.analysis.WordlistLoader;
+import org.apache.lucene.util.ResourceLoader;
+import org.apache.lucene.util.ResourceLoaderAware;
+
+/**
+ * Abstract parent class for analysis factories that accept a stopwords file as input.
+ *
+ * <p>Concrete implementations can leverage the following input attributes. All attributes are
+ * optional:
+ *
+ * <ul>
+ * <li><code>ignoreCase</code> defaults to <code>false</code>
+ * <li><code>words</code> should be the name of a stopwords file to parse, if not specified the
+ * factory will use the value provided by {@link #createDefaultWords()} implementation in
+ * concrete subclass. Defaults to {@link EnglishAnalyzer#ENGLISH_STOP_WORDS_SET}
+ * <li><code>format</code> defines how the <code>words</code> file will be parsed, and defaults to
+ * <code>wordset</code>. If <code>words</code> is not specified, then <code>format</code> must
+ * not be specified.
+ * </ul>
+ *
+ * <p>The valid values for the <code>format</code> option are:
+ *
+ * <ul>
+ * <li><code>wordset</code> - This is the default format, which supports one word per line
+ * (including any intra-word whitespace) and allows whole line comments beginning with the "#"
+ * character. Blank lines are ignored. See {@link WordlistLoader#getLines
+ * WordlistLoader.getLines} for details.
+ * <li><code>snowball</code> - This format allows for multiple words specified on each line, and
+ * trailing comments may be specified using the vertical line ("|"). Blank lines are
+ * ignored. See {@link WordlistLoader#getSnowballWordSet WordlistLoader.getSnowballWordSet}
+ * for details.
+ * </ul>
+ */
+public abstract class AbstractWordsFileFilterFactory extends TokenFilterFactory
+ implements ResourceLoaderAware {
+
+ public static final String FORMAT_WORDSET = "wordset";
+ public static final String FORMAT_SNOWBALL = "snowball";
+
+ private CharArraySet words;
+ private final String wordFiles;
+ private final String format;
+ private final boolean ignoreCase;
+
+ /** Default ctor for compatibility with SPI */
+ protected AbstractWordsFileFilterFactory() {
+ throw defaultCtorException();
+ }
+
+ /** Initialize this factory via a set of key-value pairs. */
+ public AbstractWordsFileFilterFactory(Map<String, String> args) {
+ super(args);
+ wordFiles = get(args, "words");
+ format = get(args, "format", (null == wordFiles ? null : FORMAT_WORDSET));
+ ignoreCase = getBoolean(args, "ignoreCase", false);
+ if (!args.isEmpty()) {
+ throw new IllegalArgumentException("Unknown parameters: " + args);
+ }
+ }
+
+ /** Initialize the set of stopwords provided via ResourceLoader, or using defaults. */
+ @Override
+ public void inform(ResourceLoader loader) throws IOException {
+ if (wordFiles != null) {
+ if (FORMAT_WORDSET.equalsIgnoreCase(format)) {
+ words = getWordSet(loader, wordFiles, ignoreCase);
+ } else if (FORMAT_SNOWBALL.equalsIgnoreCase(format)) {
+ words = getSnowballWordSet(loader, wordFiles, ignoreCase);
+ } else {
+ throw new IllegalArgumentException(
+ "Unknown 'format' specified for 'words' file: " + format);
+ }
+ } else {
+ if (null != format) {
+ throw new IllegalArgumentException(
+ "'format' can not be specified w/o an explicit 'words' file: " + format);
+ }
+ words = createDefaultWords();
Review comment:
And this fixes the bug, because we now always dynamically create the `CharArraySet words`, taking `ignoreCase` into account...
##########
File path: lucene/analysis/common/src/java/org/apache/lucene/analysis/commongrams/CommonGramsFilterFactory.java
##########
@@ -40,57 +36,28 @@
* @since 3.1
* @lucene.spi {@value #NAME}
*/
-public class CommonGramsFilterFactory extends TokenFilterFactory implements ResourceLoaderAware {
+public class CommonGramsFilterFactory extends AbstractWordsFileFilterFactory {
/** SPI name */
public static final String NAME = "commonGrams";
- // TODO: shared base class for Stop/Keep/CommonGrams?
- private CharArraySet commonWords;
- private final String commonWordFiles;
- private final String format;
- private final boolean ignoreCase;
-
/** Creates a new CommonGramsFilterFactory */
public CommonGramsFilterFactory(Map<String, String> args) {
super(args);
- commonWordFiles = get(args, "words");
- format = get(args, "format");
- ignoreCase = getBoolean(args, "ignoreCase", false);
- if (!args.isEmpty()) {
- throw new IllegalArgumentException("Unknown parameters: " + args);
- }
}
/** Default ctor for compatibility with SPI */
public CommonGramsFilterFactory() {
throw defaultCtorException();
}
- @Override
- public void inform(ResourceLoader loader) throws IOException {
- if (commonWordFiles != null) {
- if ("snowball".equalsIgnoreCase(format)) {
- commonWords = getSnowballWordSet(loader, commonWordFiles, ignoreCase);
- } else {
- commonWords = getWordSet(loader, commonWordFiles, ignoreCase);
- }
- } else {
- commonWords = EnglishAnalyzer.ENGLISH_STOP_WORDS_SET;
Review comment:
Ahh, this was the bug? Because this (default) path ignores `ignoreCase`?
--
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] vigyasharma commented on pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
vigyasharma commented on pull request #188:
URL: https://github.com/apache/lucene/pull/188#issuecomment-896183398
This PR is ready for review. It addresses JIRA - https://issues.apache.org/jira/browse/LUCENE-10008
--
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] mikemccand merged pull request #188: Lucene-10008: Respect ignoreCase flag in CommonGramsFilterFactory
Posted by GitBox <gi...@apache.org>.
mikemccand merged pull request #188:
URL: https://github.com/apache/lucene/pull/188
--
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