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

[Discover] Fix tiebreaker sort order in context view #32225

Merged
merged 1 commit into from
Mar 5, 2019
Merged

[Discover] Fix tiebreaker sort order in context view #32225

merged 1 commit into from
Mar 5, 2019

Conversation

wylieconlon
Copy link
Contributor

@wylieconlon wylieconlon commented Feb 28, 2019 •
edited
Loading

Summary

The sort order for context view with tiebreakers was wrong, as reported in #12937 and #18647. For example, with the context:tieBreakerFields set to offset,_doc as described in those issues, you would get a context view with time ordering in reverse, as seen in these screenshots.

The approach taken in this PR is to use the same sort order as the time field for tiebreakers, which is correct for numeric and date fields. I'm not aware of any cases where this order is wrong, but it if reviewers are able to think of situations where this approach is not generic enough, we can add a setting to Advanced Settings to choose the sort order.

Before:

screenshot 2019-02-28 13 09 01

After:

screenshot 2019-02-28 13 05 28

Closes #12937 and #18647.

Checklist

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

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

For maintainers

@wylieconlon wylieconlon added Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Feb 28, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@wylieconlon wylieconlon reopened this Feb 28, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

chrisdavies
chrisdavies approved these changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@chrisdavies chrisdavies left a comment

Choose a reason for hiding this comment

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

LGTM. I ddin't pull down and test, but reviewed, and also watched a demo of it. The main concern is whether or not the new sort order is problematic in some edge-case (such as alphabetic sorting). But ultimately, this is probably a reasonable short-put, and we need a longer-term plan to allow users to do multi-column sorts in an ad-hoc way.

@wylieconlon wylieconlon changed the title In context view, set tiebreaker sort to the same as timestamp sort [Discover] Fix tiebreaker sort order in context view Mar 4, 2019
@weltenwort
Copy link
Member

I don't see how this would cause any additional edge cases, since it just changes the sort direction. Whether or not the direction is made configurable sounds like an independent consideration.

@wylieconlon wylieconlon merged commit 116f86e into elastic:master Mar 5, 2019
@wylieconlon wylieconlon deleted the use-time-sort-for-tiebreaker branch March 5, 2019 18:27
@wylieconlon
Copy link
Contributor Author

@chrisdavies Think I need to do any backporting?

@xaon2017
Copy link

@wylieconlon Why is this problem still exist in kibana7.1.1?

@timroes timroes added the v8.0.0 label Aug 28, 2019
@timroes
Copy link
Contributor

timroes commented Aug 28, 2019

@wylieconlon Could you perhaps still backport this to 7.x, it seems you unfortunately didn't get any answer that time.

@wylieconlon
Copy link
Contributor Author

I thought this was already merged to 7.0.0 but it looks like I missed the branch-off date by a day. There are some conflicts in the backporting so I will need to take a closer look at the recent changes by @kertal

@kertal
Copy link
Member

kertal commented Aug 29, 2019

@wylieconlon happy to help if you have any questions, there's also this unmarked PR taking care of the tiebreaker field in discover #32426. However in the meantime, there have been changes in terms of sorting (multi sorting is available) #42551

@kertal
Copy link
Member

kertal commented Aug 29, 2019

do not have enough time today to investigate, but I think tiebreaker in context was implemented also in 7.x

https://github.com/elastic/kibana/blob/7.x/src/legacy/core_plugins/kibana/public/context/api/context.ts#L60

what I did back in the days is merge 2 similar functions to fetchSurroundingDocs

@kertal
Copy link
Member

kertal commented Aug 29, 2019

since I did the changes in master, and merged it back, I think we're good here

@timroes timroes added the release_note:skip Skip the PR/issue when compiling release notes label Apr 26, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 32225 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 28, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 32225 or prevent reminders by adding the backport:skip label.

5 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 32225 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 32225 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 32225 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 32225 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 32225 or prevent reminders by adding the backport:skip label.

@kertal
Copy link
Member

kertal commented May 6, 2021

should be safe to close it, but I will check to be sure

@timroes timroes added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisdavies chrisdavies chrisdavies approved these changes

@weltenwort weltenwort Awaiting requested review from weltenwort

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

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application release_note:fix release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

customizeable sort direction of context:tieBreakerFields
8 participants
@wylieconlon @elasticmachine @weltenwort @xaon2017 @timroes @kertal @kibanamachine @chrisdavies