Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      With successive changes, the edismax query parser has become more complex. It would be nice to refactor it to reduce code complexity, also to allow better extension and code reuse.

      1. qParserDiff.txt
        38 kB
        Tomás Fernández Löbbe
      2. SOLR-4208.patch
        111 kB
        Hoss Man
      3. SOLR-4208.patch
        109 kB
        Hoss Man
      4. SOLR-4208.patch
        105 kB
        Tomás Fernández Löbbe
      5. SOLR-4208.patch
        105 kB
        Tomás Fernández Löbbe
      6. SOLR-4208.patch
        98 kB
        Tomás Fernández Löbbe

        Issue Links

          Activity

          Hide
          Tomás Fernández Löbbe added a comment -

          A possible refactor:

          • I separated the ExtendedDismaxQParser so it can be extended.
          • I broke the "parse" method into smaller methods, easier to read.
          • Separated the configuration (parsing and all the actual fields) to a different class. DismaxQParser has too many configuration options, I think it's going to be clear if we keep those separately.
          • Using factory methods for the configuration and the ExtendedSolrQueryParser so that an extending class could change the implementation of those classes.
          • other minor changes.

          Thoughts?

          Show
          Tomás Fernández Löbbe added a comment - A possible refactor: I separated the ExtendedDismaxQParser so it can be extended. I broke the "parse" method into smaller methods, easier to read. Separated the configuration (parsing and all the actual fields) to a different class. DismaxQParser has too many configuration options, I think it's going to be clear if we keep those separately. Using factory methods for the configuration and the ExtendedSolrQueryParser so that an extending class could change the implementation of those classes. other minor changes. Thoughts?
          Hide
          Markus Jelsma added a comment -

          This is a very welcome change.The unit test TestExtendedDismaxParser.testAliasingBoost fails but also fails without your patch.
          +1

          Show
          Markus Jelsma added a comment - This is a very welcome change.The unit test TestExtendedDismaxParser.testAliasingBoost fails but also fails without your patch. +1
          Hide
          Tomás Fernández Löbbe added a comment -

          Does it fail with an assertion or an exception? It runs OK to me, with or without the patch. I'm running on Mac OS X and Java 6 on trunk.

          Show
          Tomás Fernández Löbbe added a comment - Does it fail with an assertion or an exception? It runs OK to me, with or without the patch. I'm running on Mac OS X and Java 6 on trunk.
          Hide
          Markus Jelsma added a comment -

          I am on trunk too. I get some exceptions like:

          [junit4:junit4]   2> 5475 T10 C0 oasc.SolrException.log SEVERE org.apache.solr.common.SolrException: org.apache.solr.search.SyntaxError: Field aliases lead to a cycle
          ...
          

          and,

          [junit4:junit4]   2> 6288 T10 oasc.SolrException.log SEVERE java.lang.NullPointerException
          [junit4:junit4]   2>            at org.apache.solr.handler.component.HttpShardHandlerFactory.close(HttpShardHandlerFactory.java:170)
          

          But they don't fail the unit test. The testAliasingBoost is marked as failed:

          [junit4:junit4] Tests with failures:
          [junit4:junit4]   - org.apache.solr.search.TestExtendedDismaxParser.testAliasingBoost
          
             <testcase classname="org.apache.solr.search.TestExtendedDismaxParser" name="testAliasingBoost" time="0.189">
                <error message="Exception during query" type="java.lang.RuntimeException">java.lang.RuntimeException: Exception during query
                  at __randomizedtesting.SeedInfo.seed([9B33524C2584B3F3:57A2A7EB2388F581]:0)
                  at org.apache.solr.SolrTestCaseJ4.assertQ(SolrTestCaseJ4.java:515)
                  at org.apache.solr.SolrTestCaseJ4.assertQ(SolrTestCaseJ4.java:482)
                  at org.apache.solr.search.TestExtendedDismaxParser.testAliasingBoost(TestExtendedDismaxParser.java:507)
                  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
                  at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
                  at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
                  at java.lang.reflect.Method.invoke(Method.java:597)
                  at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1559)
                  at com.carrotsearch.randomizedtesting.RandomizedRunner.access$600(RandomizedRunner.java:79)
                  at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:737)
                  at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:773)
                  at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:787)
                  at com.carrotsearch.randomizedtesting.rules.SystemPropertiesRestoreRule$1.evaluate(SystemPropertiesRestoreRule.java:53)
                  at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:50)
                  at org.apache.lucene.util.TestRuleFieldCacheSanity$1.evaluate(TestRuleFieldCacheSanity.java:51)
                  at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:46)
                  at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55)
                  at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:49)
                  at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70)
                  at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48)
                  at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
                  at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358)
                  at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:782)
                  at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:442)
                  at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:746)
                  at com.carrotsearch.randomizedtesting.RandomizedRunner$3.evaluate(RandomizedRunner.java:648)
                  at com.carrotsearch.randomizedtesting.RandomizedRunner$4.evaluate(RandomizedRunner.java:682)
                  at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:693)
                  at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
                  at com.carrotsearch.randomizedtesting.rules.SystemPropertiesRestoreRule$1.evaluate(SystemPropertiesRestoreRule.java:53)
                  at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:46)
                  at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:42)
                  at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55)
                  at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39)
                  at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39)
                  at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
                  at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:43)
                  at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48)
                  at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70)
                  at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:55)
                  at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
                  at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358)
                  at java.lang.Thread.run(Thread.java:662)
          Caused by: java.lang.RuntimeException: REQUEST FAILED: xpath=//result/doc[1]/str[@name=&apos;id&apos;]=42
                  xml response was: &lt;?xml version=&quot;1.0&quot; encoding=&quot;UTF-8&quot;?&gt;
          &lt;response&gt;
          &lt;lst name=&quot;responseHeader&quot;&gt;&lt;int name=&quot;status&quot;&gt;0&lt;/int&gt;&lt;int name=&quot;QTime&quot;&gt;1&lt;/int&gt;&lt;/lst&gt;&lt;result name=&quot;response&quot; numFound=&quot;2&quot; start=&quot;0&quot;&gt;&lt;doc&gt;&lt;str name=&quot;id&quot;&gt;47&lt;/str&gt;&lt;arr name=&quot;trait_ss&quot;&gt;&lt;str&gt;Pig&lt;/str&gt;&lt;/arr&gt;&lt;/doc&gt;&lt;doc&gt;&lt;str name=&quot;id&quot;&gt;42&lt;/str&gt;&lt;arr name=&quot;trait_ss&quot;&gt;&lt;str&gt;Tool&lt;/str&gt;&lt;str&gt;Obnoxious&lt;/str&gt;&lt;/arr&gt;&lt;str name=&quot;name&quot;&gt;Zapp Brannigan&lt;/str&gt;&lt;/doc&gt;&lt;/result&gt;
          &lt;/response&gt;
          
                  request was:f.myalias.qf=name+trait_ss^0.5&amp;q=Zapp+Pig&amp;qf=myalias&amp;defType=edismax
                  at org.apache.solr.SolrTestCaseJ4.assertQ(SolrTestCaseJ4.java:508)
                  ... 42 more
          </error>
             </testcase>
          

          I'm not sure what's going. This is a clean trunk check out.

          Show
          Markus Jelsma added a comment - I am on trunk too. I get some exceptions like: [junit4:junit4] 2> 5475 T10 C0 oasc.SolrException.log SEVERE org.apache.solr.common.SolrException: org.apache.solr.search.SyntaxError: Field aliases lead to a cycle ... and, [junit4:junit4] 2> 6288 T10 oasc.SolrException.log SEVERE java.lang.NullPointerException [junit4:junit4] 2> at org.apache.solr.handler.component.HttpShardHandlerFactory.close(HttpShardHandlerFactory.java:170) But they don't fail the unit test. The testAliasingBoost is marked as failed: [junit4:junit4] Tests with failures: [junit4:junit4] - org.apache.solr.search.TestExtendedDismaxParser.testAliasingBoost <testcase classname= "org.apache.solr.search.TestExtendedDismaxParser" name= "testAliasingBoost" time= "0.189" > <error message= "Exception during query" type= "java.lang.RuntimeException" >java.lang.RuntimeException: Exception during query at __randomizedtesting.SeedInfo.seed([9B33524C2584B3F3:57A2A7EB2388F581]:0) at org.apache.solr.SolrTestCaseJ4.assertQ(SolrTestCaseJ4.java:515) at org.apache.solr.SolrTestCaseJ4.assertQ(SolrTestCaseJ4.java:482) at org.apache.solr.search.TestExtendedDismaxParser.testAliasingBoost(TestExtendedDismaxParser.java:507) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1559) at com.carrotsearch.randomizedtesting.RandomizedRunner.access$600(RandomizedRunner.java:79) at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:737) at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:773) at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:787) at com.carrotsearch.randomizedtesting.rules.SystemPropertiesRestoreRule$1.evaluate(SystemPropertiesRestoreRule.java:53) at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:50) at org.apache.lucene.util.TestRuleFieldCacheSanity$1.evaluate(TestRuleFieldCacheSanity.java:51) at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:46) at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55) at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:49) at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70) at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48) at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358) at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:782) at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:442) at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:746) at com.carrotsearch.randomizedtesting.RandomizedRunner$3.evaluate(RandomizedRunner.java:648) at com.carrotsearch.randomizedtesting.RandomizedRunner$4.evaluate(RandomizedRunner.java:682) at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:693) at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) at com.carrotsearch.randomizedtesting.rules.SystemPropertiesRestoreRule$1.evaluate(SystemPropertiesRestoreRule.java:53) at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:46) at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:42) at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55) at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39) at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39) at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:43) at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48) at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70) at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:55) at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358) at java.lang. Thread .run( Thread .java:662) Caused by: java.lang.RuntimeException: REQUEST FAILED: xpath= //result/doc[1]/str[@name=&apos;id&apos;]=42 xml response was: &lt;?xml version=&quot;1.0&quot; encoding=&quot;UTF-8&quot;?&gt; &lt;response&gt; &lt;lst name=&quot;responseHeader&quot;&gt;&lt; int name=&quot;status&quot;&gt;0&lt;/ int &gt;&lt; int name=&quot;QTime&quot;&gt;1&lt;/ int &gt;&lt;/lst&gt;&lt;result name=&quot;response&quot; numFound=&quot;2&quot; start=&quot;0&quot;&gt;&lt;doc&gt;&lt;str name=&quot;id&quot;&gt;47&lt;/str&gt;&lt;arr name=&quot;trait_ss&quot;&gt;&lt;str&gt;Pig&lt;/str&gt;&lt;/arr&gt;&lt;/doc&gt;&lt;doc&gt;&lt;str name=&quot;id&quot;&gt;42&lt;/str&gt;&lt;arr name=&quot;trait_ss&quot;&gt;&lt;str&gt;Tool&lt;/str&gt;&lt;str&gt;Obnoxious&lt;/str&gt;&lt;/arr&gt;&lt;str name=&quot;name&quot;&gt;Zapp Brannigan&lt;/str&gt;&lt;/doc&gt;&lt;/result&gt; &lt;/response&gt; request was:f.myalias.qf=name+trait_ss^0.5&amp;q=Zapp+Pig&amp;qf=myalias&amp;defType=edismax at org.apache.solr.SolrTestCaseJ4.assertQ(SolrTestCaseJ4.java:508) ... 42 more </error> </testcase> I'm not sure what's going. This is a clean trunk check out.
          Hide
          Tomás Fernández Löbbe added a comment -

          I can see the "Field aliases lead to a cycle" exceptions. Those are being generated by the test testCyclicAliasing() and are expected exceptions (maybe the bad thing is that those are being logged)
          I also see the NPE, that seems to be generated when finishing the whole test, when shutting down the core.
          I can't see yet the failure with the testAliasingBoost yet, even trying with your seed.

          I'll continue looking into this.

          Show
          Tomás Fernández Löbbe added a comment - I can see the "Field aliases lead to a cycle" exceptions. Those are being generated by the test testCyclicAliasing() and are expected exceptions (maybe the bad thing is that those are being logged) I also see the NPE, that seems to be generated when finishing the whole test, when shutting down the core. I can't see yet the failure with the testAliasingBoost yet, even trying with your seed. I'll continue looking into this.
          Hide
          Tomás Fernández Löbbe added a comment -

          I also see the NPE, that seems to be generated when finishing the whole test, when shutting down the core.

          I saw this with other tests too, It looks like it may happen with all tests that extend SolrTestCaseJ4. I created SOLR-4218

          Show
          Tomás Fernández Löbbe added a comment - I also see the NPE, that seems to be generated when finishing the whole test, when shutting down the core. I saw this with other tests too, It looks like it may happen with all tests that extend SolrTestCaseJ4. I created SOLR-4218
          Hide
          Tomás Fernández Löbbe added a comment -

          Markus I have run the test on different environments and it keeps passing. Could you give me more details about your environment?
          If the same tests fails without the patch, maybe we should open a new Jira issue for it.

          Show
          Tomás Fernández Löbbe added a comment - Markus I have run the test on different environments and it keeps passing. Could you give me more details about your environment? If the same tests fails without the patch, maybe we should open a new Jira issue for it.
          Hide
          Markus Jelsma added a comment -

          Must have been my build again! A new check out passes completely. My own build still won't pass, even after i restored modified files and completely recompiled.

          Show
          Markus Jelsma added a comment - Must have been my build again! A new check out passes completely. My own build still won't pass, even after i restored modified files and completely recompiled.
          Hide
          Erik Hatcher added a comment -

          Thoughts?

          +1 to the idea, no question.

          Could you make the patch such that it's not an entire file replacement so we can more easily see what's changed exactly?

          Perhaps rather (or in conjunction with) making the parser easier/cleaner to "extends", we could make it open to Solr-style "plugins", where the edismax parser itself is still used directly, but various overrides/extensions can be plugged in (and perhaps made query-time changeable[!] by name lookup Solr-plugin-style; see HighlightComponent for example)

          Show
          Erik Hatcher added a comment - Thoughts? +1 to the idea, no question. Could you make the patch such that it's not an entire file replacement so we can more easily see what's changed exactly? Perhaps rather (or in conjunction with) making the parser easier/cleaner to "extends", we could make it open to Solr-style "plugins", where the edismax parser itself is still used directly, but various overrides/extensions can be plugged in (and perhaps made query-time changeable [!] by name lookup Solr-plugin-style; see HighlightComponent for example)
          Hide
          Tomás Fernández Löbbe added a comment -

          Could you make the patch such that it's not an entire file replacement so we can more easily see what's changed exactly?

          I think moving the parser to a different file is a good part of the refactor. I'm attaching the file "qParserDiff.txt" to show how the parser changed with the refactor, but I'm keeping it separately on SOLR-4208.patch.

          Perhaps rather (or in conjunction with) making the parser easier/cleaner to "extends", we could make it open to Solr-style "plugins", where the edismax parser itself is still used directly, but various overrides/extensions can be plugged in (and perhaps made query-time changeable[!] by name lookup Solr-plugin-style; see HighlightComponent for example)

          This is a good idea. When I started to work on my use case I initially thought on having something like the UpdateRequestProcessorChain at query time that would end with the EdismaxQParser itself (maybe a QParser that would add this component chain and use the EdismaxQParser as part of it), but that wouldn't give me enough points to extend, because the "parse()" method was going to be executed by the EdismaxQParser and there was no way of modifying its behavior. Maybe there is another way of add a "plugin style" that allows more customization?

          In the meantime I'm attaching a new patch with some more minor changes and some tests

          Show
          Tomás Fernández Löbbe added a comment - Could you make the patch such that it's not an entire file replacement so we can more easily see what's changed exactly? I think moving the parser to a different file is a good part of the refactor. I'm attaching the file "qParserDiff.txt" to show how the parser changed with the refactor, but I'm keeping it separately on SOLR-4208 .patch. Perhaps rather (or in conjunction with) making the parser easier/cleaner to "extends", we could make it open to Solr-style "plugins", where the edismax parser itself is still used directly, but various overrides/extensions can be plugged in (and perhaps made query-time changeable [!] by name lookup Solr-plugin-style; see HighlightComponent for example) This is a good idea. When I started to work on my use case I initially thought on having something like the UpdateRequestProcessorChain at query time that would end with the EdismaxQParser itself (maybe a QParser that would add this component chain and use the EdismaxQParser as part of it), but that wouldn't give me enough points to extend, because the "parse()" method was going to be executed by the EdismaxQParser and there was no way of modifying its behavior. Maybe there is another way of add a "plugin style" that allows more customization? In the meantime I'm attaching a new patch with some more minor changes and some tests
          Hide
          Erik Hatcher added a comment -

          Tomas - regarding the idea of using Solr plugin points into the edismax parser - on second thought it might be premature to put a plugin system in there at this point. Given the couple of examples you've added to the test cases, doing it with a subclass or an extension still requires Java coding and plugging in something, so again it's probably overkill to consider the plugin thing. But here's how it'd work (using the HighlighterComponent, such as formatter and encoders) where a plugin was responsible for the multi-lingual field logic of overriding the configuration object in your example, or a different plugin that was used to plug in a response to getFieldQuery for your other example. But I think leaving it how you've got it set up for subclassing works fine for now. +1

          Show
          Erik Hatcher added a comment - Tomas - regarding the idea of using Solr plugin points into the edismax parser - on second thought it might be premature to put a plugin system in there at this point. Given the couple of examples you've added to the test cases, doing it with a subclass or an extension still requires Java coding and plugging in something, so again it's probably overkill to consider the plugin thing. But here's how it'd work (using the HighlighterComponent, such as formatter and encoders) where a plugin was responsible for the multi-lingual field logic of overriding the configuration object in your example, or a different plugin that was used to plug in a response to getFieldQuery for your other example. But I think leaving it how you've got it set up for subclassing works fine for now. +1
          Hide
          Tomás Fernández Löbbe added a comment -

          Other minor change, the decision on whether to use the stopwords filter or not is extracted to a new method so that it can be overridden.

          Show
          Tomás Fernández Löbbe added a comment - Other minor change, the decision on whether to use the stopwords filter or not is extracted to a new method so that it can be overridden.
          Hide
          Markus Jelsma added a comment -

          Very nice +1

          Show
          Markus Jelsma added a comment - Very nice +1
          Hide
          Hoss Man added a comment -

          reading through things, i think this is definitely an improvement over what we have currently in terms of making things extensible in subclasses, and the test look great. adding sub-plugins seems like overkill at this point, probably best left for a future issue.

          i've improved tomas's latest patch slightly, by moving the ASL to the top of ExtendedDismaxQParser.java and setting up proper exception ignoring for those expected "Field aliases lead to a cycle" errors that were getting logged (the test already trie to make them ignored by using the "ignored_exception" magic string in the query, but these errors don't include the "q" in the exception message so it wasn't doing anything.

          I'm running tests & precommit now ... but the one thing i'd really like to do before committing is make sure all of these new/existing methods have decent javadocs ... if we're making these changes to make subclassing easier, we should make sure the jdocs are their for people who write subclasses so they understand what each method does.

          Show
          Hoss Man added a comment - reading through things, i think this is definitely an improvement over what we have currently in terms of making things extensible in subclasses, and the test look great. adding sub-plugins seems like overkill at this point, probably best left for a future issue. i've improved tomas's latest patch slightly, by moving the ASL to the top of ExtendedDismaxQParser.java and setting up proper exception ignoring for those expected "Field aliases lead to a cycle" errors that were getting logged (the test already trie to make them ignored by using the "ignored_exception" magic string in the query, but these errors don't include the "q" in the exception message so it wasn't doing anything. I'm running tests & precommit now ... but the one thing i'd really like to do before committing is make sure all of these new/existing methods have decent javadocs ... if we're making these changes to make subclassing easier, we should make sure the jdocs are their for people who write subclasses so they understand what each method does.
          Hide
          Hoss Man added a comment -

          going to try to get this into 4.1.

          Show
          Hoss Man added a comment - going to try to get this into 4.1.
          Hide
          Hoss Man added a comment -

          updated patch:

          • fixed test to not use forbidden api ("new String(byte[])" is a no-no and wasn't needed anyway)
          • added some javadocs to a few methods based on stuff that was obvious to me

          tomas & folks: can you take a look at the javadocs in the patch and let me know if i made in obvious mistakes or if you have suggestions for additional javadoc improvements?

          Show
          Hoss Man added a comment - updated patch: fixed test to not use forbidden api ("new String(byte[])" is a no-no and wasn't needed anyway) added some javadocs to a few methods based on stuff that was obvious to me tomas & folks: can you take a look at the javadocs in the patch and let me know if i made in obvious mistakes or if you have suggestions for additional javadoc improvements?
          Hide
          Commit Tag Bot added a comment -

          [trunk commit] Chris M. Hostetter
          http://svn.apache.org/viewvc?view=revision&revision=1430399

          SOLR-4208: ExtendedDismaxQParserPlugin has been refactored to make subclassing easier

          Show
          Commit Tag Bot added a comment - [trunk commit] Chris M. Hostetter http://svn.apache.org/viewvc?view=revision&revision=1430399 SOLR-4208 : ExtendedDismaxQParserPlugin has been refactored to make subclassing easier
          Hide
          Hoss Man added a comment -

          I've committed what we have so far to help move things forward in 4.1. We can always consider additional javadocs and other refactorings in future issues

          Committed revision 1430399.
          Committed revision 1430426.

          thanks tomas.

          Show
          Hoss Man added a comment - I've committed what we have so far to help move things forward in 4.1. We can always consider additional javadocs and other refactorings in future issues Committed revision 1430399. Committed revision 1430426. thanks tomas.
          Hide
          Commit Tag Bot added a comment -

          [branch_4x commit] Chris M. Hostetter
          http://svn.apache.org/viewvc?view=revision&revision=1430426

          SOLR-4208: ExtendedDismaxQParserPlugin has been refactored to make subclassing easier (merge r1430399)

          Show
          Commit Tag Bot added a comment - [branch_4x commit] Chris M. Hostetter http://svn.apache.org/viewvc?view=revision&revision=1430426 SOLR-4208 : ExtendedDismaxQParserPlugin has been refactored to make subclassing easier (merge r1430399)

            People

            • Assignee:
              Tomás Fernández Löbbe
              Reporter:
              Tomás Fernández Löbbe
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development