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

Include and Exclude params on terms agg #1660

Merged
merged 30 commits into from
Oct 20, 2014
Merged

Conversation

w33ble
Copy link
Contributor

@w33ble w33ble commented Oct 13, 2014

Closes #1616 - precursor to #1527

Adds exclude and include params to the terms agg builder

screenshot 2014-10-14 15 04 21

  • Add regex agg type
    • Add regular expression editor
    • Add RegexAggParam class
  • Change how AggParam classes are loaded (based on type)
    • Updated tests
  • Added label filter with tests

The label filter was added so I could use the aggParam name as the field label - that filter may be useful elsewhere, and could probably use some extending.

@w33ble w33ble changed the title Exclude terms Include and Exclude params on terms agg Oct 13, 2014
@w33ble w33ble added review and removed review labels Oct 13, 2014
@spalger spalger added the WIP label Oct 14, 2014
@@ -36,9 +37,12 @@ define(function (require) {
if (param.name === 'field') {
return new FieldAggParam(param);
}
else if (param.options) {
else if (param.type === 'optioned') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of specifying the 'optioned' param here, primarily because of the hard link it creates between the param "DSL" and the implementation. For the most part, I would like the param classes to be hidden from the params themselves... does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, but I needed a way to specify the new regex param type, and since it's called a "param type", it made sense to use type: regex - and to keep it consistent, I changed the other existing type as well. I'm open to other ideas though.

@w33ble
Copy link
Contributor Author

w33ble commented Oct 14, 2014

I've noticed an interesting anomaly, but I think it's just an ES thing...

screenshot 2014-10-14 14 10 28

Here, the purple bar has no value, no label in the legend or the tooltip and is just a count. I believe what happens is that there are no results when excluding ^2.* (as defined) so it returns a count of all the documents. Is this expected?

@w33ble w33ble removed the WIP label Oct 15, 2014
@w33ble
Copy link
Contributor Author

w33ble commented Oct 15, 2014

Ready for review, unless @spenceralger is really against the whole param type thing...

@spalger
Copy link
Contributor

spalger commented Oct 15, 2014

I say it's a bug. If the two regexp instead created buckets with a single document the graph would like much different.

@spalger
Copy link
Contributor

spalger commented Oct 15, 2014

Also, I'm fine with the type thing. Can't think of a better way to do it.

@rashidkpc
Copy link
Contributor

I personally don't like the array of checkboxes, its messy. Maybe a select box that adds them to a list on select? I also think the whole thing should be hidden under an "advanced" section

@rashidkpc
Copy link
Contributor

Select2's tagging mode is a good example: https://ivaynberg.github.io/select2/#tags

@w33ble
Copy link
Contributor Author

w33ble commented Oct 15, 2014

I also think the whole thing should be hidden under an "advanced" section

That's coming in the next iteration, with the rest of the advanced stuff that's been mentioned before

Select2's tagging mode is a good example

Indeed it is! Going to implement that.

@w33ble
Copy link
Contributor Author

w33ble commented Oct 16, 2014

Now with more chosen!

screenshot 2014-10-15 17 47 55

Still some styles to tweak, but I'd like to get the merged since it's the baseline for #1527. Any other reasons to hold this up?

@w33ble
Copy link
Contributor Author

w33ble commented Oct 16, 2014

Added the advanced panel and fixed some style issues on the chosen stuff. @spenceralger @rashidkpc any reason I shouldn't merge this?

@rashidkpc
Copy link
Contributor

will review today

@spalger
Copy link
Contributor

spalger commented Oct 20, 2014

LGTM, need to merge with master though

fixed by my PR accepted on bootstrap-chosen
only loop through params once
more understandable separation of basic and advanced
w33ble added a commit that referenced this pull request Oct 20, 2014
Include and Exclude params on terms agg
@w33ble w33ble merged commit 7560531 into elastic:master Oct 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exclude terms in terms agg
3 participants