Fix Lambdas in Painless to be Able to Use Top-Level Variables Such as params and doc

[jdconrad]

As the title says. See the tests for examples.

Relates (#21479)

As a bonus this also fixes a bug related to certain reserved variable names not working in a lambda.

Relates (#20869)

@s1monw I think I'm under the 1.5 hours you said I had :)

[s1monw]

@jdconrad 👍

[nik9000]

So I think this solves #21479 by giving Kibana a fully working way to do text manipulation to build its script fields without choking on normal syntax. The lambda is in the main scope and that closes over all the magic keywords like params and doc.

I think it is nice to have this as a proposal but I don't like it as much as making script query aware of something to compare to or making a built in native script that delegates to the regular script to make the value and does the comparison. Those solutions have the benefit of allowing us to reuse exactly the same script that is used during a scripted terms aggregation, making things like error reporting simpler. On the other hand this is certainly better than what we have now because it works for most script fields. Scripts with functions don't work but that is better than we have now.

I'm worried that this amounts to a dodge around the bigger issue in #21479 which is that ES contributors don't believe that kibana should make doing these queries easy at all so any sort of accommodation is a mistake.

If we want to give Kibana a way to work around #21479 in 5.1 while we come to a more permanent conclusion then I think this is the right thing to do because painless is experimental and we can remove this is later versions if we decide on a "better" solution. Also, I like the fixes allowing lambdas to close over the magic variables.

If we are going to merge this then it need yaml tests that set up the problem and the proposed work around for Kibana exactly to be sure that this makes sense in the context of the rest of the system which tends to have other sneaky stuff that doesn't come up in unit tests.

Now that i think about it, I wonder if Kibana actually needs the utility at all once you fix the parameter issue. They could do something like

boolean compare(Supplier<def> r) {return params.match == r.get());}
return compare(() -> {if(doc.whatever.value == 8) {return doc.foo.value + doc.bar.value} return doc.zot.value})

If we can do that then you have my whole hearted approval to get this in because then this is a pure positive change for painless. Kibana can use it to work around #21479 while we make a decision on it.

[jdconrad]

"Those solutions have the benefit of allowing us to reuse exactly the same script that is used during a scripted terms aggregation, making things like error reporting simpler."

This is exactly the opposite of the direction we're heading ultimately with per-context signatures. We should not encourage the re-use of scripts across contexts. If two contexts are so similar then they should be they should use the same signature.

[jdconrad]

"Now that i think about it, I wonder if Kibana actually needs the utility at all once you fix the parameter issue."

This is a good suggestion. I'm updating the PR now.

[nik9000]

If two contexts are so similar then they should be they should use the same signature.

I think we should do exactly that and the two solutions I proposed could be made to work that way when script context. I mean, I'm not sure we'll know until we build script contexts, but I think it'd work.

I opened #21479 because I'm uncomfortable with any application doing text manipulation on scripts at all. Without this solution Kibana breaks lots of stuff. With this solution Kibana keeps most painless features but breaks function definitions. We could allow you to define function inside of lambdas and maybe they'd work too but I feel like something else is going to come up. And then we've designed the language around this weird text manipulation use case.

[nik9000]

Left a nit about method naming. Otherwise this is great.

[nik9000]

Rename the method to something about captures I think now that the utility is gone.

[jdconrad]

Done.

[jdconrad]

@nik9000 Thanks for the reviews.

[s1monw]

this is a bugfix now and can potentially go in to 5.0 branch no?

[jdconrad]

@s1monw Yes, I will back port to both 5.1 and 5.0.

[jdconrad]

The hashes:

6.0 ced433e
5.x b3278b2
5.0 e2dae05

[jdconrad]

This issue has changed from the original to be a bug fix. Previously, lambdas in the main method were not accepting variables from the outer scope such as ctx, params, etc. Now, they allow those variables to be used inside of a lambda.

Example:

// Create a method that takes a form of lambda
boolean compare(Supplier s, def v) {return s.get() == v;} 
// Note how the lambda expression uses both params and doc as part of the lambda body now
return compare(() -> {if (params.size() > 1) {return doc.field.value;} else {return 0;}}, params.value);