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.

Sign up for GitHub

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

Jump to bottom

[Vis Editor] Fix fields filtering in top hit agg #56367

Merged
merged 8 commits into from
Feb 5, 2020
Merged

[Vis Editor] Fix fields filtering in top hit agg #56367

merged 8 commits into from
Feb 5, 2020

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Jan 30, 2020 •
edited
Loading

Summary

Is a part of #54460 . Fix is only acceptable for master & 7.x.
Fix field types filtering in Sort on param inside the Top Hit aggregation.

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@sulemanof
Fix top hit agg
b3d3079
@sulemanof
Merge remote-tracking branch 'upstream/master' into fix/top_hit_agg
436b3e7 
# Conflicts:
#	src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts
#	src/legacy/ui/public/agg_types/metrics/top_hit.ts
#	src/legacy/ui/public/agg_types/param_types/field.ts
@sulemanof
Add unit tests
81ce24a
@sulemanof sulemanof marked this pull request as ready for review January 30, 2020 12:52
flash1293
flash1293 approved these changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, tested and works. @maryia-lapata please have a second look, I'm not super familiar with that area of the code :)

@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

@maryia-lapata
Copy link
Contributor

@sulemanof
I created a Table visualization and faced errors:

In some case it happens that field is undefined. Could you please investigate it?

@timroes timroes added Feature:Vis Editor Visualization editor issues Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Feb 3, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@sulemanof
Copy link
Contributor Author

Hey @maryia-lapata
Thanks for a good catch!
I've fixed it. The problem starts in src/legacy/core_plugins/data/public/search/expressions/esaggs.ts
while creating the AggConfigs without schemas...
image
In that case an unknown schema is used by default.
I've added default configs to the unknown schema, but I hope after refactoring of the AggConfigs this will gone.
@ppisljar could you please confirm these changes?

@sulemanof sulemanof requested a review from ppisljar February 3, 2020 14:52
@lukeelmers
Copy link
Member

I've added default configs to the unknown schema, but I hope after refactoring of the AggConfigs this will gone.

This makes sense to me, but I'd like @ppisljar to confirm too.

Also wanted to note there are a few places in this PR which will conflict with #56353, which I expect to merge any time now (waiting on one last LGTM).

@sulemanof
Merge branch 'master' into fix/top_hit_agg
5101cbf 
# Conflicts:
#	src/legacy/core_plugins/data/public/search/aggs/metrics/metric_agg_type.ts
#	src/legacy/core_plugins/data/public/search/aggs/param_types/field.test.ts
#	src/legacy/core_plugins/data/public/search/aggs/param_types/field.ts
#	src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts
#	src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts
@sulemanof
Resolve merge conflicts
a1d0ae3
@sulemanof sulemanof requested a review from a team as a code owner February 5, 2020 13:26
ppisljar
ppisljar approved these changes Feb 5, 2020
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@sulemanof
Merge remote-tracking branch 'upstream/master' into fix/top_hit_agg
0476a0a 
# Conflicts:
#	src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@sulemanof sulemanof merged commit 8cca8b9 into elastic:master Feb 5, 2020
@sulemanof sulemanof deleted the fix/top_hit_agg branch February 5, 2020 16:20
@sulemanof sulemanof mentioned this pull request Feb 5, 2020
Merged
sulemanof added a commit that referenced this pull request Feb 6, 2020
@sulemanof @elasticmachine
[Vis Editor] Fix fields filtering in top hit agg (#56367) (#56871)
c9ede92 
* Fix top hit agg

* Add unit tests

* Fix fields filtering in table vis

* Resolve merge conflicts

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@flash1293 flash1293 mentioned this pull request Feb 27, 2020
Closed
@sulemanof sulemanof mentioned this pull request Mar 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppisljar ppisljar ppisljar approved these changes

@flash1293 flash1293 flash1293 approved these changes

@maryia-lapata maryia-lapata Awaiting requested review from maryia-lapata

Assignees
No one assigned
Labels
backported Feature:Vis Editor Visualization editor issues release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sulemanof @maryia-lapata @elasticmachine @lukeelmers @kibanamachine @flash1293 @ppisljar @timroes