[GitHub] [jackrabbit-oak] fabriziofortino commented on a change in pull request #494: OAK-9696 Improve query syntax support for dynamicBoost in ElasticSearch

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

fabriziofortino commented on a change in pull request #494:
URL: https://github.com/apache/jackrabbit-oak/pull/494#discussion_r806623002



##########
File path: oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexHelper.java
##########
@@ -276,6 +276,11 @@ private static void mapIndexRules(ElasticIndexDefinition indexDefinition, XConte
             mappingBuilder.endObject();
         }
 
+        mappingBuilder.startObject(ElasticIndexDefinition.DYNAMIC_BOOST_TAGS)
+                .field("type", "text")
+                .field("analyzer", "oak_analyzer")
+                .endObject();
+

Review comment:
       I would change this. We currently support one or more dynamicBoost properties. Setting this to a fixed field won't work in case more than one field is configured. The same mapping can be moved in the dynamicBoost for-loop after line 276. I would also rename the field name using the `:fulltext` suffix:
   
   ```java
    mappingBuilder.startObject(pd.nodeName + FieldNames.FULLTEXT)
       .field("type", "text")
       .field("analyzer", "oak_analyzer")
       .endObject();
   ```
   
   This would require changes in `ElasticDocument` and `ElasticRequestHandler`

##########
File path: oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java
##########
@@ -568,7 +568,7 @@ private boolean visitTerm(String propertyName, String text, String boost, boolea
                 if (boost != null) {
                     fullTextQuery.boost(Float.parseFloat(boost));
                 }
-                BoolQueryBuilder shouldBoolQueryWrapper = boolQuery().should(fullTextQuery);
+                BoolQueryBuilder shouldBoolQueryWrapper = boolQuery().must(fullTextQuery);

Review comment:
       We could simplify this by removing a level of nesting: `fullTextQuery` can be passed as `must` clause at line 575, the rest (dynamic score queries) should go in the `should` clause.

##########
File path: oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticDynamicBoostQueryTest.java
##########
@@ -152,15 +152,53 @@ public void testQueryDynamicBoostOrder() throws Exception {
         });
     }
 
-    // dynamic boost: space is explained as OR instead of AND, this should be documented
     @Test
-    public void testQueryDynamicBoostSpace() throws Exception {
+    public void testQueryDynamicBoostWildcard() throws Exception {
         configureIndex();
         prepareTestAssets();
 
         assertEventually(() -> {
-            assertQuery("select [jcr:path] from [dam:Asset] where contains(@title, 'blue flower')", SQL2,
+            assertQuery("select [jcr:path] from [dam:Asset] where contains(@title, 'blu?')", SQL2, Arrays.asList("/test/asset3"));
+            assertQuery("select [jcr:path] from [dam:Asset] where contains(@title, 'bl?e')", SQL2, Arrays.asList("/test/asset3"));
+            assertQuery("select [jcr:path] from [dam:Asset] where contains(@title, '?lue')", SQL2, Arrays.asList("/test/asset3"));
+            assertQuery("select [jcr:path] from [dam:Asset] where contains(@title, 'coff*')", SQL2, Arrays.asList("/test/asset2"));
+            assertQuery("select [jcr:path] from [dam:Asset] where contains(@title, 'co*ee')", SQL2, Arrays.asList("/test/asset2"));
+            assertQuery("select [jcr:path] from [dam:Asset] where contains(@title, '*ffee')", SQL2, Arrays.asList("/test/asset2"));
+        });
+    }
+
+    @Test
+    public void testQueryDynamicBoosSpace() throws Exception {

Review comment:
       typo 
   
   ```suggestion
       public void testQueryDynamicBoostSpace() throws Exception {
   ```




-- 
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