Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Lens] Filters aggregation #75635
[Lens] Filters aggregation #75635
Changes from 34 commits
e82e536
47ba9df
38fb236
be5ae8d
8d1227d
66aada5
030b5f7
f21b11f
9f3f4a8
0c85af1
07ce55a
6fbac67
f97e5f6
ccd9ed7
becb89a
c659183
9537d67
2503ba0
c3f3ded
9f2bd40
e1fe770
fdbf1ae
acbffdb
5c4c1d5
6e0b3f3
c0d36e6
a931828
98da724
2f6339d
03c00d8
3216553
c3de990
127e539
81c4080
3a29c0f
b539a0f
c18c658
871ec5b
5cf7490
e95af12
eebbbdb
e0058e2
2978769
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This phrasing has bothered me for a long time, and if you feel like it's related feel free to make a change here. It's probably a separate PR though. #76038
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a PR for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this whole grouping section's copy is not just confusing to read by the grammar is not great and I know that's because of the re-use of the field name, but I think we need phrasing or formatting that considers this better. Also the option for this type of agg shows as "Top values of each Records". I think "Records" here is incorrect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick option to do this in a clean way is to have a property on the operation definition
acceptsColumn(otherColumn: IndexPatternColumn): boolean
which is called instead of putting that logic here. The specific implementation would be part of the filters operation definition.I'm fine with not doing that now, in that case let's create a technical debt issue for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is that other columns are already accepted if the field is compatible with them (the fact they are not accepted was the misconception I got from checking the previousColumn type we talked about async). In our case, top values and filters have totally non-compatible fields set. I think this has to be completely changed anyway once we allow creating fieldless columns. I'll create an issue, but without proposal for implementation, because it depends on fieldless column. Feel free to add some details if you have any ideas.
#77017