Long range query support #3299

annashlyak · 2018-06-20T10:35:15Z

Hello, everybody,

I want to make range queries that correctly work with long type. So, I have added support of long range query.

Small test to demonstrate trouble that I want to solve .
We want to work with timestamps that are long type:

q.Range(c => c
	.Name("named_query")
	.Boost(1.1)
	.Field(p => p.Description)
	.GreaterThan(636634079999999999)
	.GreaterThanOrEquals(636634080000000000)
	.LessThan(636634080000000000)
	.LessThanOrEquals(636634079999999999)
	.Relation(RangeRelation.Within))

converts numbers to double type:

   query = new {
          range = new {
            description = new {
              _name = "named_query",
              boost = 1.1,
              gt = 6.3663408E+17,
              gte = 6.3663408E+17,
              lt = 6.3663408E+17,
              lte = 6.3663408E+17,
              relation = "within"
            }
          }
        }

and loses precision.
Elasticsearch himself is able to make such requests, but in NEST this functionality is missed.

I will be grateful if you merge my pull request and I'm ready to discuss my decision.

karmi · 2018-06-20T11:05:12Z

Hi @annashlyak, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

annashlyak · 2018-06-20T11:10:13Z

Hi @karmi, I have added email to my profile

Mpdreamz · 2018-06-22T12:56:46Z

Thanks for the PR @annashlyak 😄 this LGTM.

russcam

Thanks for the PR @annashlyak! I've left a couple of small comments, when you have a chance to take a look

russcam · 2018-08-20T05:56:21Z

src/Nest/QueryDsl/TermLevel/Range/RangeQueryJsonConverter.cs


+			var isNumeric = !jo.Properties().Any(p => p.Name == "format" || p.Name == "time_zone")


is it possible to refactor this to remove the duplicated code within isLong and isNumeric checks. It may be possible to reduce down to one enumeration/iteration of jo.Properties to determine IRangeQuery

russcam · 2018-08-20T05:56:47Z

src/Nest/QueryDsl/Visitor/DslPrettyPrintVisitor.cs

@@ -79,7 +79,9 @@ private void Write(string queryType, Field field = null)

 		public virtual void Visit(INumericRangeQuery query) => Write("numeric_range");

-        public virtual void Visit(ITermRangeQuery query) => Write("term_range");
+		public virtual void Visit(ILongRangeQuery query) => Write("numeric_range");


long_range?

russcam · 2018-08-22T01:11:22Z

would you like me to take another look at the PR @annashlyak - is it ready to review again?

annashlyak · 2018-08-22T05:33:15Z

@russcam yes, you can watch review again

russcam

Thanks @annashlyak, I've left a comment to simplify determining the range query.

Let me know what you think 🙂

russcam · 2018-08-23T05:38:19Z

src/Nest/QueryDsl/TermLevel/Range/RangeQueryJsonConverter.cs

+			var nameIsValid = !jo.Properties().Any(p => p.Name == "format" || p.Name == "time_zone");
+
+			var isLong = nameIsValid && CheckType(jo, JTokenType.Integer);
+			var isNumeric = nameIsValid && CheckType(jo, JTokenType.Float);


I think this can be simplified further.

For isNumeric and DateRangeQuery cases, CheckType will end up being called twice. It looks like it should be possible to enumerate/iterate over jo.Properties() once and determine from the existence of "format", "time_zone" or token type, which IRangeQuery it is. I was thinking something like

private static IRangeQuery GetRangeQuery(JsonSerializer serializer, JObject jo) { foreach (var property in jo.Properties()) { if (property.Name == "format" || property.Name == "time_zone") return FromJson.ReadAs<DateRangeQuery>(jo.CreateReader(), serializer); if (_rangeKeys.Contains(property.Name)) { if (property.Value.Type == JTokenType.Integer) return FromJson.ReadAs<LongRangeQuery>(jo.CreateReader(), serializer); if (property.Value.Type == JTokenType.Float) return FromJson.ReadAs<NumericRangeQuery>(jo.CreateReader(), serializer); return FromJson.ReadAs<DateRangeQuery>(jo.CreateReader(), serializer); } } }

what do you think?

Yes, thank you. LGTM. "format" and "time_zone" checks must be done for all properties in jo. When we check for a type, we can not immediately return the result, because LongRange is used, if there was no property of type float, but there was a property of type int. However, we can iterate once and I think that the code will looks better.

russcam

Left some additional comments @annashlyak. Let me know your thoughts

russcam · 2018-08-27T02:10:01Z

