Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Function score: Add default to field_value_factor #10845

Merged
merged 1 commit into from Apr 28, 2015
Merged

Function score: Add default to field_value_factor #10845

merged 1 commit into from Apr 28, 2015

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 28, 2015

field_value_factor now takes a default that is used if the document doesn't
have a value for that field. It looks like:

"field_value_factor": {
  "field": "popularity",
  "default": 1
}

Closes #10841

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015

@dakrone I can't get ElasticsearchF running locally and more. Its been a few months since I tried. I'm getting:

[2015-04-27 20:58:29,955][INFO ][node                     ] [Kick-Ass] version[2.0.0-SNAPSHOT], pid[18345], build[${build/NA]
[2015-04-27 20:58:29,955][INFO ][node                     ] [Kick-Ass] initializing ...
[2015-04-27 20:58:29,960][INFO ][plugins                  ] [Kick-Ass] loaded [], sites []
[2015-04-27 20:58:30,022][INFO ][env                      ] [Kick-Ass] using [1] data paths, mounts [[/ (/dev/sda1)]], net usable_space [58.5gb], net total_space [227.1gb], spins? [no], types [ext4]
{2.0.0-SNAPSHOT}: Initialization Failed ...
1) ExceptionInInitializerError
    NullPointerException2) NoClassDefFoundError[Could not initialize class org.apache.lucene.analysis.ar.ArabicAnalyzer$DefaultSetHolder]

Is that something I'm doing?

@dakrone
Copy link
Member

dakrone commented Apr 28, 2015

@nik9000 no, this is a byproduct of #10717 that we're trying to figure out the best way around

For now, you can append -Des.security.manager.enabled=false when running from an IDE, or use mvn clean compile exec:exec to run ES (which also passes this parameter)

@dakrone dakrone self-assigned this Apr 28, 2015
@dakrone dakrone added the review label Apr 28, 2015
@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015

Thanks! That did it.

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015

I ran some local tests comparing this implementation which performs one auto-unboxing per document to a change that uses a primative instead. Ran across 1 million documents missing the field and both implementations were the same speed.

I just wanted to test my instinct that auto-unboxing has the overhead of a method call and that looks true.

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015

@dakrone, is there a chance you can review today? I want to backport to https://github.com/wikimedia/search-extra soon and I would prefer to backport from a merged pull request.

@jpountz
Copy link
Contributor

jpountz commented Apr 28, 2015

We have a similar parameter on _sort which is called missing, maybe this would be more consistent naming-wise? http://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-sort.html#_missing_values

jpountz
jpountz reviewed Apr 28, 2015
View reviewed changes
docs/reference/query-dsl/queries/function-score-query.asciidoc
@@ -193,6 +194,7 @@ There are a number of options for the `field_value_factor` function:
|`modifier` |Modifier to apply to the field value, can be one of: `none`, `log`,
`log1p`, `log2p`, `ln`, `ln1p`, `ln2p`, `square`, `sqrt`, or `reciprocal`.
Defaults to `none`.
|`default` |Default value used if the document doesn't have that field.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should make explicit that the modifier would still be applied in that case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015

We have a similar parameter on _sort which is called missing, maybe this would be more consistent naming-wise? http://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-sort.html#_missing_values

Sure.

@dakrone
Copy link
Member

dakrone commented Apr 28, 2015

Yeah, +1 on missing to be explicit that this takes the place of missing values. Other than the documentation thing Adrien suggested, I think this looks good to me!

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015

Woops forgot one place.

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015

OK - I think I got it but I'm rerunning all the tests just in case. And because I like to hear my laptop's fan.

@jpountz
Copy link
Contributor

jpountz commented Apr 28, 2015

This looks good to me too!

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015 

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 9:46.420s
[INFO] Finished at: Tue Apr 28 10:54:08 EDT 2015
[INFO] Final Memory: 38M/723M
[INFO] ------------------------------------------------------------------------

OK. Going to go and backport now!

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015

Thanks!

@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015

Do you want me to squash?

@dakrone
Copy link
Member

dakrone commented Apr 28, 2015

@nik9000 sure, if you squash I'll merge :)

@nik9000
Add default to field_value_factor
cb89a14 
field_value_factor now takes a default that is used if the document doesn't
have a value for that field. It looks like:
"field_value_factor": {
  "field": "popularity",
  "missing": 1
}

Closes elastic#10841
@nik9000
Copy link
Member Author

nik9000 commented Apr 28, 2015

Squashed.

@dakrone dakrone merged commit cb89a14 into elastic:master Apr 28, 2015
@kevinkluge kevinkluge removed the review label Apr 28, 2015
@nik9000 nik9000 deleted the defaults_for_field_value branch April 28, 2015 16:40
@clintongormley clintongormley mentioned this pull request May 8, 2015
Closed
@clintongormley clintongormley changed the title Add default to field_value_factor Function score: Add default to field_value_factor Jun 7, 2015
@gmarz gmarz mentioned this pull request Jun 10, 2015
Closed
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dakrone dakrone

Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v1.6.0 v2.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

field_value_factor should support a default value for documents that don't have the field
5 participants
@nik9000 @dakrone @jpountz @clintongormley @kevinkluge