You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by ng...@apache.org on 2022/08/09 04:46:50 UTC
[jackrabbit-oak] branch trunk updated: OAK-9878 | OAK-9858 | Fixing failing(Ignored) tests for ES (#653)
This is an automated email from the ASF dual-hosted git repository.
ngupta pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/trunk by this push:
new cdfc855a2b OAK-9878 | OAK-9858 | Fixing failing(Ignored) tests for ES (#653)
cdfc855a2b is described below
commit cdfc855a2b26358baaf67e0f799dcec1fa78cc13
Author: nit0906 <ni...@gmail.com>
AuthorDate: Tue Aug 9 10:16:45 2022 +0530
OAK-9878 | OAK-9858 | Fixing failing(Ignored) tests for ES (#653)
* OAK-9858 | Making ES indexing sync(similar to lucene) for IndexQueryCommonTests that are based on text files
* Re-enabling traversal since it's needed by some join queries in tests, also making changes in sql2 test to accomodate for newer properties in test collateral since this test was disabled
* Fixing sort for ES index in case of non explicit properties covered via some regex property
* Fixing like queries in ES in case of fulltext/analyzed properties
* Fixing queries on node name in case of ES index - needed to use :nodeName..keyword instead of :nodeName
* Code Cleanup
* Incorporating review comments
* Fixing minor issue in like query
* Added missing test for path() function
---
.../jackrabbit/oak/query/AbstractQueryTest.java | 10 +++-
.../org/apache/jackrabbit/oak/query/sql2.txt | 12 +++--
.../index/lucene/LuceneIndexQueryCommonTest.java | 21 +++-----
.../index/elastic/query/ElasticRequestHandler.java | 24 ++++++---
.../index/elastic/ElasticIndexQueryCommonTest.java | 32 ++----------
.../oak/plugins/index/FunctionIndexCommonTest.java | 30 +++++++++++
.../oak/plugins/index/IndexQueryCommonTest.java | 58 +++++++++++++++++-----
7 files changed, 120 insertions(+), 67 deletions(-)
diff --git a/oak-core/src/test/java/org/apache/jackrabbit/oak/query/AbstractQueryTest.java b/oak-core/src/test/java/org/apache/jackrabbit/oak/query/AbstractQueryTest.java
index f096b64c68..c1914aedfc 100644
--- a/oak-core/src/test/java/org/apache/jackrabbit/oak/query/AbstractQueryTest.java
+++ b/oak-core/src/test/java/org/apache/jackrabbit/oak/query/AbstractQueryTest.java
@@ -29,6 +29,7 @@ import java.nio.charset.StandardCharsets;
import java.text.ParseException;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
@@ -227,7 +228,14 @@ public abstract class AbstractQueryTest {
w.println(line);
line = line.substring("commit".length()).trim();
apply(root, line);
- root.commit();
+ // This part of code is used by both lucene and elastic tests.
+ // The index definitions in these tests don't have async property set
+ // So lucene, in this case behaves in a sync manner. But the tests fail on Elastic,
+ // since ES indexing is always async.
+ // The below commit info map (sync-mode = rt) would make Elastic use RealTimeBulkProcessHandler.
+ // This would make ES indexes also sync.
+ // This will NOT have any impact on the lucene tests.
+ root.commit(Collections.singletonMap("sync-mode", "rt"));
}
w.flush();
}
diff --git a/oak-core/src/test/resources/org/apache/jackrabbit/oak/query/sql2.txt b/oak-core/src/test/resources/org/apache/jackrabbit/oak/query/sql2.txt
index 7168fe6d3d..d7dfba98d0 100644
--- a/oak-core/src/test/resources/org/apache/jackrabbit/oak/query/sql2.txt
+++ b/oak-core/src/test/resources/org/apache/jackrabbit/oak/query/sql2.txt
@@ -407,21 +407,22 @@ commit /testRoot + "test2": { "name": "World!" }
commit /testRoot + "test3": { "name": "Hallo" }
commit /testRoot + "test4": { "name": "10%" }
commit /testRoot + "test5": { "name": "10 percent" }
+commit /testRoot + "test6": { "name": "brave" }
select a.name
from [nt:base] as a
where a.name is not null and isdescendantnode(a , '/testRoot') order by upper(a.name)
10 percent
10%
+brave
Hallo
hello
World!
select [jcr:path]
from [nt:base]
- where length(name) = 5
-/testRoot/test
-/testRoot/test3
+ where length(name) = 10
+/testRoot/test5
select [jcr:path]
from [nt:base]
@@ -440,8 +441,9 @@ select [jcr:path]
select [jcr:path]
from [nt:base]
- where name like '%o_%'
-/testRoot/test2
+ where name like '%e_%'
+/testRoot/test
+/testRoot/test5
select [jcr:path]
from [nt:base]
diff --git a/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexQueryCommonTest.java b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexQueryCommonTest.java
index 93f8da3ee1..84d9419162 100644
--- a/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexQueryCommonTest.java
+++ b/oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneIndexQueryCommonTest.java
@@ -23,18 +23,11 @@ import org.apache.jackrabbit.oak.plugins.index.LuceneIndexOptions;
import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore;
import org.junit.After;
import org.junit.Rule;
-import org.junit.Test;
import org.junit.rules.TemporaryFolder;
-
-import static org.junit.Assert.assertEquals;
-
import java.io.File;
-import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
-import javax.jcr.query.Query;
-
/**
* Tests the query engine using the default index implementation: the
* {@link LuceneIndexProvider}
@@ -59,14 +52,6 @@ public class LuceneIndexQueryCommonTest extends IndexQueryCommonTest {
executorService.shutdown();
}
- @Test
- public void descendantTestWithIndexTagExplain() throws Exception {
- List<String> result = executeQuery(
- "explain select [jcr:path] from [nt:base] where isdescendantnode('/test') option (index tag x)", Query.JCR_SQL2);
- assertEquals("[[nt:base] as [nt:base] /* lucene:test-index(/oak:index/test-index) :ancestors:/test\n"
- + " where isdescendantnode([nt:base], [/test]) */]", result.toString());
- }
-
@Override
public String getContainsValueForEqualityQuery_native() {
return "+:ancestors:/test +propa:bar";
@@ -91,4 +76,10 @@ public class LuceneIndexQueryCommonTest extends IndexQueryCommonTest {
public String getContainsValueForNotNullQuery_native() {
return "+:ancestors:/test +propa:[* TO *]";
}
+
+ @Override
+ public String getExplainValueForDescendantTestWithIndexTagExplain() {
+ return "[nt:base] as [nt:base] /* lucene:test-index(/oak:index/test-index) :ancestors:/test" +
+ " where isdescendantnode([nt:base], [/test]) */";
+ }
}
diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java
index 4c932cb034..717dbec49d 100644
--- a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java
+++ b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java
@@ -41,6 +41,7 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
+import java.util.Optional;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.BiConsumer;
import java.util.function.BiPredicate;
@@ -214,6 +215,7 @@ public class ElasticRequestHandler {
return DEFAULT_SORTS;
}
Map<String, List<PropertyDefinition>> indexProperties = elasticIndexDefinition.getPropertiesByName();
+
boolean hasTieBreaker = false;
List<SortOptions> list = new ArrayList<>();
for (QueryIndex.OrderEntry o : sortOrder) {
@@ -225,7 +227,12 @@ public class ElasticRequestHandler {
hasTieBreaker = true;
} else if (JCR_SCORE.equals(sortPropertyName)) {
fieldName = "_score";
- } else if (indexProperties.containsKey(sortPropertyName)) {
+ } else if (indexProperties.containsKey(sortPropertyName) || elasticIndexDefinition.getDefinedRules()
+ .stream().anyMatch(rule -> rule.getConfig(sortPropertyName) != null)) {
+ // There are 2 conditions in this if statement -
+ // First one returns true if sortPropertyName is one of the defined indexed properties on the index
+ // Second condition returns true if sortPropertyName might not be explicitly defined but covered by a regex property
+ // in any of the defined index rules.
fieldName = elasticIndexDefinition.getElasticKeyword(sortPropertyName);
} else {
LOG.warn("Unable to sort by {} for index {}", sortPropertyName, elasticIndexDefinition.getIndexName());
@@ -732,11 +739,11 @@ public class ElasticRequestHandler {
return Query.of(q -> q.bool(bqBuilder.build()));
}
- private static Query nodeName(Filter.PropertyRestriction pr) {
+ private Query nodeName(Filter.PropertyRestriction pr) {
String first = pr.first != null ? pr.first.getValue(Type.STRING) : null;
if (pr.first != null && pr.first.equals(pr.last) && pr.firstIncluding && pr.lastIncluding) {
// [property]=[value]
- return Query.of(q -> q.term(t -> t.field(FieldNames.NODE_NAME).value(FieldValue.of(first))));
+ return Query.of(q -> q.term(t -> t.field(elasticIndexDefinition.getElasticKeyword(FieldNames.NODE_NAME)).value(FieldValue.of(first))));
}
if (pr.isLike) {
@@ -746,7 +753,7 @@ public class ElasticRequestHandler {
throw new IllegalStateException("For nodeName queries only EQUALS and LIKE are supported " + pr);
}
- private static Query like(String name, String first) {
+ private Query like(String name, String first) {
first = first.replace('%', WildcardQuery.WILDCARD_STRING);
first = first.replace('_', WildcardQuery.WILDCARD_CHAR);
@@ -754,19 +761,24 @@ public class ElasticRequestHandler {
int indexOfWC = first.indexOf(WildcardQuery.WILDCARD_CHAR);
int len = first.length();
+ // Non full text (Non analyzed) properties are keyword types in ES. For those field would be equal to name.
+ // Analyzed properties, however are of text type on which we can't perform wildcard or prefix queries so we use the keyword (sub) field
+ // by appending .keyword to the name here.
+ String field = elasticIndexDefinition.getElasticKeyword(name);
+
if (indexOfWS == len || indexOfWC == len) {
// remove trailing "*" for prefix query
first = first.substring(0, first.length() - 1);
if (JCR_PATH.equals(name)) {
return newPrefixPathQuery(first);
} else {
- return newPrefixQuery(name, first);
+ return newPrefixQuery(field, first);
}
} else {
if (JCR_PATH.equals(name)) {
return newWildcardPathQuery(first);
} else {
- return newWildcardQuery(name, first);
+ return newWildcardQuery(field, first);
}
}
}
diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexQueryCommonTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexQueryCommonTest.java
index 4594893aea..5f05e98ae3 100644
--- a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexQueryCommonTest.java
+++ b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexQueryCommonTest.java
@@ -21,14 +21,6 @@ import org.apache.jackrabbit.oak.api.ContentRepository;
import org.apache.jackrabbit.oak.plugins.index.IndexQueryCommonTest;
import org.apache.jackrabbit.oak.plugins.memory.MemoryNodeStore;
import org.junit.ClassRule;
-import org.junit.Ignore;
-import org.junit.Test;
-
-import static org.junit.Assert.assertEquals;
-
-import java.util.List;
-
-import javax.jcr.query.Query;
public class ElasticIndexQueryCommonTest extends IndexQueryCommonTest {
@@ -48,15 +40,6 @@ public class ElasticIndexQueryCommonTest extends IndexQueryCommonTest {
return repositoryOptionsUtil.getOak().createContentRepository();
}
- @Test
- public void descendantTestWithIndexTagExplain() {
- List<String> result = executeQuery(
- "explain select [jcr:path] from [nt:base] where isdescendantnode('/test') option (index tag x)", Query.JCR_SQL2);
- assertEquals("[[nt:base] as [nt:base] /* elasticsearch:test-index(/oak:index/test-index) "
- + "{\"bool\":{\"filter\":[{\"term\":{\":ancestors\":{\"value\":\"/test\"}}}]}}\n"
- + " where isdescendantnode([nt:base], [/test]) */]", result.toString());
- }
-
@Override
public String getContainsValueForEqualityQuery_native() {
return "\"filter\":[{\"term\":{\":ancestors\":{\"value\":\"/test\"}}},{\"term\":{\"propa.keyword\":{\"value\":\"bar\"}}}]";
@@ -86,17 +69,10 @@ public class ElasticIndexQueryCommonTest extends IndexQueryCommonTest {
}
@Override
- @Ignore("Failing on ES")
- @Test
- public void isChildNodeTest() throws Exception {
- super.isChildNodeTest();
- }
-
- @Override
- @Ignore("OAK-9858")
- @Test
- public void sql2FullText() throws Exception {
- super.sql2FullText();
+ public String getExplainValueForDescendantTestWithIndexTagExplain() {
+ return "[nt:base] as [nt:base] /* elasticsearch:test-index(/oak:index/test-index) "
+ + "{\"bool\":{\"filter\":[{\"term\":{\":ancestors\":{\"value\":\"/test\"}}}]}}"
+ + " where isdescendantnode([nt:base], [/test]) */";
}
}
diff --git a/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/FunctionIndexCommonTest.java b/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/FunctionIndexCommonTest.java
index e56b718ab2..e4c6f9453e 100644
--- a/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/FunctionIndexCommonTest.java
+++ b/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/FunctionIndexCommonTest.java
@@ -221,6 +221,36 @@ public abstract class FunctionIndexCommonTest extends AbstractQueryTest {
assertQuery(query, paths);
}
+ @Test
+ public void path() throws Exception {
+ Tree index = createIndex("pathIndex", Collections.<String>emptySet());
+ Tree func = index.addChild(FulltextIndexConstants.INDEX_RULES)
+ .addChild("nt:base")
+ .addChild(FulltextIndexConstants.PROP_NODE)
+ .addChild("pathFunction");
+ func.setProperty(FulltextIndexConstants.PROP_FUNCTION, "path()");
+
+ Tree test = root.getTree("/").addChild("test");
+ test.addChild("hello");
+ test.addChild("world");
+ test.addChild("hello world");
+ root.commit();
+ postCommitHook();
+
+ String query = "select [jcr:path] from [nt:base] where path() = '/test/world'";
+ assertThat(explain(query), containsString(getIndexProvider() + "pathIndex(/oak:index/pathIndex)"));
+ assertQuery(query, asList("/test/world"));
+
+ query = "select [jcr:path] from [nt:base] where path() like '%hell%'";
+ assertThat(explain(query), containsString(getIndexProvider() + "pathIndex(/oak:index/pathIndex)"));
+ assertQuery(query, asList("/test/hello", "/test/hello world"));
+
+ query = "select [jcr:path] from [nt:base] where path() like '%ll_'";
+ assertThat(explain(query), containsString(getIndexProvider() + "pathIndex(/oak:index/pathIndex)"));
+ assertQuery(query, asList("/test/hello"));
+
+ }
+
@Test
public void testOrdering2() throws Exception {
Tree index = root.getTree("/");
diff --git a/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java b/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java
index b8e076571f..ed9c7ba041 100644
--- a/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java
+++ b/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java
@@ -56,7 +56,6 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
@Override
protected void createTestIndexNode() throws Exception {
- setTraversalEnabled(false);
Tree index = root.getTree("/");
indexDefn = createTestIndexNode(index, indexOptions.getIndexType());
TestUtil.useV2(indexDefn);
@@ -78,16 +77,27 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
Tree dateProp = TestUtil.enableForOrdered(props, "propDate");
dateProp.setProperty(FulltextIndexConstants.PROP_TYPE, "Date");
+ // Note - certain tests in this class like #sql2 test regex based like queries.
+ // And since all the tests here use this common full text index - please be careful while adding any new properties.
+ // For example - #sql2() tests with a query on length of name property.
+ // Since this is a fulltext index with a regex property that indexes everything, those property names are also indexed.
+ // So if we add any property with propName that has length equal to what that test expects - that will effectively break the #sql2() test (giving more results).
+ // Ideally one would see the test failing while adding new properties - but there have been cases where this test was ignored due to a different reason
+ // and adding a new property added more failure reasons.
+
+ // So just be careful while changing the test collateral/setup here.
+
root.commit();
}
+ // TODO : The below 3 tests - #sql1, #sq2 and #sql2FullText need refactoring.
+ // These are huge tests with multiple queries running and verification happening in the end by comparing against results in an expected test file.
+ // These could possibly be broken down into several smaller tests instead which would make debugging much easier.
@Test
public void sql1() throws Exception {
test("sql1.txt");
}
- // TODO: Test failing on Lucene and ES
- @Ignore("OAK-9858")
@Test
public void sql2() throws Exception {
test("sql2.txt");
@@ -167,6 +177,26 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
});
}
+ @Test
+ public void descendantTestWithIndexTagExplain() throws Exception {
+ Tree test = root.getTree("/").addChild("test");
+ test.addChild("a");
+ test.addChild("b");
+ root.commit();
+
+ String query = "explain select [jcr:path] from [nt:base] where isdescendantnode('/test') option (index tag x)";
+ assertEventually(getAssertionForExplain(query, Query.JCR_SQL2, getExplainValueForDescendantTestWithIndexTagExplain(), true));
+ }
+
+ // Check if this is a valid behaviour or not ?
+ // This was discovered when we removed setTraversalEnabled(false); from the test setup.
+ @Ignore("Index not picked even when using option tag if traversal cost is lower")
+ @Test
+ public void descendantTestWithIndexTagExplainWithNoData() {
+ String query = "explain select [jcr:path] from [nt:base] where isdescendantnode('/test') option (index tag x)";
+ assertEventually(getAssertionForExplain(query, Query.JCR_SQL2, getExplainValueForDescendantTestWithIndexTagExplain(), true));
+ }
+
@Test
public void descendantTest2() throws Exception {
Tree test = root.getTree("/").addChild("test");
@@ -184,8 +214,6 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
});
}
- // TODO: Test failing on Lucene and ES
- @Ignore("OAK-9859")
@Test
public void isChildNodeTest() throws Exception {
Tree tree = root.getTree("/");
@@ -573,7 +601,7 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
String query = "explain /jcr:root/test//*[propa!='bar']";
- assertEventually(getAssertionForExplainContains(query, XPATH, getContainsValueForInequalityQuery_native()));
+ assertEventually(getAssertionForExplain(query, XPATH, getContainsValueForInequalityQuery_native(), false));
String query2 = "/jcr:root/test//*[propa!='bar']";
@@ -592,7 +620,7 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
String query = "explain select * from [nt:base] as s where propa is not null and ISDESCENDANTNODE(s, '/test')";
- assertEventually(getAssertionForExplainContains(query, SQL2, getContainsValueForNotNullQuery_native()));
+ assertEventually(getAssertionForExplain(query, SQL2, getContainsValueForNotNullQuery_native(), false));
String query2 = "select * from [nt:base] as s where propa is not null and ISDESCENDANTNODE(s, '/test')";
@@ -612,7 +640,7 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
String query = "explain //*[propa!='bar']";
- assertEventually(getAssertionForExplainContains(query, XPATH, getContainsValueForInequalityQueryWithoutAncestorFilter_native()));
+ assertEventually(getAssertionForExplain(query, XPATH, getContainsValueForInequalityQueryWithoutAncestorFilter_native(), false));
String query2 = "//*[propa!='bar']";
@@ -632,7 +660,7 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
root.commit();
String query = "explain /jcr:root/test//*[propa!='bar' and propb='world']";
- assertEventually(getAssertionForExplainContains(query, XPATH, getContainsValueForEqualityInequalityCombined_native()));
+ assertEventually(getAssertionForExplain(query, XPATH, getContainsValueForEqualityInequalityCombined_native(), false));
String query2 = "/jcr:root/test//*[propa!='bar' and propb='world']";
// Expected - nodes with both properties defined and propb with value 'world' and propa with value not equal to bar should be returned
@@ -653,7 +681,7 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
String query = "explain /jcr:root/test//*[propa='bar']";
- assertEventually(getAssertionForExplainContains(query, XPATH, getContainsValueForEqualityQuery_native()));
+ assertEventually(getAssertionForExplain(query, XPATH, getContainsValueForEqualityQuery_native(), false));
String query2 = "/jcr:root/test//*[propa='bar']";
@@ -752,7 +780,9 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
public abstract String getContainsValueForNotNullQuery_native();
- private Runnable getAssertionForExplainContains(String query, String language, String containValue) {
+ public abstract String getExplainValueForDescendantTestWithIndexTagExplain();
+
+ private Runnable getAssertionForExplain(String query, String language, String expected, boolean matchComplete) {
return () -> {
Result result = null;
try {
@@ -761,7 +791,11 @@ public abstract class IndexQueryCommonTest extends AbstractQueryTest {
fail(e.getMessage());
}
ResultRow row = result.getRows().iterator().next();
- assertTrue(row.getValue("plan").toString().contains(containValue));
+ if (matchComplete) {
+ assertEquals(row.getValue("plan").toString(), expected);
+ } else {
+ assertTrue(row.getValue("plan").toString().contains(expected));
+ }
};
}