Support Date range filter #12

itye-msft · 2020-02-26T15:56:40Z

Added support for date range filter

…eQuery

balteravishay · 2020-02-27T07:37:37Z

K2Bridge/Visitors/RangeClauseVisitor.cs

@@ -29,8 +30,19 @@ public void Visit(RangeClause rangeClause)
            {
                // general "is between" filter on numeric fields uses a rangeClause query with GTE+LT (not LTE like above)
                EnsureClause.IsNotNull(rangeClause.LTValue, nameof(rangeClause.LTValue));
-
-                rangeClause.KustoQL = $"{rangeClause.FieldName} >= {rangeClause.GTEValue} and {rangeClause.FieldName} < {rangeClause.LTValue}";
+                var t = ClauseFieldTypeProcessor.GetType(schemaRetriever, rangeClause.FieldName).Result;


shouldn't this be awaited?

Yes it should. How about I open a new issue to change this as a new user story?

balteravishay · 2020-02-27T07:38:37Z

K2Bridge/Visitors/ClauseFieldTypeProcessor.cs

+        [System.Diagnostics.CodeAnalysis.SuppressMessage("Globalization", "CA1303:Do not pass literals as localized parameters", Justification = "Resources are not supported yet.")]
+        public static async Task<ClauseFieldType> GetType(ISchemaRetriever schemaRetriever, string fieldName)
+        {
+            if (schemaRetriever == null)


ensure can be used?

K2Bridge.Tests.UnitTests/K2Bridge.Tests.UnitTests.csproj

tamirkamara · 2020-02-27T09:20:13Z

K2Bridge/Visitors/RangeClauseVisitor.cs

+                    case ClauseFieldType.Text:
+                        rangeClause.KustoQL = $"{rangeClause.FieldName} >= {rangeClause.GTEValue} and {rangeClause.FieldName} < {rangeClause.LTValue}";
+                        break;


Range clause can't work with text types in ADX. this case should throw an error.

Something is wrong in the tests since a query like: field >= aaa and field < bbb should fail since you're missing quotes

eladiw · 2020-03-04T07:19:13Z

K2Bridge.Tests.UnitTests/Visitors/LuceneNet/LucenePhraseVisitorTests.cs

@@ -76,7 +78,7 @@ public string Visit_WithValidTermQuery_ReturnsValidReponse()
            var es = phraseQuery.ESQuery;
            Assert.NotNull(es);

-            var visitor = new ElasticSearchDSLVisitor(SchemaRetrieverMock.CreateMockSchemaRetriever());
+            var visitor = VisitorTestsUtils.CreateRootVisitor();


A) here you are using the utils class

eladiw · 2020-03-04T07:20:04Z

K2Bridge.Tests.UnitTests/Visitors/LuceneNet/LuceneRangeVisitorTests.cs

@@ -57,7 +59,8 @@ public string Visit_WithValidRangeQuery_ReturnsValidResponse()
            var es = rangeQuery.ESQuery;
            Assert.NotNull(es);

-            var visitor = new ElasticSearchDSLVisitor(SchemaRetrieverMock.CreateMockSchemaRetriever());
+            var visitor = new ElasticSearchDSLVisitor(SchemaRetrieverMock.CreateMockSchemaRetriever("days", "long"));


b) Here you are using the new ESDSLvisitor statement.

eladiw · 2020-03-04T07:23:43Z

K2Bridge.Tests.UnitTests/Visitors/VisitorTestsUtils.cs

+        /// Have the ISchemaRetriever initialised by the default visitor.
+        /// </summary>
+        /// <param name="visitor"></param>
+        internal static void AddRootDsl(ElasticSearchDSLVisitor visitor)


Name is not so clear.
e.g.
WrapWithRootDslAndAccept (maybe too long)
AddRootAndAccept?
AcceptWithRoot?

but AddRootDsl which gets a visitor. seems like the visitor is the one being added to (or added into) with the root....

eladiw · 2020-03-04T07:30:41Z

K2Bridge.Tests.UnitTests/Visitors/VisitorTestsUtils.cs

+                },
+                IndexName = "someindex",
+            };
+            visitor.Visit(dsl);


also, the method says: AddRootDsl
but at the end you call visit... so its much more that just add root. it actually visits.

