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

Add numeric_type option for correct sort order on mixed date and date_nanos fields #44212

Merged
merged 17 commits into from
Sep 20, 2019
Merged

Add numeric_type option for correct sort order on mixed date and date_nanos fields #44212

merged 17 commits into from
Sep 20, 2019

Conversation

kertal
Copy link
Member

@kertal kertal commented Aug 28, 2019 •
edited
Loading

Summary

When you are searching multiple indices and they are using a mix of date and date_nanos, the sort order is not correct. The reason is that the internal representation is a long since the start of the epoche for both, but because of the different precision this doesn't sort correctly. Elasticsearch has added a workaround for that in 7.2 with an option "numeric_type": "date_nanos" to cast everything to nanosecond precision. This PR adds this option for searches within Discover.

Steps to reproduce:

  1. Mapping
PUT timestamp-millis
{
  "mappings": {
    "properties": {
      "timestamp": {
        "type": "date"
      }
    }
  }
}

PUT timestamp-nanos
{
  "mappings": {
    "properties": {
      "timestamp": {
        "type": "date_nanos"
      }
    }
  }
}
  1. Sample documents:
PUT timestamp-millis/_doc/1
{
  "timestamp": "2019-01-01T12:10:30.123Z"
}

PUT timestamp-millis/_doc/2
{
  "timestamp": "2019-01-01T12:10:30.124Z"
}

PUT timestamp-nanos/_doc/3
{
  "timestamp": "2019-01-01T12:10:30.123456789Z"
}

PUT timestamp-nanos/_doc/4
{
  "timestamp": "2019-01-01T12:10:30.123498765Z"
}
  1. Sort in Discover (format in the index pattern is switched to Date Nanos):

discover-nanos

Fixes #43939

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

- [ ] This was checked for breaking API changes and was labeled appropriately

@kertal kertal self-assigned this Aug 28, 2019
@kertal kertal changed the title Add numeric_type option to sort containing date_nanos for sort order on mixed date and date_nanos fields Add numeric_type option for correct sort order on mixed date and date_nanos fields Aug 28, 2019
@kertal kertal added the WIP Work in progress label Aug 28, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kertal kertal removed the WIP Work in progress label Sep 17, 2019
TinaHeiligers
TinaHeiligers approved these changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The code looks and testing this in Chrome (I had to change my timezone to Europe/Athens from America/Phoenix), the fix works as expected.
@kertal is there an issue for the timezone bug with 'America/Phoenix' registering date nano timestamps as before epoch time?

Bargs
Bargs suggested changes Sep 17, 2019
View reviewed changes
src/legacy/core_plugins/kibana/public/discover/controllers/discover.js Outdated
order: sortPair[timeFieldName],
numeric_type: 'date_nanos'
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move this code into the getSort function itself? getSort is used in the search panel in Dashboard as well so I suspect putting this in the Discover controller means the sorting still won't work in Dashboard.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Bargs, yes this needs to be adapted , I already did . I initially built this in the getSort function, but this broke functionality like changing the sort order in the doc table. So I thought, better add this change before setting the sort of searchSource, since it's only relevant for Elasticsearch. Now I've extracted this to a separate function getSortForSearchSource, adapted also for dashboard. works

Copy link
Member Author

Choose a reason for hiding this comment

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

dear @Bargs, is that change ok for you? merci!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I could have sworn I left a comment on this right after you made the changes. Must have forgot to hit submit. Yeah, LGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

great, thx!

test/functional/apps/discover/_date_nanos_mixed.js Show resolved Hide resolved
@kertal
Copy link
Member Author

kertal commented Sep 18, 2019

The code looks and testing this in Chrome (I had to change my timezone to Europe/Athens from America/Phoenix), the fix works as expected.
@kertal is there an issue for the timezone bug with 'America/Phoenix' registering date nano timestamps as before epoch time?

👍 Tina, yes, there's an ElasticSearch issue, I'll ask when aggregation with date_nanos will work worldwide

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kertal
Copy link
Member Author

kertal commented Sep 18, 2019

@TinaHeiligers here's the ES issue elastic/elasticsearch#39107

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kertal kertal merged commit 7ef4138 into elastic:master Sep 20, 2019
kertal added a commit to kertal/kibana that referenced this pull request Sep 20, 2019
@kertal
Add numeric_type option for correct sort order on mixed date and date…
f4ee97a 
…_nanos fields (elastic#44212)

* Implement getSortForSearchSource for add-on of 'numeric_type' to the ES request. Then sorting on a field that can be of date or date_nanos type works correctly

* Add functional test
@kertal kertal mentioned this pull request Sep 20, 2019
Merged
kertal added a commit that referenced this pull request Sep 20, 2019
@kertal
Add numeric_type option for correct sort order on mixed date and date…
dbda7f4 
…_nanos fields (#44212) (#46212)

* Implement getSortForSearchSource for add-on of 'numeric_type' to the ES request. Then sorting on a field that can be of date or date_nanos type works correctly

* Add functional test
@kertal kertal deleted the kertal-pr-fix-sort-on-mixed-date-and-date_nanos branch August 25, 2020 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TinaHeiligers TinaHeiligers TinaHeiligers approved these changes

@Bargs Bargs Bargs approved these changes

Assignees

@kertal kertal

Labels
Feature:Discover Discover Application release_note:fix v7.5.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix sort order on mixed date and date_nanos fields
4 participants
@kertal @elasticmachine @Bargs @TinaHeiligers