Solr
  1. Solr
  2. SOLR-3963

SOLR: map() does not allow passing sub-functions in 4,5 parameters

    Details

    • Type: Improvement Improvement
    • Status: Reopened
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 4.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      I want to do:

      boost=map(achievement_count,1,1000,recip(achievement_count,-.5,10,25),1)

      I want to return recip(achievement_count,-.5,10,25) if achievement_count is between 1 and 1,000. FOr any other values I want to return 1.

      I cannot get it to work. I get the error below. Interesting this does work:

      boost=recip(map(achievement_count,0,0,200),.5,10,25)

      It almost appears that map() cannot take a function.

      Specified argument was out of the range of valid values.
      Parameter name: value
      Description: An unhandled exception occurred during the execution of the current web request. Please review the stack trace for more information about the error and where it originated in the code.

      Exception Details: System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
      Parameter name: value

      Source Error:

      An unhandled exception was generated during the execution of the current web request. Information regarding the origin and location of the exception can be identified using the exception stack trace below.

      Stack Trace:

      [ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
      Parameter name: value]
      System.Web.HttpResponse.set_StatusDescription(String value) +5200522
      FacilityService.Controllers.FacilityController.ActionCompleted(String actionName, IFacilityResults results) +265
      FacilityService.Controllers.FacilityController.SearchByPointCompleted(IFacilityResults results) +25
      lambda_method(Closure , ControllerBase , Object[] ) +114
      System.Web.Mvc.Async.<>c_DisplayClass7.<BeginExecute>b_5(IAsyncResult asyncResult) +283
      System.Web.Mvc.Async.<>c_DisplayClass41.<BeginInvokeAsynchronousActionMethod>b_40(IAsyncResult asyncResult) +22
      System.Web.Mvc.Async.<>c_DisplayClass3b.<BeginInvokeActionMethodWithFilters>b_35() +120
      System.Web.Mvc.Async.<>c_DisplayClass51.<InvokeActionMethodFilterAsynchronously>b_4b() +452
      System.Web.Mvc.Async.<>c_DisplayClass39.<BeginInvokeActionMethodWithFilters>b_38(IAsyncResult asyncResult) +15
      System.Web.Mvc.Async.<>c_DisplayClass2c.<BeginInvokeAction>b_22() +33
      System.Web.Mvc.Async.<>c_DisplayClass27.<BeginInvokeAction>b_24(IAsyncResult asyncResult) +240
      System.Web.Mvc.<>c_DisplayClass19.<BeginExecuteCore>b_14(IAsyncResult asyncResult) +28
      System.Web.Mvc.Async.<>c_DisplayClass4.<MakeVoidDelegate>b_3(IAsyncResult ar) +15
      System.Web.Mvc.AsyncController.EndExecuteCore(IAsyncResult asyncResult) +63
      System.Web.Mvc.Async.<>c_DisplayClass4.<MakeVoidDelegate>b_3(IAsyncResult ar) +15
      System.Web.Mvc.<>c_DisplayClassb.<BeginProcessRequest>b_4(IAsyncResult asyncResult) +42
      System.Web.Mvc.Async.<>c_DisplayClass4.<MakeVoidDelegate>b_3(IAsyncResult ar) +15
      System.Web.CallHandlerExecutionStep.OnAsyncHandlerCompletion(IAsyncResult ar) +282

        Activity

        Hide
        Hoss Man added a comment -

        Bill: i'm not understanding the error message you've pposted at all – it seems to be from some C# client code used to talk to solr?

        can you post the actual error message logged by solr? preferably with the (java) stack trace?

        FWIW: https://wiki.apache.org/solr/FunctionQuery#map

        map(x,min,max,target) maps any values of the function x that fall within min and max inclusive to target. min,max,target are constants. It outputs the field's value if it does not fall between min and max.

        ...it may be possible to modify the function so min/max/target could be functions, but as designed it does not and you should get a clear error message about that.

        Show
        Hoss Man added a comment - Bill: i'm not understanding the error message you've pposted at all – it seems to be from some C# client code used to talk to solr? can you post the actual error message logged by solr? preferably with the (java) stack trace? FWIW: https://wiki.apache.org/solr/FunctionQuery#map map(x,min,max,target) maps any values of the function x that fall within min and max inclusive to target. min,max,target are constants. It outputs the field's value if it does not fall between min and max. ...it may be possible to modify the function so min/max/target could be functions, but as designed it does not and you should get a clear error message about that.
        Hide
        Bill Bell added a comment -

        This is the error we are seeing with this query:

        http://localhost:8983/solr/select/?q.alt=*:*&start=0&rows=10&indent=on&defType=edismax&boost=map(achievement_count,1,1000,recip(achievement_count,-0.5,10,25),1)

        Problem accessing /solr/facility/select/. Reason:

        For input string: "recip(achievement_count"

        java.lang.NumberFormatException: For input string: "recip(achievement_count"
        at sun.misc.FloatingDecimal.readJavaFormatString(FloatingDecimal.java:1222)
        at java.lang.Float.parseFloat(Float.java:422)
        at org.apache.solr.search.FunctionQParser.parseFloat(FunctionQParser.java:134)
        at org.apache.solr.search.ValueSourceParser$9.parse(ValueSourceParser.java:189)
        at org.apache.solr.search.FunctionQParser.parseValueSource(FunctionQParser.java:363)
        at org.apache.solr.search.FunctionQParser.parse(FunctionQParser.java:67)
        at org.apache.solr.search.QParser.getQuery(QParser.java:143)
        at org.apache.solr.search.ExtendedDismaxQParser.parse(ExtendedDismaxQParserPlugin.java:389)
        at org.apache.solr.search.QParser.getQuery(QParser.java:143)
        at org.apache.solr.handler.component.QueryComponent.prepare(QueryComponent.java:105)
        at org.apache.solr.handler.component.SearchHandler.handleRequestBody(SearchHandler.java:165)
        at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:129)
        at org.apache.solr.core.SolrCore.execute(SolrCore.java:1376)
        at org.apache.solr.servlet.SolrDispatchFilter.execute(SolrDispatchFilter.java:365)
        at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:260)
        at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1337)
        at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:484)
        at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:119)
        at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:524)
        at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:233)
        at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1065)
        at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:413)
        at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:192)
        at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:999)
        at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:117)
        at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:250)
        at org.eclipse.jetty.server.handler.HandlerCollection.handle(HandlerCollection.java:149)
        at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:111)
        at org.eclipse.jetty.server.Server.handle(Server.java:351)
        at org.eclipse.jetty.server.AbstractHttpConnection.handleRequest(AbstractHttpConnection.java:454)
        at org.eclipse.jetty.server.BlockingHttpConnection.handleRequest(BlockingHttpConnection.java:47)
        at org.eclipse.jetty.server.AbstractHttpConnection.headerComplete(AbstractHttpConnection.java:890)
        at org.eclipse.jetty.server.AbstractHttpConnection$RequestHandler.headerComplete(AbstractHttpConnection.java:944)
        at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:634)
        at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:230)
        at org.eclipse.jetty.server.BlockingHttpConnection.handle(BlockingHttpConnection.java:66)
        at org.eclipse.jetty.server.bio.SocketConnector$ConnectorEndPoint.run(SocketConnector.java:254)
        at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:599)
        at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:534)
        at java.lang.Thread.run(Thread.java:662)

        Show
        Bill Bell added a comment - This is the error we are seeing with this query: http://localhost:8983/solr/select/?q.alt=*:*&start=0&rows=10&indent=on&defType=edismax&boost=map(achievement_count,1,1000,recip(achievement_count,-0.5,10,25),1 ) Problem accessing /solr/facility/select/. Reason: For input string: "recip(achievement_count" java.lang.NumberFormatException: For input string: "recip(achievement_count" at sun.misc.FloatingDecimal.readJavaFormatString(FloatingDecimal.java:1222) at java.lang.Float.parseFloat(Float.java:422) at org.apache.solr.search.FunctionQParser.parseFloat(FunctionQParser.java:134) at org.apache.solr.search.ValueSourceParser$9.parse(ValueSourceParser.java:189) at org.apache.solr.search.FunctionQParser.parseValueSource(FunctionQParser.java:363) at org.apache.solr.search.FunctionQParser.parse(FunctionQParser.java:67) at org.apache.solr.search.QParser.getQuery(QParser.java:143) at org.apache.solr.search.ExtendedDismaxQParser.parse(ExtendedDismaxQParserPlugin.java:389) at org.apache.solr.search.QParser.getQuery(QParser.java:143) at org.apache.solr.handler.component.QueryComponent.prepare(QueryComponent.java:105) at org.apache.solr.handler.component.SearchHandler.handleRequestBody(SearchHandler.java:165) at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:129) at org.apache.solr.core.SolrCore.execute(SolrCore.java:1376) at org.apache.solr.servlet.SolrDispatchFilter.execute(SolrDispatchFilter.java:365) at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:260) at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1337) at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:484) at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:119) at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:524) at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:233) at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1065) at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:413) at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:192) at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:999) at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:117) at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:250) at org.eclipse.jetty.server.handler.HandlerCollection.handle(HandlerCollection.java:149) at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:111) at org.eclipse.jetty.server.Server.handle(Server.java:351) at org.eclipse.jetty.server.AbstractHttpConnection.handleRequest(AbstractHttpConnection.java:454) at org.eclipse.jetty.server.BlockingHttpConnection.handleRequest(BlockingHttpConnection.java:47) at org.eclipse.jetty.server.AbstractHttpConnection.headerComplete(AbstractHttpConnection.java:890) at org.eclipse.jetty.server.AbstractHttpConnection$RequestHandler.headerComplete(AbstractHttpConnection.java:944) at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:634) at org.eclipse.jetty.http.HttpParser.parseAvailable(HttpParser.java:230) at org.eclipse.jetty.server.BlockingHttpConnection.handle(BlockingHttpConnection.java:66) at org.eclipse.jetty.server.bio.SocketConnector$ConnectorEndPoint.run(SocketConnector.java:254) at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:599) at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:534) at java.lang.Thread.run(Thread.java:662)
        Hide
        Hoss Man added a comment -

        boost=map(achievement_count,1,1000,recip(achievement_count,-0.5,10,25),1)

        java.lang.NumberFormatException: For input string: "recip(achievement_count"

        right ... and as i mentioned, as documented the map function only allows nested functions in it's first param. all of the other parmas only accept constant numbers – and "recip(achievement_count,-0.5,10,25)" is definitely not a valid number.

        So there doesn't seem to be any bug here, correct? .. i's just a question of wether map() could be improved to accept nested functions for it's other arguments.

        Show
        Hoss Man added a comment - boost=map(achievement_count,1,1000,recip(achievement_count,-0.5,10,25),1) java.lang.NumberFormatException: For input string: "recip(achievement_count" right ... and as i mentioned, as documented the map function only allows nested functions in it's first param. all of the other parmas only accept constant numbers – and "recip(achievement_count,-0.5,10,25)" is definitely not a valid number. So there doesn't seem to be any bug here, correct? .. i's just a question of wether map() could be improved to accept nested functions for it's other arguments.
        Hide
        Bill Bell added a comment -

        Yep. Enhancement request.

        Bill Bell
        Sent from mobile

        Show
        Bill Bell added a comment - Yep. Enhancement request. Bill Bell Sent from mobile
        Hide
        Bill Bell added a comment -

        Patch to include new RangeMapFloatFunction2 in Lucene for map(). It allows ValueSource for 4,5 parameters like the 1st parameter.

        Test case shown:

        ant -Dtestcase=TestValueSources clean test

        http://localhost:8983/solr/select?q=*:*&fl=price,map(price,0,100,log(add(price,.1)),sum(price,.5))

        Show
        Bill Bell added a comment - Patch to include new RangeMapFloatFunction2 in Lucene for map(). It allows ValueSource for 4,5 parameters like the 1st parameter. Test case shown: ant -Dtestcase=TestValueSources clean test http://localhost:8983/solr/select?q=*:*&fl=price,map(price,0,100,log(add(price,.1)),sum(price,.5 ))
        Hide
        Bill Bell added a comment -

        Based on Revision 1408031

        Show
        Bill Bell added a comment - Based on Revision 1408031
        Hide
        Bill Bell added a comment -

        Since you were commenting on it.

        I was scared about changing RangeMapFloatFunction.java and just created another one: RangeMapFloatFunction2.java

        Show
        Bill Bell added a comment - Since you were commenting on it. I was scared about changing RangeMapFloatFunction.java and just created another one: RangeMapFloatFunction2.java
        Hide
        Bill Bell added a comment -

        This does not break anything, just enhances map() can we get it committed?

        Show
        Bill Bell added a comment - This does not break anything, just enhances map() can we get it committed?
        Hide
        Bill Bell added a comment -

        Added map() tests.

        Show
        Bill Bell added a comment - Added map() tests.
        Hide
        Hoss Man added a comment -

        Hey Bill,

        First off, some specific comments on your patch...

        • your tests look pretty good to me
        • your description and toString methods don't look quite right – you should be calling target.description() and target.toString(doc)
          • both should also be using defaultVal – that looks like an oversight in RangeMapFloatFunction that you copied over
        • the class level javadoc comment makes no sense ... also a bug copy/pasted from RangeMapFloatFunction it seems
        • instead of a new "RangeMapFloatFunction2" i think it would make a lot more sense to just change "RangeMapFloatFunction" to use your new code and modify the existing constructor to call your new constructor wrapping the constants in ConstValueSource instances
          • while we're at it we can fix that javadoc bug and the oversight of ignoring defaultVal in description & toString
        • if we're going to support ValueSources in target & default, is there any reason not to support ValueSources for min & max as well?

        Second: some broader comments on the overall idea that occured to me while reading your patch...

        The changes you are proposing are definitely more general purpose then the current implementation – but the trade off is that (in theory) using constant values is faster then dealing with nested ValueSource objects because of the method call overhead. so (in theory) making this change adversely affects people who are currently using constant values. That doesn't mean it shouldn't be done – but it's worthwhile taking a moment to think about wether there is a best of both worlds situation.

        Unless i'm missing something, what you want to do...

        map(nested(...),min,max,target(...),default(...))

        ...should already possible using something like...

        if(map(nested(...),min,max,1,0),target(...),default(...))

        ...and that should have roughly the same performance as your your suggested change, w/o affecting the performance for people who are only using constants.

        So perhaps we should actually just leave the code in the map(...) function alone, and instead improve it's docs to clarify that for people who want more complex non-constant values they can use that if(...) pattern.

        what do you think?

        Show
        Hoss Man added a comment - Hey Bill, First off, some specific comments on your patch... your tests look pretty good to me your description and toString methods don't look quite right – you should be calling target.description() and target.toString(doc) both should also be using defaultVal – that looks like an oversight in RangeMapFloatFunction that you copied over the class level javadoc comment makes no sense ... also a bug copy/pasted from RangeMapFloatFunction it seems instead of a new "RangeMapFloatFunction2" i think it would make a lot more sense to just change "RangeMapFloatFunction" to use your new code and modify the existing constructor to call your new constructor wrapping the constants in ConstValueSource instances while we're at it we can fix that javadoc bug and the oversight of ignoring defaultVal in description & toString if we're going to support ValueSources in target & default, is there any reason not to support ValueSources for min & max as well? Second: some broader comments on the overall idea that occured to me while reading your patch... The changes you are proposing are definitely more general purpose then the current implementation – but the trade off is that (in theory) using constant values is faster then dealing with nested ValueSource objects because of the method call overhead. so (in theory) making this change adversely affects people who are currently using constant values. That doesn't mean it shouldn't be done – but it's worthwhile taking a moment to think about wether there is a best of both worlds situation. Unless i'm missing something, what you want to do... map(nested(...),min,max,target(...),default(...)) ...should already possible using something like... if(map(nested(...),min,max,1,0),target(...),default(...)) ...and that should have roughly the same performance as your your suggested change, w/o affecting the performance for people who are only using constants. So perhaps we should actually just leave the code in the map(...) function alone, and instead improve it's docs to clarify that for people who want more complex non-constant values they can use that if(...) pattern. what do you think?
        Hide
        Bill Bell added a comment -

        I personally think we should make all parameters for map() allow for ValueSource. Even though in this case if() may work, it gets very complicated when you have several nested sum/product that you want to use. I also think it should be like this and was an oversight to begin with. if() is a clever way to solve the issue, but in my opinion all functions should support nested functions if possible. The code could easily switch to Constant using a short-circuit approach (if parenthesis) to increase performance.

        Thoughts?

        Show
        Bill Bell added a comment - I personally think we should make all parameters for map() allow for ValueSource. Even though in this case if() may work, it gets very complicated when you have several nested sum/product that you want to use. I also think it should be like this and was an oversight to begin with. if() is a clever way to solve the issue, but in my opinion all functions should support nested functions if possible. The code could easily switch to Constant using a short-circuit approach (if parenthesis) to increase performance. Thoughts?
        Hide
        Hoss Man added a comment -

        I also think it should be like this and was an oversight to begin with.

        it definitely wasn't an oversight – there are many functions like this where only constants are accepted for some params – as i said: the motivation was optimizing the common case, since more complicated functions could be used for the more complicated cases (the linear float function for example can trivially be implemented using sum() and prod(), but it's optimized for the common linear case of slope & intercept being constants)

        I personally think we should make all parameters for map() allow for ValueSource.

        For ease of use, I agree ... i just wanted to sound out the possible performance concerns.

        The code could easily switch to Constant using a short-circuit approach (if parenthesis) to increase performance.

        I'm not sure how easy that would be... the VM could/should probably do it once it notices that the method call is returning a constant, but i can't think of any (clean) way write the code to do that ... if you want to take a crack at that would be awesome, but maybe tackle that in a distinct issue?

        in the meantime: if you want to update your patch to deal with the other issues i noted, i'm happy to commit (unless someone else chimes in with objections about hte performance issue first)

        Show
        Hoss Man added a comment - I also think it should be like this and was an oversight to begin with. it definitely wasn't an oversight – there are many functions like this where only constants are accepted for some params – as i said: the motivation was optimizing the common case, since more complicated functions could be used for the more complicated cases (the linear float function for example can trivially be implemented using sum() and prod(), but it's optimized for the common linear case of slope & intercept being constants) I personally think we should make all parameters for map() allow for ValueSource. For ease of use, I agree ... i just wanted to sound out the possible performance concerns. The code could easily switch to Constant using a short-circuit approach (if parenthesis) to increase performance. I'm not sure how easy that would be... the VM could/should probably do it once it notices that the method call is returning a constant, but i can't think of any (clean) way write the code to do that ... if you want to take a crack at that would be awesome, but maybe tackle that in a distinct issue? in the meantime: if you want to update your patch to deal with the other issues i noted, i'm happy to commit (unless someone else chimes in with objections about hte performance issue first)
        Hide
        David Smiley added a comment -

        This is marked as fixed against 4.0 (even the Wiki says this). But I'm not seeing it in the code in trunk, nor 4x branch, no reference in CHANGES.txt either.

        Show
        David Smiley added a comment - This is marked as fixed against 4.0 (even the Wiki says this). But I'm not seeing it in the code in trunk, nor 4x branch, no reference in CHANGES.txt either.
        Hide
        Hoss Man added a comment -

        hmmm... good catch david, not sure when/how that happened.

        as far as i know this situation hasn't changed since my last comment: happy to commit if we clean up the patch a bit.

        Show
        Hoss Man added a comment - hmmm... good catch david, not sure when/how that happened. as far as i know this situation hasn't changed since my last comment: happy to commit if we clean up the patch a bit.

          People

          • Assignee:
            Hoss Man
            Reporter:
            Bill Bell
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development