also, I think you shouldnt use visitor.Visit but dsl.Accept(visitor)

I renamed the method, however, I kept Visit since I am not changing existing tests code.

eladiw · 2020-03-04T07:31:27Z

K2Bridge.Tests.UnitTests/Visitors/VisitorTestsUtils.cs

+        {
+            var visitor = new ElasticSearchDSLVisitor(SchemaRetrieverMock.CreateMockSchemaRetriever());
+            AddRootDsl(visitor);
+            return visitor;


here you have an issue. you 'AddRootDsl' which already visits and then you return a visitor. needs some refactoring to be clearer

eladiw · 2020-03-04T07:40:16Z

K2Bridge/Visitors/QueryStringClauseVisitor.cs

-                _ => false,
-            };
+            Ensure.IsNotNullOrEmpty(fieldName, nameof(fieldName));
+            var t = await ClauseFieldTypeProcessor.GetType(schemaRetriever, fieldName);


Suggested change

var t = await ClauseFieldTypeProcessor.GetType(schemaRetriever, fieldName);

var fieldType = await ClauseFieldTypeProcessor.GetType(schemaRetriever, fieldName);

eladiw

this is the right direction, however I think there is some mixup in the addRootDsl method

eladiw · 2020-03-05T19:02:16Z

K2Bridge.Tests.UnitTests/Visitors/ParseElasticToKqlTests.cs

@@ -379,15 +380,15 @@ public string TestComplexWildcardQuery(string queryString)
        public string TestPrefixQuery(string queryString)
        {
            var query = JsonConvert.DeserializeObject<Query>(queryString);
-            var visitor = new ElasticSearchDSLVisitor(SchemaRetrieverMock.CreateMockSchemaRetriever());
+            var visitor = VisitorTestsUtils.CreateAndVisitRootVisitor();


all of those tests (like this) are now visited twice? was once before...

Itye added 2 commits February 26, 2020 17:52

Support date range filter

089603c

Merge remote-tracking branch 'origin/master' into feature/AddTimeRang…

85e28c8

…eQuery

itye-msft requested a review from a team as a code owner February 26, 2020 15:56

balteravishay reviewed Feb 27, 2020

View reviewed changes

tamirkamara requested changes Feb 27, 2020

View reviewed changes

eladiw mentioned this pull request Feb 27, 2020

add reference to boolean in function GetIsFieldNumeric #9

Closed

eladiw assigned tamirkamara and LizaShak Mar 1, 2020

Itye added 5 commits March 3, 2020 10:30

merge master

353a8e5

fixed UT

163e727

Merge branch 'master' into feature/AddTimeRangeQuery

7112c3b

fix

cdb4964

refactor

7a44d5b

tamirkamara approved these changes Mar 3, 2020

View reviewed changes

eladiw reviewed Mar 4, 2020

View reviewed changes

eladiw requested changes Mar 4, 2020

View reviewed changes

Itye added 2 commits March 5, 2020 10:44

Merge branch 'master' into feature/AddTimeRangeQuery

315cd68

fixes

4877617

eladiw reviewed Mar 5, 2020

View reviewed changes

eladiw approved these changes Mar 5, 2020

View reviewed changes

eladiw merged commit 1ed785f into master Mar 5, 2020

eladiw deleted the feature/AddTimeRangeQuery branch March 5, 2020 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Date range filter #12

Support Date range filter #12

itye-msft commented Feb 26, 2020

balteravishay Feb 27, 2020

itye-msft Mar 3, 2020

itye-msft Mar 3, 2020

balteravishay Feb 27, 2020

tamirkamara Feb 27, 2020

itye-msft Mar 3, 2020

eladiw Mar 4, 2020

eladiw Mar 4, 2020

eladiw Mar 4, 2020

eladiw Mar 4, 2020

eladiw Mar 4, 2020

itye-msft Mar 5, 2020

eladiw Mar 4, 2020

eladiw Mar 4, 2020

eladiw left a comment

eladiw Mar 5, 2020

	var t = await ClauseFieldTypeProcessor.GetType(schemaRetriever, fieldName);
	var fieldType = await ClauseFieldTypeProcessor.GetType(schemaRetriever, fieldName);

Support Date range filter #12

Support Date range filter #12

Conversation

itye-msft commented Feb 26, 2020

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

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

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

eladiw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment