Lucene - Core
  1. Lucene - Core
  2. LUCENE-6245

ConstantScoreQuery etc have crazy toString()'s

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.1, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      For backwards compatibility reasons, LUCENE-1518 gave Filter a default "crap" toString(String) impl of getClass().getSimpleName(). I don't think we should do this. It causes problems e.g. when filters are wrapped in ConstantScoreQuery or other places, because toString(String) does the wrong thing.

      Filter f = new MultiTermQueryWrapperFilter<WildcardQuery>(new WildcardQuery(new Term("foo", "b*ar")));
          
      System.out.println(f.toString()); // foo:b*ar
      System.out.println(f.toString("foo")); // MultiTermQueryWrapperFilter
      

      Instead i think that impl should be removed (leaving it abstract), and Query.toString() should be final for a hard break:

        /** Prints a query to a string, with <code>field</code> assumed to be the 
         * default field and omitted.
         */
        public abstract String toString(String field);
      
        /** Prints a query to a string. */
        @Override
        public ****final**** String toString() {
          return toString("");
        }
      

      having buggy toString's is worse than requiring a change in custom filters. It impacts all users rather than just expert ones. Also by doing this, all the current toString bugs in the codebase show up as compile errors.

        Activity

        Hide
        Adrien Grand added a comment -

        OK I see the issue. I wanted to keep Filter as backward-compatible as possible but I had not anticipated the bad effects on toString(). Your proposal sounds good to me, I'll work on a fix unless you already started.

        Show
        Adrien Grand added a comment - OK I see the issue. I wanted to keep Filter as backward-compatible as possible but I had not anticipated the bad effects on toString(). Your proposal sounds good to me, I'll work on a fix unless you already started.
        Hide
        Robert Muir added a comment -

        I didn't start on anything yet... I didn't know what other options we had.

        Its very surprising to me that Query.toString(String) is abstract but Query.toString() is not final, to me that is the root problem and fixing that means introducing a break. Anything else I think will be clumsy?

        Show
        Robert Muir added a comment - I didn't start on anything yet... I didn't know what other options we had. Its very surprising to me that Query.toString(String) is abstract but Query.toString() is not final, to me that is the root problem and fixing that means introducing a break. Anything else I think will be clumsy?
        Hide
        Adrien Grand added a comment -

        Right, I thought I could do it without introducing breaks but as you notice it does not work.

        Then maybe we should only target the merge of queries and filters for 6.0. I could back out LUCENE-1518 from 5.x. There is so much left to do that I don't think we would be ready anytime soon anyway, and maybe this merge of queries and filters could be a good reason to release 6.0 once we are done?

        Show
        Adrien Grand added a comment - Right, I thought I could do it without introducing breaks but as you notice it does not work. Then maybe we should only target the merge of queries and filters for 6.0. I could back out LUCENE-1518 from 5.x. There is so much left to do that I don't think we would be ready anytime soon anyway, and maybe this merge of queries and filters could be a good reason to release 6.0 once we are done?
        Hide
        Robert Muir added a comment -

        I think we should first fix the issue for trunk, and then think about the impact.

        My issue here is that breaking toString() functionality silently will affect many many users. I am not worried about making a clean compile-time break that only impacts custom queries/filters code. It only impacts advanced users and does so without confusing them.

        Show
        Robert Muir added a comment - I think we should first fix the issue for trunk, and then think about the impact. My issue here is that breaking toString() functionality silently will affect many many users. I am not worried about making a clean compile-time break that only impacts custom queries/filters code. It only impacts advanced users and does so without confusing them.
        Hide
        Robert Muir added a comment -

        Besides wanting to see the change come to fruition before 6.0, I also think it would be great to deprecate Filter, and not just rip it out. This would be a much easier transition for users I think.

        We can rip it out in trunk of course.

        Show
        Robert Muir added a comment - Besides wanting to see the change come to fruition before 6.0, I also think it would be great to deprecate Filter, and not just rip it out. This would be a much easier transition for users I think. We can rip it out in trunk of course.
        Hide
        Ryan Ernst added a comment - - edited

        Here is a patch which does as proposed. I also think this is a good solution for 5.x. Forcing users with custom filters to make a decision when compiling is better than hidden odd/changing behavior for most users.

        Show
        Ryan Ernst added a comment - - edited Here is a patch which does as proposed. I also think this is a good solution for 5.x. Forcing users with custom filters to make a decision when compiling is better than hidden odd/changing behavior for most users.
        Hide
        Adrien Grand added a comment -

        +1 to the patch

        Show
        Adrien Grand added a comment - +1 to the patch
        Hide
        ASF subversion and git services added a comment -

        Commit 1659982 from Ryan Ernst in branch 'dev/trunk'
        [ https://svn.apache.org/r1659982 ]

        LUCENE-6245: Force Filter subclasses to implement toString API from Query

        Show
        ASF subversion and git services added a comment - Commit 1659982 from Ryan Ernst in branch 'dev/trunk' [ https://svn.apache.org/r1659982 ] LUCENE-6245 : Force Filter subclasses to implement toString API from Query
        Hide
        ASF subversion and git services added a comment -

        Commit 1660028 from Ryan Ernst in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1660028 ]

        LUCENE-6245: Force Filter subclasses to implement toString API from Query (merged 1659982)

        Show
        ASF subversion and git services added a comment - Commit 1660028 from Ryan Ernst in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1660028 ] LUCENE-6245 : Force Filter subclasses to implement toString API from Query (merged 1659982)
        Hide
        Timothy Potter added a comment -

        Bulk close after 5.1 release

        Show
        Timothy Potter added a comment - Bulk close after 5.1 release

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development