Solr
  1. Solr
  2. SOLR-6704

TrieDateField type drops schema properties in branch 4.10

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.10.2
    • Fix Version/s: 4.10.3
    • Component/s: None
    • Labels:
      None

      Description

      Properties set in the FieldType, like multiValued, docValues, etc are dropped on TrieDateField type.
      This is only happening in the 4.10 branch, 5x and trunk don't seem to have this issue.

      1. SOLR-6704.patch
        5 kB
        Tomás Fernández Löbbe
      2. SOLR-6704.patch
        4 kB
        Tomás Fernández Löbbe
      3. SOLR-6704-branch_4_10.patch
        2 kB
        Tomás Fernández Löbbe
      4. SOLR-6704-trunk.patch
        4 kB
        Tomás Fernández Löbbe
      5. testcases.patch
        3 kB
        Tomás Fernández Löbbe

        Activity

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

        I'm attaching two test cases that pass in trunk and 5x but fail in 4.10

        Show
        Tomás Fernández Löbbe added a comment - I'm attaching two test cases that pass in trunk and 5x but fail in 4.10
        Hide
        Tomás Fernández Löbbe added a comment -

        The problem seems to be that the "setArgs" should be delegating to the wrappedField. This is not an issue in 5x or trunk because the implementation was changed and TrieDateField is no longer decorating TrieField but extending it.
        I'm wondering if we are missing other methods in TrieDateField too. I'll take a look

        Show
        Tomás Fernández Löbbe added a comment - The problem seems to be that the "setArgs" should be delegating to the wrappedField. This is not an issue in 5x or trunk because the implementation was changed and TrieDateField is no longer decorating TrieField but extending it. I'm wondering if we are missing other methods in TrieDateField too. I'll take a look
        Hide
        Tomás Fernández Löbbe added a comment -

        Looks like this was caused by SOLR-6211. Steve Rowe, you proposed not calling super.init() from init() and instead override hasProperty(p) and isMultiValued(). properties are never set in the wrapped instance because setArgs is never called. If we call wrappedField.setArgs like I did in my initial patch, then we need to make sure anything that requires properties is delegated to wrappedField. If instead we do super.init() from init() this is not needed.
        Here is a patch (for branch 4.10.x) with these changes, old and new tests passing. Any thoughts?

        Show
        Tomás Fernández Löbbe added a comment - Looks like this was caused by SOLR-6211 . Steve Rowe , you proposed not calling super.init() from init() and instead override hasProperty(p) and isMultiValued() . properties are never set in the wrapped instance because setArgs is never called. If we call wrappedField.setArgs like I did in my initial patch, then we need to make sure anything that requires properties is delegated to wrappedField. If instead we do super.init() from init() this is not needed. Here is a patch (for branch 4.10.x) with these changes, old and new tests passing. Any thoughts?
        Hide
        Steve Rowe added a comment - - edited

        Yeah, SOLR-6211 did cause this - it was an incomplete fix. My thought then was that there should not be two active copies of properties, one in the wrapped impl and another in the containing class. I still think that's better; this is fixed in trunk and 5x because extending instead of wrapping results in just one properties copy.

        If we call wrappedField.setArgs like I did in my initial patch, then we need to make sure anything that requires properties is delegated to wrappedField. If instead we do super.init() from init() this is not needed.

        super.init() calls PrimitiveFieldType.init(), which conditionally sets the omit norms property (doing this would have fixed SOLR-6211), and then chains to FieldType.init(), which is a no-op. The only place setArgs() is called is from FieldTypePluginLoader.init(). The way your second patch fixes the problem is by removing the overridden hasProperty() and isMultiValued() methods on TrieDateField, so that queries are answered with the containing class's copy of properties instead of the wrapped class's copy.

        Here is a patch (for branch 4.10.x) with these changes, old and new tests passing. Any thoughts?

        +1, it's the simplest fix. In hindsight I should have gone this way with the SOLR-6211 fix.

        Thanks Tomás!

        Show
        Steve Rowe added a comment - - edited Yeah, SOLR-6211 did cause this - it was an incomplete fix. My thought then was that there should not be two active copies of properties, one in the wrapped impl and another in the containing class. I still think that's better; this is fixed in trunk and 5x because extending instead of wrapping results in just one properties copy. If we call wrappedField.setArgs like I did in my initial patch, then we need to make sure anything that requires properties is delegated to wrappedField. If instead we do super.init() from init() this is not needed. super.init() calls PrimitiveFieldType.init() , which conditionally sets the omit norms property (doing this would have fixed SOLR-6211 ), and then chains to FieldType.init() , which is a no-op. The only place setArgs() is called is from FieldTypePluginLoader.init() . The way your second patch fixes the problem is by removing the overridden hasProperty() and isMultiValued() methods on TrieDateField , so that queries are answered with the containing class's copy of properties instead of the wrapped class's copy. Here is a patch (for branch 4.10.x) with these changes, old and new tests passing. Any thoughts? +1, it's the simplest fix. In hindsight I should have gone this way with the SOLR-6211 fix. Thanks Tomás!
        Hide
        Tomás Fernández Löbbe added a comment -

        Thanks for reviewing. Will also include the tests in trunk and branch 5x

        Show
        Tomás Fernández Löbbe added a comment - Thanks for reviewing. Will also include the tests in trunk and branch 5x
        Hide
        Tomás Fernández Löbbe added a comment -

        Patch for trunk attached. It only includes the tests. This will also be merged to branch 5x

        Show
        Tomás Fernández Löbbe added a comment - Patch for trunk attached. It only includes the tests. This will also be merged to branch 5x
        Hide
        Tomás Fernández Löbbe added a comment -

        Patch for branch 4.10.x

        Show
        Tomás Fernández Löbbe added a comment - Patch for branch 4.10.x
        Hide
        ASF subversion and git services added a comment -

        Commit 1636980 from Tomás Fernández Löbbe in branch 'dev/trunk'
        [ https://svn.apache.org/r1636980 ]

        SOLR-6704: Added unit tests that uncovered TrieDateField bug in branch 4.10.x

        Show
        ASF subversion and git services added a comment - Commit 1636980 from Tomás Fernández Löbbe in branch 'dev/trunk' [ https://svn.apache.org/r1636980 ] SOLR-6704 : Added unit tests that uncovered TrieDateField bug in branch 4.10.x
        Hide
        ASF subversion and git services added a comment -

        Commit 1636985 from Tomás Fernández Löbbe in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1636985 ]

        SOLR-6704: Added unit tests that uncovered TrieDateField bug in branch 4.10.x

        Show
        ASF subversion and git services added a comment - Commit 1636985 from Tomás Fernández Löbbe in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1636985 ] SOLR-6704 : Added unit tests that uncovered TrieDateField bug in branch 4.10.x
        Hide
        ASF subversion and git services added a comment -

        Commit 1636998 from Tomás Fernández Löbbe in branch 'dev/branches/lucene_solr_4_10'
        [ https://svn.apache.org/r1636998 ]

        SOLR-6704: fixed bug in TrieDateField that was making Solr drop schema properties defined in the field type

        Show
        ASF subversion and git services added a comment - Commit 1636998 from Tomás Fernández Löbbe in branch 'dev/branches/lucene_solr_4_10' [ https://svn.apache.org/r1636998 ] SOLR-6704 : fixed bug in TrieDateField that was making Solr drop schema properties defined in the field type

          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