Fix filters for complex painless scripted fields

[Bargs]

Painless scripted fields that consisted of more than a single expression
would break when the user attempted to filter on them in Discover or
Visualize because of the naive way we were building them. We now wrap
the user's script in a lambda so that it can be as complex at they need
it to be. The only special exception is that the user cannot define
named functions since those cannot go inside a Painless lambda.

Note Make sure to use a recent build of ES when testing, you'll get an error if the ES PR referenced below isn't included in your build. ESVM's build is currently out of date.

Fixes #9024
Related elastic/elasticsearch#21635

[Bargs]

@nik9000 any chance you could glance over the painless I wrote in here and make sure I'm not doing anything stupid? Things seem to be working but I'm not a painless guru.

[nik9000]

Makes sense to me. I left a super minor thing.

[nik9000]

Technically you don't need a ; before a } in painless ever. Mad lexer hax.

Anyway, maybe only emit the one of these comparators that you need? It doesn't seem worth it to go through the trouble of building these methods if you aren't going to use them.

[Bargs]

Cool, thanks for the quick feedback!

[tbragin]

@Bargs I tried out a couple of scripts

Filtering works for this script

def m = /^([0-9]+)\..*$/.matcher(doc['clientip.keyword'].value);
if ( m.matches() ) {
   return Integer.parseInt(m.group(1))
} else {
   return 0
}

But not for this

def path = doc['url.keyword'].value;
if (path != null) {
    int lastSlashIndex = path.lastIndexOf('/');
    if (lastSlashIndex > 0) {
	return path.substring(lastSlashIndex+1)
    }
}

Resulting filter copied from UI

{
  "script": {
    "script": {
      "inline": "boolean compare(Supplier s, def v) {return s.get() == v;}\n                        compare(() -> { def path = doc['url.keyword'].value;\nif (path != null) {\n    int lastSlashIndex = path.lastIndexOf('/');\n    if (lastSlashIndex > 0) {\n\treturn path.substring(lastSlashIndex+1)\n    }\n} }, params.value);",
      "lang": "painless",
      "params": {
        "value": "profile"
      }
    }
  }
}

[nik9000]

What about

def path = doc['url.keyword'].value;
if (path != null) {
    int lastSlashIndex = path.lastIndexOf('/');
    if (lastSlashIndex > 0) {
	return path.substring(lastSlashIndex+1)
    }
}
return null

?

[tbragin]

@nik9000 That works! So is it paramount that a well-defined return value is provided for every execution path?

[nik9000]

Yeah, at least it is in methods. Maybe @jdconrad can talk about that? I
don't know why it is ok for the main function and not all functions in
general.

On Mon, Nov 21, 2016, 7:53 PM Tanya Bragin [email protected] wrote:

@nik9000 https://github.com/nik9000 That works! So is it paramount that
a well-defined return value is provided for every execution path?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9171 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AANLonbLflUFEVIFbjCT7t6_EEIEv0cfks5rAj0igaJpZM4K40-z
.

[tbragin]

So modifying the substring script to be what @nik9000 recommended, namely to ensure the substrings script always returned something, I was able to run all the script examples in the painless blog and filter by them with no problems using this PR on top of Kibana master against latest ES master:

[jdconrad]

As to the functions -- there isn't necessarily an obvious default return value for functions, unfortunately, as they can be void and return nothing or return a primitive type and maybe a default of 0 makes sense? But maybe that breaks some things. I'm not actually 100% sure as to the behavior of lambdas since Robert Muir did that work, but I would guess it's similar for them. They use something called LambdaMetaFactory (a Java class) that has an insane rule set that I can take a look at tomorrow.

[stacey-gammon]

refactor suggestion, but otherwise, lgtm

[stacey-gammon]

Maybe this could be refactored? getInlineScriptForPhraseFilter, or similar helper function on the field object, perhaps? Then it won't have to be written out three times. I was toying with the idea when I first wrote this fix, but it was going in right before the release so I opted for minimal changes, but now there's a bit more content to be refactored so might be worth it.

[Bargs]

@stacey-gammon Since this logic is highly specific to the phrase filter I was worried about spreading it to the field class, so I exported it as a separate function in the phrase module which can be reused in all three places. A nice side effect is that the test is no longer whitespace dependent, which I didn't like.

I also added some additional tests and included Nik's suggestion. Please take another look and let me know what you think.

[stacey-gammon]

lgtm