[GitHub] [jackrabbit-oak] thomasmueller commented on a change in pull request #444: OAK-9647: oak-search-elastic: nullCheckEnabled and notNullCheckEnabled queries not working as expected

[GitBox <gi...@apache.org>]

thomasmueller commented on a change in pull request #444:
URL: https://github.com/apache/jackrabbit-oak/pull/444#discussion_r772377566



##########
File path: oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/PropertyIndexCommonTest.java
##########
@@ -171,6 +180,56 @@ public void propertyExistenceQuery() throws Exception {
                 Arrays.asList("/test/a", "/test/b")));
     }
 
+    @Test
+    public void propertyExistenceQueryWithNullCheck() throws Exception {
+        NodeTypeRegistry.register(root, IOUtils.toInputStream(TestUtil.TEST_NODE_TYPE), "test nodeType");
+
+        Tree idx = indexOptions.setIndex(root, "test1",
+                indexOptions.createIndex(indexOptions.createIndexDefinitionBuilder(), TestUtil.NT_TEST, false, "propa", "propb"));
+        Tree props = TestUtil.newRulePropTree(idx, TestUtil.NT_TEST);
+        Tree prop = props.addChild(TestUtil.unique("prop"));
+        prop.setProperty(PROP_NAME, "propa");
+        prop.setProperty(PROP_PROPERTY_INDEX, true);
+        prop.setProperty(PROP_NOT_NULL_CHECK_ENABLED, true);
+        root.commit();
+
+        Tree test = root.getTree("/").addChild("test");
+        createNodeWithType(test, "a", "oak:TestNode").setProperty("propa", "a");
+        createNodeWithType(test, "b", "oak:TestNode").setProperty("propa", "c");
+        createNodeWithType(test, "c", "oak:TestNode").setProperty("propb", "e");
+        root.commit();
+
+        String query = "select [jcr:path] from [oak:TestNode] where [propa] is not null";
+        String explanation = explain(query);
+        assertThat(explanation, containsString(indexOptions.getIndexType() + ":test1(/oak:index/test1) "));
+        assertEventually(() -> assertQuery(query, asList("/test/a", "/test/b")));
+    }
+
+    @Test
+    public void propertyNonExistenceQuery() throws Exception {
+        NodeTypeRegistry.register(root, IOUtils.toInputStream(TestUtil.TEST_NODE_TYPE), "test nodeType");
+
+        Tree idx = indexOptions.setIndex(root, "test2",
+                indexOptions.createIndex(indexOptions.createIndexDefinitionBuilder(), TestUtil.NT_TEST, false, "propa", "propb"));
+        Tree props = TestUtil.newRulePropTree(idx, TestUtil.NT_TEST);
+        Tree prop = props.addChild(TestUtil.unique("prop"));
+        prop.setProperty(PROP_NAME, "propa");
+        prop.setProperty(PROP_PROPERTY_INDEX, true);
+        prop.setProperty(PROP_NULL_CHECK_ENABLED, true);
+        root.commit();
+
+        Tree test = root.getTree("/").addChild("test");
+        createNodeWithType(test, "a", "oak:TestNode").setProperty("propa", "a");
+        createNodeWithType(test, "b", "oak:TestNode").setProperty("propa", "c");
+        createNodeWithType(test, "c", "oak:TestNode").setProperty("propb", "e");
+        root.commit();
+
+        String query = "select [jcr:path] from [oak:TestNode] where [propa] is null";
+        String explanation = explain(query);
+        assertThat(explanation, containsString(indexOptions.getIndexType() + ":test2(/oak:index/test2) "));

Review comment:
       I think it is insufficient. The old test verified the elastic query, however the new test doesn't. The test wouldn't fail even if the query string isn't correct, as the Oak query engine would filter out the data.

##########
File path: oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/util/TermQueryBuilderFactory.java
##########
@@ -128,20 +115,17 @@ public static TermQueryBuilder newNullPropQuery(String propName) {
         } else if (pr.first != null && pr.last != null) {
             return newRangeQuery(propertyName, first, last,
                     pr.firstIncluding, pr.lastIncluding);
-        } else if (pr.first != null && pr.last == null) {
+        } else if (pr.first != null) {

Review comment:
       This change seems to be unrelated to the oak issue. I see that technically, it's not needed.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org