Add parent-join module #24638

jimczi · 2017-05-12T05:37:07Z

This change adds a new module named parent-join.
The goal of this module is to provide a replacement for the _parent field but as a first step this change only moves the has_child, has_parent queries and the children aggregation to this module.
These queries and aggregations are no longer in core but they are deployed by default as a module.

Relates #20257

This change adds a new module named `parent-join`. The goal of this module is to provide a replacement for the `_parent` field but as a first step this change only moves the `has_child`, `has_parent` queries and the `children` aggregation to this module. These queries and aggregations are no longer in core but they are deployed by default as a module. Relates elastic#20257

jpountz

I had a look out of curiosity, I'm curious what @martijnvg thinks.

jpountz · 2017-05-12T09:35:47Z

core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/CustomQueryScorer.java

+         * Workaround to detect parent/child query
+         */
+        private static final String PARENT_CHILD_QUERY_NAME = "HasChildQueryBuilder$LateParsingQuery";
+        private static boolean isChildOrParentQuery(Class<?> clazz) {


Maybe make this method public and test in the parent join module that it actually detects this query?

We have a test for this in ChildQuerySearchIT#testHighlighHighlightersIgnoreParentChild

martijnvg

Looks great! I left some small comments.

martijnvg · 2017-05-12T09:25:24Z

modules/parent-join/src/main/java/org/elasticsearch/join/query/JoinQueryBuilders.java

+import org.apache.lucene.search.join.ScoreMode;
+import org.elasticsearch.index.query.QueryBuilder;
+
+public abstract class JoinQueryBuilders {


maybe change this into an enum? So that no subclasses can ever by created.

martijnvg · 2017-05-12T09:25:36Z

...s/parent-join/src/main/java/org/elasticsearch/join/aggregations/JoinAggregationBuilders.java

+
+package org.elasticsearch.join.aggregations;
+
+public abstract class JoinAggregationBuilders {


maybe change this into an enum? So that no subclasses can ever by created.

martijnvg · 2017-05-12T09:27:45Z

modules/parent-join/src/main/java/org/elasticsearch/join/ParentJoinPlugin.java

+import java.util.List;
+
+public class ParentJoinPlugin extends Plugin implements SearchPlugin {
+    private final Settings settings;


maybe remove settings fields? because it is unused.

martijnvg · 2017-05-12T09:30:26Z

buildSrc/src/main/resources/checkstyle_suppressions.xml

@@ -620,7 +620,6 @@
  <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]index[/\\]query[/\\]BoolQueryBuilderTests.java" checks="LineLength" />
  <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]index[/\\]query[/\\]BoostingQueryBuilderTests.java" checks="LineLength" />
  <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]index[/\\]query[/\\]GeoDistanceQueryBuilderTests.java" checks="LineLength" />
-  <suppress files="core[/\\]src[/\\]test[/\\]java[/\\]org[/\\]elasticsearch[/\\]index[/\\]query[/\\]HasChildQueryBuilderTests.java" checks="LineLength" />


martijnvg · 2017-05-12T09:31:46Z

core/src/main/java/org/elasticsearch/index/query/InnerHitBuilder.java

@@ -195,7 +195,8 @@ private InnerHitBuilder(InnerHitBuilder other) {
        }
    }

-    InnerHitBuilder(InnerHitBuilder other, QueryBuilder query, String parentChildType, boolean ignoreUnmapped) {
+    // TODO public for hasChild and hasParent query.
+    public InnerHitBuilder(InnerHitBuilder other, QueryBuilder query, String parentChildType, boolean ignoreUnmapped) {


maybe add a note that these methods are for internal usage only and no one should rely on this method to remain to be available?

martijnvg · 2017-05-12T09:33:21Z

core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/CustomQueryScorer.java

+        /**
+         * Workaround to detect parent/child query
+         */
+        private static final String PARENT_CHILD_QUERY_NAME = "HasChildQueryBuilder$LateParsingQuery";


not so pretty... but I'm not sure how this check can be achieved in a better way.

martijnvg · 2017-05-12T09:34:15Z

core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/CustomQueryScorer.java

+         */
+        private static final String PARENT_CHILD_QUERY_NAME = "HasChildQueryBuilder$LateParsingQuery";
+        private static boolean isChildOrParentQuery(Class<?> clazz) {
+            return clazz.getName().endsWith(PARENT_CHILD_QUERY_NAME);


maybe do this:

return clazz.getSimpeName().equals(PARENT_CHILD_QUERY_NAME)

?

martijnvg · 2017-05-12T09:57:57Z

@jimczi One other question: i don't think this needs to be pushed into the 5.x branch?

clintongormley · 2017-05-12T10:00:58Z

@jimczi One other question: i don't think this needs to be pushed into the 5.x branch?

if we can introduce it in 5.x to allow people to start adjusting their applications now, that would be preferable.

jimczi · 2017-05-12T14:00:23Z

Thanks for reviewing @martijnvg and @jpountz
I'll wait a bit before back porting this to 5.x

…Child and hasParent in the tests

… default

* Add parent-join module This change adds a new module named `parent-join`. The goal of this module is to provide a replacement for the `_parent` field but as a first step this change only moves the `has_child`, `has_parent` queries and the `children` aggregation to this module. These queries and aggregations are no longer in core but they are deployed by default as a module. Relates #20257

… default

jimczi added 3 commits May 11, 2017 23:30

clean

0d936a3

fix duplicate line

f9bd2e7

jimczi added :Parent/Child >non-issue v5.5.0 v6.0.0 labels May 12, 2017

jimczi requested a review from martijnvg May 12, 2017 05:37

jpountz approved these changes May 12, 2017

View reviewed changes

martijnvg approved these changes May 12, 2017

View reviewed changes

martijnvg mentioned this pull request May 12, 2017

Move AggregatorTestCase to test framework #24594

Closed

after review

085f98d

jimczi merged commit 279a18a into elastic:master May 12, 2017

jimczi deleted the modules/parent_join branch May 12, 2017 13:58

jimczi added backport pending and removed review labels May 12, 2017

jimczi added a commit that referenced this pull request May 12, 2017

[Tests] #24638 Fix rest high level client docs build by restoring has…

4291880

…Child and hasParent in the tests

jimczi added a commit that referenced this pull request May 12, 2017

#24638 Add the parent-join plugin in the prebuilt transport client by…

e804817

… default

jimczi added a commit that referenced this pull request May 16, 2017

#24638 Add the parent-join plugin in the prebuilt transport client by…

c0bc550

… default

jimczi removed the backport pending label Jun 6, 2017

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Parent/Child labels Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parent-join module #24638

Add parent-join module #24638

jimczi commented May 12, 2017

jpountz left a comment

jpountz May 12, 2017

jimczi May 12, 2017

martijnvg left a comment

martijnvg May 12, 2017

martijnvg May 12, 2017

martijnvg May 12, 2017

martijnvg May 12, 2017

martijnvg May 12, 2017

martijnvg May 12, 2017

martijnvg May 12, 2017

martijnvg commented May 12, 2017

clintongormley commented May 12, 2017

jimczi commented May 12, 2017


		package org.elasticsearch.join.aggregations;

		public abstract class JoinAggregationBuilders {

Add parent-join module #24638

Add parent-join module #24638

Conversation

jimczi commented May 12, 2017

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg left a 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

martijnvg commented May 12, 2017

clintongormley commented May 12, 2017

jimczi commented May 12, 2017