src/Nest/QueryDsl/TermLevel/Range/RangeQueryJsonConverter.cs

@@ -37,36 +37,30 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist

 		private static IRangeQuery GetRangeQuery(JsonSerializer serializer, JObject jo)
 		{
-			var nameIsValid = !jo.Properties().Any(p => p.Name == "format" || p.Name == "time_zone");
+			IRangeQuery fq = FromJson.ReadAs<DateRangeQuery>(jo.CreateReader(), serializer);


At this point, we don't know if the range query is one on date fields. I think assigning null is more appropriate

IRangeQuery fq = null;

and deserializing to DateRangeQuery only when it is known that the query is one on date fields.

Earlier we deserialize to DateRangeQuery always when check for numeric Type was false. I think that we can just not declare fq.

russcam · 2018-08-27T02:15:14Z

src/Nest/QueryDsl/TermLevel/Range/RangeQueryJsonConverter.cs

+				if (_rangeKeys.Contains(property.Name))
+				{
+					if (property.Value.Type == JTokenType.Float)
+						isNumeric = true;


If any one of the range properties is a float JSON type, then I think it's safe to return NumericRangeQuery query here.

The assumption that can't be made, if I've understood correctly, is that we can't rule out NumericRangeQuery if a range property is an integer JSON type until we've seen all the range properties i.e. put another way, we can't be sure to return LongRangeQuery until we've seen all of the range properties, and observed that they are all integer JSON types.

Earlier was check !jo.Properties().Any(p => p.Name == "format" || p.Name == "time_zone") It is the same as jo.Properties().All(p => p.Name != "format" && p.Name != "time_zone"). So, we have to done this for all properties and I dont want return here NumericRangeQuery

russcam · 2018-09-06T01:29:58Z

This looks great, @annashlyak! Thank you for your effort 👍

add LongRangeQuery to handle range queries on long numeric field types. (cherry picked from commit 69733ea)

annashlyak added 5 commits June 19, 2018 12:54

added LongRangeQuery

6dca245

moved LongRangeQuery in LongRangeQuery.cs and added tests

51d7589

added VisitQuery

2166072

test fix

1020933

RangeQueryJsonConverter fix: added support for long deserealization

a3e493b

returned method TermsSet

6686482

Mpdreamz approved these changes Jun 22, 2018

View reviewed changes

russcam suggested changes Aug 20, 2018

View reviewed changes

review fixes

24b1793

russcam suggested changes Aug 23, 2018

View reviewed changes

simplified GetRangeQuery

78d918b

russcam suggested changes Aug 27, 2018

View reviewed changes

Mpdreamz mentioned this pull request Sep 3, 2018

6.3.0 release #3386

Closed

45 tasks

fq initialization fix

958118c

annashlyak force-pushed the feature/long_range branch from 85821ac to 958118c Compare September 4, 2018 14:41

fix

b4fbbb3

russcam approved these changes Sep 6, 2018

View reviewed changes

russcam merged commit 69733ea into elastic:6.x Sep 6, 2018

russcam added Feature labels Sep 6, 2018

Mpdreamz pushed a commit that referenced this pull request Sep 10, 2018

Long range query support (#3299)

d02335d

add LongRangeQuery to handle range queries on long numeric field types. (cherry picked from commit 69733ea)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Long range query support #3299

Long range query support #3299

annashlyak commented Jun 20, 2018

karmi commented Jun 20, 2018

annashlyak commented Jun 20, 2018

Mpdreamz commented Jun 22, 2018

russcam left a comment

russcam Aug 20, 2018

russcam Aug 20, 2018

russcam commented Aug 22, 2018

annashlyak commented Aug 22, 2018

russcam left a comment •

edited

Loading

russcam Aug 23, 2018

annashlyak Aug 23, 2018

russcam left a comment •

edited

Loading

russcam Aug 27, 2018

annashlyak Sep 5, 2018

russcam Aug 27, 2018

annashlyak Sep 5, 2018

russcam commented Sep 6, 2018


		var isNumeric = !jo.Properties().Any(p => p.Name == "format" \|\| p.Name == "time_zone")

Long range query support #3299

Long range query support #3299

Conversation

annashlyak commented Jun 20, 2018

karmi commented Jun 20, 2018

annashlyak commented Jun 20, 2018

Mpdreamz commented Jun 22, 2018

russcam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russcam commented Aug 22, 2018

annashlyak commented Aug 22, 2018

russcam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russcam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russcam commented Sep 6, 2018

russcam left a comment •

edited

Loading

russcam left a comment •

edited

Loading