Skip to content

Conversation

@jimczi
Copy link
Contributor

@jimczi jimczi commented Jun 1, 2017

This change ensures that there is a single parent-join field defined per mapping.
The verification is done through the addition of a special field mapper (MetaJoinFieldMapper) with a unique name (_parent_join) that is registered to the mapping service
when the first parent-join field is defined. If a new parent-join is added, this field mapper will clash with the new one and the update will fail.
This change also simplifies the parent join fetch sub phase by retrieving the parent-join field without iterating on all fields in the mapping.

This change ensures that there is a single parent-join field defined per mapping.
The verification is done through the addition of a special field mapper (MetaJoinFieldMapper) with a unique name (_parent_join) that is registered to the mapping service
when the first parent-join field is defined. If a new parent-join is added, this field mapper will clash with the new one and the update will fail.
This change also simplifies the parent join fetch sub phase by retrieving the parent-join field without iterating on all fields in the mapping.
@jimczi
Copy link
Contributor Author

jimczi commented Jun 1, 2017

\cc @jpountz

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a question and a small comment. LGTM

* Returns the {@link ParentJoinFieldMapper} associated with the <code>service</code> or null
* if there is no parent-join field in this mapping.
*/
public static ParentJoinFieldMapper getMapper(MapperService service) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 (I added the same method here locally (differently implemented) )

List<ParentIdFieldMapper> parentIdFields) {
super(simpleName, fieldType, Defaults.FIELD_TYPE, indexSettings, MultiFields.empty(), null);
this.parentIdFields = parentIdFields;
this.uniqueFieldMapper = uniqueFieldMapper;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also add uniqueFieldMapper.setFieldMapper(this) here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, thanks

return parentIdFields.stream().map((field) -> (Mapper) field).iterator();
List<Mapper> mappers = new ArrayList<> (parentIdFields);
mappers.add(uniqueFieldMapper);
return mappers.iterator();//parentIdFields.stream().map((field) -> (Mapper) field).iterator();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment?

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an ok hack for now, let's just make sure we revisit later, eg. the error message could be better.

@jimczi jimczi merged commit f4aee1e into elastic:master Jun 2, 2017
@jimczi
Copy link
Contributor Author

jimczi commented Jun 2, 2017

Thanks @jpountz and @martijnvg

let's just make sure we revisit later, eg. the error message could be better.

Yep, we also need to state clearly in the docs why it is forbidden.

@jimczi jimczi deleted the unique_parent_join_field branch June 2, 2017 07:22
jasontedor added a commit to s12v/elasticsearch that referenced this pull request Jun 2, 2017
* master: (62 commits)
  Handle already closed while filling gaps
  [DOCS] Clarify behaviour of scripted-metric arg with empty parent buckets
  [DOCS] Clarify connections and gateway nodes selection in cross cluster search docs (elastic#24859)
  Java api: Remove unneeded getTookInMillis method (elastic#23923)
  Adds nodes usage API to monitor usages of actions (elastic#24169)
  Add superset size to Significant Term REST response (elastic#24865)
  Disallow multiple parent-join fields per mapping (elastic#25002)
  [Test] Remove unused test resources in core (elastic#25011)
  Scripting: Add optional context parameter to put stored script requests (elastic#25014)
  Extract a common base class for scroll executions (elastic#24979)
  Build: fix version sorting
  Build: Move verifyVersions to new branchConsistency task (elastic#25009)
  Add backwards compatibility indices
  Build: improve verifyVersions error message (elastic#25006)
  Add version 5.4.2 constant
  Docs: More search speed advices. (elastic#24802)
  Add version 5.3.3 constant
  Reorganize docs of global ordinals. (elastic#24982)
  Provide the TransportRequest during validation of a search context (elastic#24985)
  [TEST] fix SearchIT assertion to also accept took set to 0
  ...
jimczi added a commit to jimczi/elasticsearch that referenced this pull request Jun 7, 2017
This change ensures that there is a single parent-join field defined per mapping.
The verification is done through the addition of a special field mapper (MetaJoinFieldMapper) with a unique name (_parent_join) that is registered to the mapping service
when the first parent-join field is defined. If a new parent-join is added, this field mapper will clash with the new one and the update will fail.
This change also simplifies the parent join fetch sub phase by retrieving the parent-join field without iterating on all fields in the mapping.
jimczi added a commit that referenced this pull request Jun 7, 2017
This is a full backport of the typeless parent child feature (parent-join) introduced in master.
It includes:
* Introduce ParentJoinFieldMapper, a field mapper that creates parent/child relation within documents of the same index (#24978)
* Disallow multiple parent-join fields per mapping (#25002)
* Change `has_child`, `has_parent` queries and `childen` aggregation to work with the new join field type and at the same time maintaining support for the `_parent` meta field type.
* Move parent_id query to the parent-join module (#25072)
* Changed inner_hits to work with the new join field type and
at the same time maintaining support for the `_parent` meta field type

Relates #20257
@pangyikhei
Copy link
Contributor

pangyikhei commented Jun 8, 2017

If I'm interpreting the statement in the blog https://www.elastic.co/blog/this-week-in-elasticsearch-and-apache-lucene-2017-06-05 correctly:

Good progress has been made in 5.5 with the introduction of the ParentJoinFieldMapper, a field mapper that creates parent/child relation within documents of the same index. Only one join-field is allowed per index.

Does this mean we can't have a parent with multiple child types anymore?
Ex. Question type as the parent of Answer and Comment child types.

What about nested parent-child relationships?
Ex. Question with child types Answer and Question_Comment and Answer having a child type Answer_Comment

@martijnvg
Copy link
Member

No, it just means one join field can be configured per index. However a join field can be configured with multiple relations like you described above.

@clintongormley clintongormley added the :Search/Search Search-related issues that do not fall into other categories label Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement :Search/Search Search-related issues that do not fall into other categories v5.6.0 v6.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants