Expressions scripts in filter contexts throw exception #26429
Comments
dakrone
added
:Core/Infra/Scripting
|
@rjernst - It appears this bug might be causing Kibana build failures on master. Any ETA on a fix? |
|
I'm going to mark this as a blocker for 6.1 since we disabled a sizable chunk of integration tests in Kibana in order to get builds passing. I assume this is a regression that needs to be fixed for 6.1 anyway, but I'm really just blocking on resolution one way or another. |
|
@epixa Looking back at this again, I'm actually not sure filters make sense for expressions. Expressions only know how to read numeric values, and return numeric values. There was previously hacky "treat 0 as false and anything else as true" code in filter scripts, but that was removed with my refactoring to create a filter script context. Why can't Kibana use painless for filters? The same example script @dakrone gives in the issue description would work fine in painless. |
|
@rjernst Seems like a reasonable question to me, especially since we want people to use painless instead of lucene expressions for this stuff since it's designed more for these specific use cases rather than relying on hacky type coercion. That said, unless we can guarantee complete compatibility between the behaviors of expression-based filters and painless-based filters, this is going to be a breaking change for a lot of Kibana users, so I think we should preserve the existing behavior until 7.0. People can filter on Kibana scripted fields, which can use either expressions or painless scripts. At the very least, we'll need to make changes to Kibana to make it so only painless scripted fields can be filtered on, we'll need to start throwing deprecation notices for the existing expression filters, and we probably want to add a migration mechanism to the upgrade assistant for people to convert their existing scripted fields over. It's worth mentioning though, that we've never had any person (to my knowledge) that encountered unexpected behaviors with expression-based scripted fields in kibana. Was the 0->false coercion problematic from a performance or maintenance standpoint? Given the impact of the change on existing users and the amount of development that'll go into providing a bridge for those users going into 7.0, is it more practical for us to simply preserve the 0->false coercion as the documented behavior of how expressions work in a filter context? |
|
Part of the reason for the context work we have been doing in scripting is performance. When you do a coercion a million times (assuming one million docs being evaluated), the total time can be non-negligible. This is part of the reason expressions are currently faster in simple cases than painless. Once we have painless performance on par with expressions, I don't think there is any reason to keep expressions around. They were an early experiment in Lucene into doing scripted scoring, and will likely stay there for a long time. But having 2 languages in elasticsearch, especially one with limited functionality, is both confusing for users ("which one should I use?") and a maintenance burden on developers. Expanding on the latter, expressions require manual work for every new context we add. It is not simply a matter of "preserving coercion". There are a few classes necessary to be created and handled for every context expressions supports. So I think beginning the journey to remove uses of expressions is well worth the time investment. I can add in a hack for 6.1, but I would like to remove it for 7.0 (ie remove filter script support for expressions then). |
|
+1 to add a workaround for now and removing filter support for expressions in 7.0 (or even remove expressions entirely?) |
|
@Bargs What do you think? |
|
The benchmarks still show expressions as being faster than painless. I think it'd make sense for us to wait for painless to catch up to expressions before we talk about removing it entirely. |
|
Is it worth potential confusion due to users wondering "which one should I use"? |
|
We've had that confusion for a long time though. I think the issue may be moot - we'll likely work to closing that performance gap anyway. |
|
In Kibana I think we either need to support expressions everywhere or not at all. Having some scripted fields that work with filtering and some that don't will be incredibly confusing to kibana users who didn't set up the scripted fields in the first place. Removing expression support entirely will be a pretty big breaking change. Kibana maintainers will have to rewrite all of their scripts. I'm not sure how we could migrate them automatically. That might be ok as long as our reasons are good enough, breaking changes happen. But I think we need to be absolutely sure removing expressions doesn't make anything impossible that's already possible today. If expressions still outperform painless in certain scenarios, are there use cases where expressions are viable but painless is not? As to confusion over having two languages, I don't think it's a problem, for Kibana at least. In Kibana we default scripted fields to painless and make it clear that's the recommended choice. |
@nik9000 Not sure what you mean by that. Given the pervasiveness of expressions in Kibana described here, I think it is a worthwhile discussion to have. We need to be thinking far ahead on how to migrate users off of expressions. It is good that painless is the default. And in most cases I think an expressions should "just work" as a painless script, so I'm not that worried about transitioning. My concern over continuing to support expressions as filter scripts is the possibility for confusion by users. Because expression only return a double, we have to interpret that double, and cannot distinguish between "this was a boolean value" and "this was a double value". For example, if a user had an expression like
@Bargs This is simply not possible. Expressions already don't work in some contexts. For example, update scripts, reindex scripts, or anything else that doesn't return a numeric value. The only reason they worked before for filter scripts is this very old hack that existed within filter scripts which converted 0.0 to false and everything else to true. As I said before, I can add a hack back in just for expressions for filter scripts, but I don't want to do so unless there is agreement and a plan of action to eliminate this hack long term. Regardless of when expressions are deprecated and removed overall, I don't want expressions supporting filter scripts because of the ambiguities I have described here. |
I meant that we are likely to close the performance gap significantly during the 6.x release cycle so we might be able to remove expressions entirely in 7.0 so my point about waiting until Painless catches up might not matter because it will catch up. I agree with your concern about expressions in filters. I find the tricks that kibana plays with scripts to be a bit tricky and this sort of 0-as-false thing plays along. I'd like to avoid it if we can but you are right that the transition path is going to be fun.
Kibana has a slightly different meaning for the phrase "script context" then we do so we can have communications issues around this. One simplistic answer to this is "kibana doesn't care about those contexts". That isn't strictly true and is oversimplifying it gives you a sense as to why expressions work in all of kibanas script contexts. |
Yes, thank you @nik9000, this is what I meant. I should have been more specific and said: "In Kibana I think we either need to fully support expressions in "kibana scripted fields" or not at all. "Kibana scripted fields" aren't used for updating, reindexing, etc. So just to clarify my thoughts: if we remove the ability to filter with expressions in Elasticsearch I think we should also remove expression support from "Kibana scripted fields" entirely. I'm ok with that if we're sure we're not leaving any users up the creek without a paddle. |
|
@Bargs and I talked about this a bit, and he's going to proceed with deprecating expressions in Kibana scripted fields in 6.1 and removing them entirely from master. @rjernst Can you add a hack for this in 6.x so the existing behavior starts working again? Kibana is currently pinned to a month old commit of Elasticsearch in CI, so I'd like to undo that asap. |
|
Sure, this comment from you is enough of an agreement. I'll work on a PR soon. :) |
|
Awesome, thanks |
This commit adds a hack converting 0.0 to false and non-zero to true for expressions operating under a filter context. closes elastic#26429
) This commit adds a hack converting 0.0 to false and non-zero to true for expressions operating under a filter context. closes #26429
) This commit adds a hack converting 0.0 to false and non-zero to true for expressions operating under a filter context. closes #26429
In the master branch, if I do a query with an expression script like:
{ "query": { "function_score": { "query": { "constant_score": { "filter": { "bool": { "must": [ { "script": { "script": { "lang": "expression", "inline": "birth_date >= doc[\"birth_date\"].value", "params": { "birth_date": 14 }}}}]}}}}}}}I get the following error:
The error also says "painless" instead of "expressions" from
elasticsearch/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java
Line 111 in c075323
The text was updated successfully, but these errors were encountered: