Allow different decay values depending on the score function

[matiassalles99]

Tackles #78887

Currently, when setting a decay value on a score function, you can only set a value between 0 and 1 exclusively, independently of the score function you're using.

// elasticsearch/server/src/main/java/org/elasticsearch/index/query/functionscore/DecayFunctionBuilder.java
protected DecayFunctionBuilder(String fieldName, Object origin, Object scale, Object offset, double decay) {
        if (fieldName == null) {
            throw new IllegalArgumentException("decay function: field name must not be null");
        }
        if (scale == null) {
            throw new IllegalArgumentException("decay function: scale must not be null");
        }
        if (decay <= 0 || decay >= 1.0) {
            throw new IllegalStateException("decay function: decay must be in range 0..1!");
        }
// ....

This makes sense with gauss and exp score functions, since they use ln(decay) in their formula, and ln(0) does not exist.
But when using linear functions the decay should be allowed to be 0, since the formula would not result in a division by 0.

I'm open to any suggestions on how to tackle it better or test it better

[elasticsearchmachine]

Pinging @elastic/es-search (Team:Search)

[benwtrent]

I think the change is mostly good. It clarifies the mathematical restrictions within the validations.

I mentioned one change required. We should only have a separate decay validator.

Additionally, It would be good to add some test lines to ScoreFunctionBuilderTests:

elasticsearch/server/src/test/java/org/elasticsearch/index/query/functionscore/ScoreFunctionBuilderTests.java

Lines 27 to 41 in 12ad399

	public void testIllegalArguments() {
	expectThrows(IllegalArgumentException.class, () -> new RandomScoreFunctionBuilder().seed(null));
	expectThrows(IllegalArgumentException.class, () -> new ScriptScoreFunctionBuilder((Script) null));
	expectThrows(IllegalArgumentException.class, () -> new FieldValueFactorFunctionBuilder((String) null));
	expectThrows(IllegalArgumentException.class, () -> new FieldValueFactorFunctionBuilder("").modifier(null));
	expectThrows(IllegalArgumentException.class, () -> new GaussDecayFunctionBuilder(null, "", "", ""));
	expectThrows(IllegalArgumentException.class, () -> new GaussDecayFunctionBuilder("", "", null, ""));
	expectThrows(IllegalArgumentException.class, () -> new GaussDecayFunctionBuilder("", "", null, "", randomDouble()));
	expectThrows(IllegalArgumentException.class, () -> new LinearDecayFunctionBuilder(null, "", "", ""));
	expectThrows(IllegalArgumentException.class, () -> new LinearDecayFunctionBuilder("", "", null, ""));
	expectThrows(IllegalArgumentException.class, () -> new LinearDecayFunctionBuilder("", "", null, "", randomDouble()));
	expectThrows(IllegalArgumentException.class, () -> new ExponentialDecayFunctionBuilder(null, "", "", ""));
	expectThrows(IllegalArgumentException.class, () -> new ExponentialDecayFunctionBuilder("", "", null, ""));
	expectThrows(IllegalArgumentException.class, () -> new ExponentialDecayFunctionBuilder("", "", null, "", randomDouble()));
	}

Something along the lines

expectThrows(IllegalArgumentException.class, () -> new ExponentialDecayFunctionBuilder("", "", "", "", 0.0));
expectThrows(IllegalArgumentException.class, () -> new ExponentialDecayFunctionBuilder("", "", "", "", 1.0));
expectThrows(IllegalArgumentException.class, () -> new GaussDecayFunctionBuilder("", "", "", "", 0.0));
expectThrows(IllegalArgumentException.class, () -> new GaussDecayFunctionBuilder("", "", "", "", 1.0));
expectThrows(IllegalArgumentException.class, () -> new LinearDecayFunctionBuilder("", "", "", "", 1.0));
expectThrows(IllegalArgumentException.class, () -> new LinearDecayFunctionBuilder("", "", "", "", -1.0));
// should not throw
LinearDecayFunctionBuilder("", "", "", "", 0.0);

[benwtrent]

This could simply be a validateDecay function. The null checks will always be universal.

But, I think this idea of a protected/Overridable function is a good one.

[matiassalles99]

@benwtrent Thanks for the comments! will work on it 💪

[benwtrent]

jenkins test this please

[benwtrent]

Once CI is ✅ I will give it a merge.

Thank you for your contribution!

[benwtrent]

@matiassalles99 please run ./gradlew spotlessApply locally and commit the changes. This will apply correct formatting. There are some formatting complaints by CI.

One day maybe we will just auto apply formatting via a workflow. But not yet :).

[matiassalles99]

Sure!

[benwtrent]

@elasticmachine update branch

[benwtrent]

jenkins test this please

[benwtrent]

🎉 🎉 🎉 🎉
Congratulations @matiassalles99 on your first pull-request to Elasticsearch! Thank you for the contribution!!
🎉 🎉 🎉 🎉