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

RangeQuery gt, gte, lt, lte with backward compatibility #1453

Closed

Conversation

vdamery
Copy link

@vdamery vdamery commented Dec 24, 2020

I'm migrating an application from Scala (Elastic4s https://github.com/sksamuel/elastic4s) to Golang (Olivere) and I've experienced the same behaviour as PR #1187

elastic.NewRangeQuery(DatePublication).Gt(xxx)

Expected : Scala, rest api, elatic documentation

{
  "range": {
    "datePublication": {
      "gt": "xxx"
    }
  }
}

VS
Got : Golang olivere

{
  "range": {
    "datePublication": {
      "from": "xxx",
      "include_lower": false,
      "include_upper": true,
      "to": null
    }
  }
}

See https://www.elastic.co/guide/en/elasticsearch/reference/0.90/query-dsl-range-query.html

Deprecated in 0.90.4.
The from, to, include_lower and include_upper parameters have been deprecated in favour of gt,gte,lt,lte

I have change in RangeQuery from, to, include_lower and include_upper parameters into gt, gte, lt, lte but I have kept From, To, IncludeLower and IncludeUpper for backward compatibility.

I don't know if you want some verification like theses :

  1. at least one of gt, gte, lt and lte
  2. not gt + gte or lt + lte

🎉 Merry Christmas 🎅

@hastinc
Copy link

hastinc commented May 10, 2021

Hi @olivere how are you doing? I'm just wondering what's the current status of this PR as I believe this solves an issue I'm currently experiencing with gte lte range queries using date math.

Currently, when using elastic.NewRangeQuery the following query is made to ES

{
   "query":{
      "range":{
         "created_date":{
            "from":"now-1m/m",
            "include_lower":true,
            "include_upper":true,
            "to":"now/m"
         }
      }
   }
}

While the documentation suggests

{
   "query":{
      "range":{
         "created_date":{
            "gte":"now-1m/m",
            "lt":"now/m"
         }
      }
   }
}

Many thanks,
Cathal

@olivere
Copy link
Owner

olivere commented May 10, 2021

@hastinc I treated this PR as a "nice-to-have" mainly because the on-the-wire format still uses the old syntax even in the current version of ES. In other words: While the JSON in the request might be different (and nicer), the results that you should see in the response should be the same.

If there is a reproducible bug with the current on-the-wire format, I would be happy to fix it. Feel free to file an issue.

@hastinc
Copy link

hastinc commented May 10, 2021

@hastinc I treated this PR as a "nice-to-have" mainly because the on-the-wire format still uses the old syntax even in the current version of ES. In other words: While the JSON in the request might be different (and nicer), the results that you should see in the response should be the same.

If there is a reproducible bug with the current on-the-wire format, I would be happy to fix it. Feel free to file an issue.

@olivere thanks for following up and the clear and concise explanation.

@RyouZhang
Copy link

so it's fixed now? I find use gt/lt should more faster than from/to

@RyouZhang
Copy link

it is two years ago....😓

@mitar
Copy link

mitar commented Sep 16, 2022

So if I understand correctly, Elastic is using non-documented fields which still happen to be mapped correctly to underlying wire format because they are used in the wire format? Is there any reason we want to keep this non-documented behavior? Backwards compatibility with a very old ES version (0.90)?

@vdamery
Copy link
Author

vdamery commented Apr 5, 2024

I'm closing my PR since there hasn't been activity in a long time and :

Deprecated: Use the official Elasticsearch client for Go at https://github.com/elastic/go-elasticsearch

@vdamery vdamery closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants