Skip to content

Change long to float for Weight #1087

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Conversation

virgilxie
Copy link

Elasitcsearch itself accepts float type weight

@Mpdreamz
Copy link
Member

Mpdreamz commented Dec 5, 2014

ty for bringing this to our attention @virgilxie this PR has some issues though

The following unit tests fail:

Nest.Tests.Unit.Search.Query.Singles (1)
    FunctionScoreQueryTests.FunctionScoreQueryWithJustWeight    
Nest.Tests.Unit.Search.Sorting (3)
    FunctionScoreTests.TestBoostFactor  
    FunctionScoreTests.TestDecayFunction    
    FunctionScoreTests.TestScriptScore

We can't change the property in 1.x releases since it would break backwards compatibility this PR will need to target our 2.0 branch instead of develop.

and lastly can you sign the elasticsearch cla: http://www.elasticsearch.org/contributor-agreement/ so that we may pull your bits in ? 😄

Please let us know if you would like to take this on or would like us to do so (we'll still credit you in our changelog)

@virgilxie
Copy link
Author

Hi Martijn,

Thank you so much for your reply. I have signed CLA.

About the test (FunctionScoreQueryWithJustWeight and etc) :
The test asserts that the generated json expression equals raw json
expression.
But after we change the weight to accept float, the weight will be like 5.0
but not 5. We may have to modify the test for this change.

I don't really understand why it's will break the backwards compatibility
as the weight is just introduced by elasticsearch 1.4. If the user is using
1.3.x elasticsearch, will he be able to use the weight?
I think I can take this on as it's just an easy and minor fix.

Thank you for your great work on nest client!
Fei

2014-12-05 6:09 GMT-08:00 Martijn Laarman [email protected]:

ty for bringing this to our attention @virgilxie
https://github.com/virgilxie this PR has some issues though

The following unit tests fail:

Nest.Tests.Unit.Search.Query.Singles (1)
FunctionScoreQueryTests.FunctionScoreQueryWithJustWeight
Nest.Tests.Unit.Search.Sorting (3)
FunctionScoreTests.TestBoostFactor
FunctionScoreTests.TestDecayFunction
FunctionScoreTests.TestScriptScore

We can't change the property in 1.x releases since it would break
backwards compatibility this PR will need to target our 2.0 branch
instead of develop.

and lastly can you sign the elasticsearch cla:
http://www.elasticsearch.org/contributor-agreement/ so that we may pull
your bits in ? [image: 😄]

Please let us know if you would like to take this on or would like us to
do so (we'll still credit you in our changelog)


Reply to this email directly or view it on GitHub
#1087 (comment)
.

Fei (Virgil) Xie

Software Engineer @zazzle

[email protected]

Alumni, Tsinghua University, Carnegie Mellon University

Enable Functions in FunctionScoreQueryDescriptor accept FunctionScoreFunction<T>[] functions. So when you add multiple functions with different filters, you don't need to have FieldValueFactor for each of them. In the other words, I think we can only make  the following query with this modification. Let me know if I was wrong.
"query": {
    "function_score": {
      "filter": { 
        "term": { "city": "Barcelona" }
      },
      "functions": [ 
        {
              "field_value_factor": {
                "field": "score_US"
              }
        },
        {
          "filter": { "term": { "features": "wifi" }}, 
          "weight": 1
        },
        {
          "filter": { "term": { "features": "garden" }}, 
          "weight": 1
        },
        {
          "filter": { "term": { "features": "pool" }}, 
          "weight": 2 
        }
      ],
      "score_mode": "sum", 
    }
  }
@gmarz
Copy link
Contributor

gmarz commented Dec 9, 2014

@virgilxie it's not a backwards compatibility issue with Elasticsearch, but with NEST. By changing the property from long to float, we might break someones code who is already using Weight(long).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants