Lucene - Core
  1. Lucene - Core
  2. LUCENE-5414

suggest module should not depend on expression module

    Details

    • Type: Wish Wish
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.6, 6.0
    • Fix Version/s: 4.7, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Currently our suggest module depends on the expression module just because the DocumentExpressionDictionary provides some util ctor to pass in an expression directly. That is a lot of dependency for little value IMO and pulls in lots of JARs. DocumentExpressionDictionary should only take a ValueSource instead.

      1. LUCENE-5414.patch
        35 kB
        Simon Willnauer
      2. LUCENE-5414.patch
        34 kB
        Simon Willnauer
      3. LUCENE-5414.patch
        10 kB
        Simon Willnauer
      4. LUCENE-5414.patch
        8 kB
        Simon Willnauer

        Activity

        Hide
        Simon Willnauer added a comment -

        here is a start - I am still after all these years not familiar with how we manage the maven stuff but I guess it somehow figures this out?

        Show
        Simon Willnauer added a comment - here is a start - I am still after all these years not familiar with how we manage the maven stuff but I guess it somehow figures this out?
        Hide
        Simon Willnauer added a comment -

        I wonder actually if we should rename that dictionary now seems like the wrong name for it if it doesn't take an expression anymore?

        Show
        Simon Willnauer added a comment - I wonder actually if we should rename that dictionary now seems like the wrong name for it if it doesn't take an expression anymore?
        Hide
        Robert Muir added a comment -

        +1 to just take ValueSource like this patch, I think it keeps our dependencies simpler and cleaner.

        I also think we could remove the test dep too (in the future or whatever), because the test only needs to show it consumes valuesource correctly.

        Show
        Robert Muir added a comment - +1 to just take ValueSource like this patch, I think it keeps our dependencies simpler and cleaner. I also think we could remove the test dep too (in the future or whatever), because the test only needs to show it consumes valuesource correctly.
        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        Simon Willnauer added a comment -

        another iteration... removed the deps to expression entirely. I will need to fix the javadocs and the class name still...

        Show
        Simon Willnauer added a comment - another iteration... removed the deps to expression entirely. I will need to fix the javadocs and the class name still...
        Hide
        Steve Rowe added a comment -

        I am still after all these years not familiar with how we manage the maven stuff but I guess it somehow figures this out?

        LUCENE-5217 recently added an Ant+Ivy->Maven dependencies converter that also defeats Maven's transitive dependency resolution mechanism by excluding all dependencies' direct dependencies in the <dependencyManagement> section of the grandparent POM.

        I ran ant get-maven-poms before and after the latest patch on this issue and compared the resulting suggest module's POMs. With the patch, the following dependencies are no longer present:

            <dependency>
              <groupId>org.apache.lucene</groupId>
              <artifactId>lucene-expressions</artifactId>
            </dependency>
        ...
            <dependency>
              <groupId>org.antlr</groupId>
              <artifactId>antlr-runtime</artifactId>
            </dependency>
            <dependency>
              <groupId>org.ow2.asm</groupId>
              <artifactId>asm</artifactId>
            </dependency>
            <dependency>
              <groupId>org.ow2.asm</groupId>
              <artifactId>asm-commons</artifactId>
            </dependency>
        

        Those last three dependencies are direct dependencies of the expressions module; since the Ant build currently pulls in everything from expressions/lib/, the Ant->Maven dependencies converter interprets them as direct dependencies of the suggest module.

        After applying the latest patch, all Lucene/Solr tests pass under Maven.

        Show
        Steve Rowe added a comment - I am still after all these years not familiar with how we manage the maven stuff but I guess it somehow figures this out? LUCENE-5217 recently added an Ant+Ivy->Maven dependencies converter that also defeats Maven's transitive dependency resolution mechanism by excluding all dependencies' direct dependencies in the <dependencyManagement> section of the grandparent POM. I ran ant get-maven-poms before and after the latest patch on this issue and compared the resulting suggest module's POMs. With the patch, the following dependencies are no longer present: <dependency> <groupId> org.apache.lucene </groupId> <artifactId> lucene-expressions </artifactId> </dependency> ... <dependency> <groupId> org.antlr </groupId> <artifactId> antlr-runtime </artifactId> </dependency> <dependency> <groupId> org.ow2.asm </groupId> <artifactId> asm </artifactId> </dependency> <dependency> <groupId> org.ow2.asm </groupId> <artifactId> asm-commons </artifactId> </dependency> Those last three dependencies are direct dependencies of the expressions module; since the Ant build currently pulls in everything from expressions/lib/ , the Ant->Maven dependencies converter interprets them as direct dependencies of the suggest module. After applying the latest patch, all Lucene/Solr tests pass under Maven.
        Hide
        Simon Willnauer added a comment -

        updated patch:

        • Renamed DocumentExpressionDictionary to DocumentValueSourceDictionary
        • Fixed JavaDocs
        • Fixed DocumentExpressionDictionaryFactory to still accept an expression. I also kept the name since it adds this functionality on top of the DocumentValueSourceDictionary.

        I am not set on the name we could also go for WeightedDocumentDictionary?

        Show
        Simon Willnauer added a comment - updated patch: Renamed DocumentExpressionDictionary to DocumentValueSourceDictionary Fixed JavaDocs Fixed DocumentExpressionDictionaryFactory to still accept an expression. I also kept the name since it adds this functionality on top of the DocumentValueSourceDictionary. I am not set on the name we could also go for WeightedDocumentDictionary?
        Hide
        Robert Muir added a comment -

        Cool naming. I like the name actually, we might have another way in the future to weight things.

        Show
        Robert Muir added a comment - Cool naming. I like the name actually, we might have another way in the future to weight things.
        Hide
        Simon Willnauer added a comment -

        next iteration - added example how to use the expression module for building the ValueSource to the directories JavaDocs to prevent confusion. I think it's ready

        Show
        Simon Willnauer added a comment - next iteration - added example how to use the expression module for building the ValueSource to the directories JavaDocs to prevent confusion. I think it's ready
        Hide
        Robert Muir added a comment -

        Looks great, thanks Simon.

        Show
        Robert Muir added a comment - Looks great, thanks Simon.
        Hide
        ASF subversion and git services added a comment -

        Commit 1561415 from Simon Willnauer in branch 'dev/trunk'
        [ https://svn.apache.org/r1561415 ]

        LUCENE-5414: Suggest module should not depend on expression module

        Show
        ASF subversion and git services added a comment - Commit 1561415 from Simon Willnauer in branch 'dev/trunk' [ https://svn.apache.org/r1561415 ] LUCENE-5414 : Suggest module should not depend on expression module
        Hide
        ASF subversion and git services added a comment -

        Commit 1561416 from Simon Willnauer in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1561416 ]

        LUCENE-5414: Suggest module should not depend on expression module

        Show
        ASF subversion and git services added a comment - Commit 1561416 from Simon Willnauer in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1561416 ] LUCENE-5414 : Suggest module should not depend on expression module
        Hide
        ASF subversion and git services added a comment -

        Commit 1561448 from Robert Muir in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1561448 ]

        LUCENE-5414: disable 3.x codec in test

        Show
        ASF subversion and git services added a comment - Commit 1561448 from Robert Muir in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1561448 ] LUCENE-5414 : disable 3.x codec in test
        Hide
        ASF subversion and git services added a comment -

        Commit 1561707 from Steve Rowe in branch 'dev/trunk'
        [ https://svn.apache.org/r1561707 ]

        LUCENE-5414: intellij config

        Show
        ASF subversion and git services added a comment - Commit 1561707 from Steve Rowe in branch 'dev/trunk' [ https://svn.apache.org/r1561707 ] LUCENE-5414 : intellij config
        Hide
        ASF subversion and git services added a comment -

        Commit 1561708 from Steve Rowe in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1561708 ]

        LUCENE-5414: intellij config (merged trunk r1561707)

        Show
        ASF subversion and git services added a comment - Commit 1561708 from Steve Rowe in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1561708 ] LUCENE-5414 : intellij config (merged trunk r1561707)

          People

          • Assignee:
            Simon Willnauer
            Reporter:
            Simon Willnauer
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development