Solr
  1. Solr
  2. SOLR-6013

Fix method visibility of Evaluator, refactor DateFormatEvaluator for extensibility

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.7
    • Fix Version/s: 4.9, 6.0
    • Labels:
      None

      Description

      This is similar to issue 5981, the Evaluator class is declared as abstract, yet the parseParams method is package private? Surely this is an oversight, as I wouldn't expect everyone writing their own evaluators to have to deal with parsing the parameters.

      Similarly, I needed to refactor DateFormatEvaluator because I need to do some custom date math/parsing and it wasn't written in a way that I can extend it.

      Please review/apply my attached patch to the next version of Solr, ie: 4.8 or 4.9 if I must wait.

      Thanks!

      1. 0001-add-getters-for-datemathparser.patch
        2 kB
        Aaron LaBella
      2. 0001-change-method-access-to-protected.patch
        4 kB
        Aaron LaBella
      3. 0001-change-method-variable-visibility-and-refactor-for-extensibility.patch
        9 kB
        Aaron LaBella
      4. SOLR-6013.patch
        10 kB
        Shalin Shekhar Mangar

        Activity

        Hide
        Aaron LaBella added a comment -

        attaching the patch for review/comments.

        Show
        Aaron LaBella added a comment - attaching the patch for review/comments.
        Hide
        Aaron LaBella added a comment -

        one more small patch after fully testing the changes for extensibility

        Show
        Aaron LaBella added a comment - one more small patch after fully testing the changes for extensibility
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks Aaron. Some comments:

        1. DateFormatEvaluator methods such as evaluateWrapper, evaluateString, parseMathString, resolveMapper and getDateMathParser have no business being public but we can make them protected if you want. All of them should be marked as experimental API.
        2. Evaluator.parseParams should be protected not public.
        3. I don't like that we are creating a method such as getVariableWrapper. These things are not supposed to be pluggable and it should definitely not be public. We can mark it as protected along with a javadoc warning saying that this is experimental API. If I were writing Evaluator today, I'd just use a Callable instead.

        On an unrelated note, I wonder why we cache all available locales and timezones. If looking up locale/timezone is expensive then it can be done as-needed. I'll create an issue.

        Show
        Shalin Shekhar Mangar added a comment - Thanks Aaron. Some comments: DateFormatEvaluator methods such as evaluateWrapper, evaluateString, parseMathString, resolveMapper and getDateMathParser have no business being public but we can make them protected if you want. All of them should be marked as experimental API. Evaluator.parseParams should be protected not public. I don't like that we are creating a method such as getVariableWrapper. These things are not supposed to be pluggable and it should definitely not be public. We can mark it as protected along with a javadoc warning saying that this is experimental API. If I were writing Evaluator today, I'd just use a Callable instead. On an unrelated note, I wonder why we cache all available locales and timezones. If looking up locale/timezone is expensive then it can be done as-needed. I'll create an issue.
        Hide
        Aaron LaBella added a comment -

        Thanks Shalin,

        I'm attaching another patch to change the method accessors to protected (instead of public) and marked the methods as lucene.experimental. Let me know if there's anything else. Otherwise, can you, or someone else commit/push these patches into the 4.x branch so it makes the next release?

        Thanks

        Show
        Aaron LaBella added a comment - Thanks Shalin, I'm attaching another patch to change the method accessors to protected (instead of public) and marked the methods as lucene.experimental. Let me know if there's anything else. Otherwise, can you, or someone else commit/push these patches into the 4.x branch so it makes the next release? Thanks
        Hide
        Aaron LaBella added a comment -

        NOTE: the patches can be applied from oldest to newest

        Show
        Aaron LaBella added a comment - NOTE: the patches can be applied from oldest to newest
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks Aaron. Here's a single patch with all your changes. We typically attach a patch named as SOLR-xxx.patch and if/when we need to revise, we upload patch with the same name again. Jira will automatically gray out the older patches in the UI. This makes it easier to know the latest patch.

        I made three changes:

        1. FileListEntityProcessor.getDate was missing a return statement
        2. I removed the getters for VariableWrapper and made the attributes public final
        3. Changed DateFormatCacheKey to static protected.

        I will commit this shortly.

        Show
        Shalin Shekhar Mangar added a comment - Thanks Aaron. Here's a single patch with all your changes. We typically attach a patch named as SOLR-xxx.patch and if/when we need to revise, we upload patch with the same name again. Jira will automatically gray out the older patches in the UI. This makes it easier to know the latest patch. I made three changes: FileListEntityProcessor.getDate was missing a return statement I removed the getters for VariableWrapper and made the attributes public final Changed DateFormatCacheKey to static protected. I will commit this shortly.
        Hide
        ASF subversion and git services added a comment -

        Commit 1591573 from shalin@apache.org in branch 'dev/trunk'
        [ https://svn.apache.org/r1591573 ]

        SOLR-6013: Fix method visibility of Evaluator, refactor DateFormatEvaluator for extensibility

        Show
        ASF subversion and git services added a comment - Commit 1591573 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1591573 ] SOLR-6013 : Fix method visibility of Evaluator, refactor DateFormatEvaluator for extensibility
        Hide
        ASF subversion and git services added a comment -

        Commit 1591574 from shalin@apache.org in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1591574 ]

        SOLR-6013: Fix method visibility of Evaluator, refactor DateFormatEvaluator for extensibility

        Show
        ASF subversion and git services added a comment - Commit 1591574 from shalin@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1591574 ] SOLR-6013 : Fix method visibility of Evaluator, refactor DateFormatEvaluator for extensibility
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks Aaron!

        Show
        Shalin Shekhar Mangar added a comment - Thanks Aaron!
        Hide
        Aaron LaBella added a comment -

        Thanks Shalin, but, you forgot two of the public 'getter' methods that I added in the last patch to Evaluator.VariableWrapper:

        public String getVarName()

        { return this.varName; }

        public VariableResolver getVariableResolver()

        { return this.vr; }

        Can you fix/commit/close?
        Thanks.

        Aaron

        Show
        Aaron LaBella added a comment - Thanks Shalin, but, you forgot two of the public 'getter' methods that I added in the last patch to Evaluator.VariableWrapper: public String getVarName() { return this.varName; } public VariableResolver getVariableResolver() { return this.vr; } Can you fix/commit/close? Thanks. Aaron
        Hide
        Shalin Shekhar Mangar added a comment -

        you forgot two of the public 'getter' methods that I added in the last patch to Evaluator.VariableWrapper

        No, I didn't. Those two are now public final fields.

        Show
        Shalin Shekhar Mangar added a comment - you forgot two of the public 'getter' methods that I added in the last patch to Evaluator.VariableWrapper No, I didn't. Those two are now public final fields.
        Hide
        Aaron LaBella added a comment -

        Shalin, that's fine I suppose (sorry I didn't notice you changed them to public/final). I'm just wondering though, wouldn't it make sense to access the bean properties using traditional getter methods instead of accessing them directly? Just curious as to the reasoning of not providing the getters. In either case, I'm fine with whatever you decide and re-closing this issue.

        Thanks.

        Aaron

        Show
        Aaron LaBella added a comment - Shalin, that's fine I suppose (sorry I didn't notice you changed them to public/final). I'm just wondering though, wouldn't it make sense to access the bean properties using traditional getter methods instead of accessing them directly? Just curious as to the reasoning of not providing the getters. In either case, I'm fine with whatever you decide and re-closing this issue. Thanks. Aaron
        Hide
        Shalin Shekhar Mangar added a comment -

        Shalin, that's fine I suppose (sorry I didn't notice you changed them to public/final). I'm just wondering though, wouldn't it make sense to access the bean properties using traditional getter methods instead of accessing them directly? Just curious as to the reasoning of not providing the getters. In either case, I'm fine with whatever you decide and re-closing this issue.

        This is just a simple internal object with final values. There is no value added by getters here.

        Show
        Shalin Shekhar Mangar added a comment - Shalin, that's fine I suppose (sorry I didn't notice you changed them to public/final). I'm just wondering though, wouldn't it make sense to access the bean properties using traditional getter methods instead of accessing them directly? Just curious as to the reasoning of not providing the getters. In either case, I'm fine with whatever you decide and re-closing this issue. This is just a simple internal object with final values. There is no value added by getters here.

          People

          • Assignee:
            Shalin Shekhar Mangar
            Reporter:
            Aaron LaBella
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development