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

Advanced aggs #1732

Merged
merged 14 commits into from
Oct 28, 2014
Merged

Advanced aggs #1732

merged 14 commits into from
Oct 28, 2014

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented Oct 22, 2014

Add script input for terms and ranges, add filters to significant terms.

Closes all but 1 potential input in #1527

@rashidkpc
Copy link
Contributor

There's a pull on your repo that tweaks this a bit, but otherwise it looks good. The only thing I'd like to see would be a link to some reference on elasticsearch scripting. Unfortunately we don't have great error handling for bad scripts but there's not much we can do about it.

@rashidkpc
Copy link
Contributor

@w33ble Found one more interesting issue here. If the user specifies a script, and the result of the script would be a different type than the selected field, elasticsearch will throw a ClassCastException. Eg something like this:

GET /_search
{
  "size": 0,
  "aggs": {
    "agg_10": {
      "terms": {
        "field": "bytes",
        "size": 5,
        "order": {
          "_count": "desc"
        },
        "script": "doc[\"bytes\"].value > 1 ? 'foo' : 'bar'"
      }
    }
  }
}

Here we're aggregating on bytes, an integer, but returning a string. However, we don't actually need to specify the field here. I'm thinking if the user specifies a script we should just not send the field param, since that seems to solve the problem.

@w33ble
Copy link
Contributor Author

w33ble commented Oct 27, 2014

@rashidkpc

I'm thinking if the user specifies a script we should just not send the field param, since that seems to solve the problem.

That would solve the script problem of mixing field and script, but if you're trying to use this as a value script input, you still need to specify the field (if I understand the ES docs correctly), so just ignoring the field if script is set isn't the answer here.

Early on I saw this conflict and decided to treat this as a value script input instead of a value input for that reason. As it stands, since field is required, using script gets messy. Unfortunately, I don't know what the answer to that should be...

@w33ble
Copy link
Contributor Author

w33ble commented Oct 27, 2014

The plan is to remove script for now and and shoehorn it into the field value. See #1755.

This PR will just add include/exclude to significant terms in addition to the advanced field toggle.

@rashidkpc
Copy link
Contributor

Once scripts are removed, I'm +1 on this, merge it.

w33ble added a commit that referenced this pull request Oct 28, 2014
@w33ble w33ble merged commit c9078de into elastic:master Oct 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants