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

Updated Date Range to Follow Documentation When Assuming Missing Values #112258

Merged
merged 35 commits into from
Oct 1, 2024

Conversation

john-wagster
Copy link
Contributor

Based on a review of the documentation here: https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-range-query.html. Our behavior at query and index time for date range queries was inconsistent.

This would lead to what was discovered here: #111484 where an indexed date_range field like the one shown below would not be returned by the exact same query shown below.

I attempted to fix this by changing the query parser to use DateMathParser which rounds appropriately to what's documented above. This changes missing information in date time values to be inline with the documentation often by 1 millisecond (such as the time information in the below example).

A few concerns worth discussing with this are where this should land in terms of releases and impacts to customer who may have assumptions about how date range has been working up to this point. Opening this as a discussion with some caution around putting it in until we get appropriate feedback.

query example
PUT /test_index1?pretty
{
    "mappings" : {
        "properties" : {
            "opening_dates": {
              "type": "date_range",
              "format": "yyyy-MM-dd"
            }
        }
    }
}

PUT test_index1/_doc/7
{
    "opening_dates": [
    {
      "gte": "2014-09-18",
      "lte": "2016-12-18"
    }
  ]
}

GET test_index1/_search
{
  "explain": true, 
  "query": {
    "bool": {
      "must": [
        {
          "range": {
            "opening_dates": {
              "gte": "2014-09-18",
              "lte": "2016-12-18",
              "relation": "contains",
              "format": "yyyy-MM-dd"
            }
          }
        }
      ]
    }
  }
}

@elasticsearchmachine elasticsearchmachine added the Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch label Aug 27, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-relevance (Team:Search Relevance)

@elasticsearchmachine
Copy link
Collaborator

Hi @john-wagster, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

Hi @john-wagster, I've updated the changelog YAML for you.

@benwtrent benwtrent self-requested a review September 3, 2024 13:10
@benwtrent
Copy link
Member

//cc @elastic/es-core-infra

This does seem like the way it should have always been for range values :/ wanting to make sure we aren't doing something crazy.

@benwtrent benwtrent added :Search Foundations/Mapping Index mappings, including merging and defining field types and removed :Search Relevance/Search Catch all for Search Relevance labels Sep 3, 2024
@elasticsearchmachine elasticsearchmachine added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch and removed Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch labels Sep 3, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@elasticsearchmachine
Copy link
Collaborator

Hi @john-wagster, I've updated the changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

Hi @john-wagster, I've updated the changelog YAML for you.

@john-wagster john-wagster requested a review from a team as a code owner September 27, 2024 20:02
@john-wagster
Copy link
Contributor Author

@elasticsearchmachine test this please

@@ -46,7 +46,8 @@ public Set<NodeFeature> getFeatures() {
IndexSettings.IGNORE_ABOVE_INDEX_LEVEL_SETTING,
SourceFieldMapper.SYNTHETIC_SOURCE_COPY_TO_INSIDE_OBJECTS_FIX,
TimeSeriesRoutingHashFieldMapper.TS_ROUTING_HASH_FIELD_PARSES_BYTES_REF,
FlattenedFieldMapper.IGNORE_ABOVE_WITH_ARRAYS_SUPPORT
FlattenedFieldMapper.IGNORE_ABOVE_WITH_ARRAYS_SUPPORT,
RangeFieldMapper.DATE_RANGE_INDEXING_FIX
Copy link
Member

Choose a reason for hiding this comment

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

This can be a test feature I think...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't sure about when to use test featuers or not; thanks for the suggestion. Let me know if that's not what you were thinking.

@elasticsearchmachine
Copy link
Collaborator

Hi @john-wagster, I've updated the changelog YAML for you.

@thecoop thecoop self-requested a review October 1, 2024 13:55
@john-wagster john-wagster merged commit 0fbb3bc into elastic:main Oct 1, 2024
16 checks passed
elasticsearchmachine pushed a commit that referenced this pull request Oct 3, 2024
matthewabbott pushed a commit to matthewabbott/elasticsearch that referenced this pull request Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.15.3 v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants