Skip to content

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

Closed
vdamery wants to merge 2 commits into
olivere:release-branch.v6from
vdamery:range-query-gt-gte-lt-lte
Closed

RangeQuery gt, gte, lt, lte with backward compatibility#1453
vdamery wants to merge 2 commits into
olivere:release-branch.v6from
vdamery:range-query-gt-gte-lt-lte

Conversation

@vdamery

@vdamery vdamery commented Dec 24, 2020

Copy link
Copy Markdown

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

hastinc commented May 10, 2021

Copy link
Copy Markdown

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

olivere commented May 10, 2021

Copy link
Copy Markdown
Owner

@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

hastinc commented May 10, 2021

Copy link
Copy Markdown

@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
Copy Markdown

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

@RyouZhang

Copy link
Copy Markdown

it is two years ago....😓

@mitar

mitar commented Sep 16, 2022

Copy link
Copy Markdown

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

vdamery commented Apr 5, 2024

Copy link
Copy Markdown
Author

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