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

Split Filters aggregation #936

Closed
ewgRa opened this issue Sep 16, 2015 · 6 comments
Closed

Split Filters aggregation #936

ewgRa opened this issue Sep 16, 2015 · 6 comments

Comments

@ewgRa
Copy link
Contributor

ewgRa commented Sep 16, 2015

In elasticsearch we can't mix anonymous and named filters in one query

Such queries is not possible:

      "filters" : {
        "filters" : {
          0 :   { "term" : { "body" : "error"   }},
          "warnings" : { "term" : { "body" : "warning" }}
        }
      },

Answer buckets always will be or all anonymous, or all named.

My suggestion is to split Filters on NamedFilters and AnonymousFilters and deprecate Filters.

Now code allow add named and anonymous filters simultaneously, that give unexpected results.

Any comments?

@im-denisenko
Copy link
Contributor

@ewgRa If array key will be quoted will it be a valid filter name?

@ruflin
Copy link
Owner

ruflin commented Sep 17, 2015

@ewgRa In case elasticsearch does not allow the mix, I suggest we also prevent it in Elastica and give the user an exception.

@ewgRa
Copy link
Contributor Author

ewgRa commented Sep 17, 2015

@im-denisenko

Yes, it is valid:

      "filters" : {
        "filters" : {
          "0" :   { "term" : { "body" : "error"   }},
          "warnings" : { "term" : { "body" : "warning" }}
        }
      },

In buckets there will be "0" key. I already add this case to tests in #935.
If you use arrays with keys - you must use string as key. If this is not an associative array - it will be anonymous buckets.

Just fyi: "e'räro"rs" also valid, becomes in results "e'räro"rs"

@ewgRa
Copy link
Contributor Author

ewgRa commented Sep 17, 2015

@ruflin but in exists class? you don't want to split it to be close to Elasticsearch?

@ruflin
Copy link
Owner

ruflin commented Sep 17, 2015

@ewgRa No I would not split the class, but throw an error before elasticsearch has to do it. This is more convenience for the person that implements it. Instead of getting a response exception and having to figure out what is going on, it is already thrown before the request is sent and is very specific.

@ewgRa
Copy link
Contributor Author

ewgRa commented Sep 17, 2015

PR for prevent mixing keys - #939

@ewgRa ewgRa closed this as completed Sep 17, 2015
ruflin added a commit that referenced this issue Sep 18, 2015
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

No branches or pull requests

3 participants