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.

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

Script in terms aggregation that is not a value script should fail with descriptive failure message #11728

Closed
brwe opened this issue Jun 17, 2015 · 7 comments
Labels
:Analytics/Aggregations Aggregations >enhancement help wanted adoptme Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)

Comments

@brwe
Copy link
Contributor

brwe commented Jun 17, 2015

When a script is not a value script in a terms aggregation then the request must not contain the "fields" parameter.
However, there is no warning to the user if the request has the "fields" parameter anyway. The aggregation will just fail with cryptic error messages or even worse just silently not do the right thing.

This fails with an ArrayIndexOutOfBoundsException:

{
  "aggs": {
    "2": {
      "terms": {
        "field": "label",
        "script": "doc['label'].value"
      }
    }
  }
}

This silently just does not do the right thing (returns one bucket with key 0 and all docs in it):

{
  "aggs": {
    "2": {
      "terms": {
        "field": "label",
        "script": "_index['text']['the'].tf()"
      }
    }
  }
}
@luckywin
Copy link

I have same issue. Value script in terms aggregation breaks migration from terms facet (I've already posted question about such migration https://stackoverflow.com/questions/31006983/migration-from-terms-facet-to-terms-aggregation)

@brwe
Copy link
Contributor Author

brwe commented Jun 24, 2015

@luckywin just to be sure: you know that removing the "fields" parameter in the aggregation should make your script work?

@luckywin
Copy link

@brwe yes, I knew this, thank you. After removing "field" parameter, agg starts responding with "true" and "false" as a result of aggregation, and this is not expected result for me. I also tried to modify script to return doc['field'].value (or null when I want to exclude value from agg), but it not worked correctly too. Finally seems I've found workaround by using _source.field and null as returned values.

Anyway, descriptive failure message for "not value script" can save lot of time for elastic users.

@markharwood
Copy link
Contributor

cc @elastic/es-search-aggs
Some of the script examples in this old issue are outdated but master still accepts field and script params together when it should be one or the other.

@rjernst rjernst added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 4, 2020
@imotov
Copy link
Contributor

imotov commented Jun 25, 2020

In #31532 we decided not to deprecate "value scripts", which are still not very well documented. I am not getting cryptic failure messages anymore. The first example works as expected. We could add a deprecation warning on absence of _value in the script, but I am not sure that's the best approach here.

@imotov
Copy link
Contributor

imotov commented Jul 8, 2020

We discussed the issue and we would like to start sending warnings to users if _value is not present in the value script. It should be possible after #53702 is implemented and we can add a compiler pass to check if _value is present.

@wchaparro
Copy link
Contributor

Closing as not planned, focus on ESQL development. Painless has helped with this issue.

@wchaparro wchaparro closed this as not planned Won't fix, can't repro, duplicate, stale Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >enhancement help wanted adoptme Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo)
Projects
None yet
Development

No branches or pull requests

7 participants