Skip to content

Conversation

@alexshadow007
Copy link
Contributor

Closes #27323

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@dnhatn
Copy link
Member

dnhatn commented Nov 12, 2017

@elasticmachine please test this.

@dnhatn dnhatn added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Nov 12, 2017
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

This looks good, but I have a couple asks.

Copy link
Member

Choose a reason for hiding this comment

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

I think this initialization should stay, for the case meta is not defined in the json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjernst I was thinking of using null for this case. Or is empty map better?
And which value must be used for unset meta?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with null. I had thought changing this from emtpy map to null would require updating Mapping.toXContent. However, it looks like it currently handles both cases. I would update that to remove the empty case (which means explicitly putting an empty _meta would be serialized, not disappear).

Copy link
Member

Choose a reason for hiding this comment

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

I would flip this logic to remove negations. ie mergeWith.meta == null ? meta : mergeWith.meta. Positive logic is easier to reason about. It might also be better to just move it out to a local instead of trying to inline it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@rjernst
Copy link
Member

rjernst commented Nov 16, 2017

@alexshadow007 Can you update Mapping.toXContent as I suggested?

@alexshadow007
Copy link
Contributor Author

@rjernst Done

@rjernst
Copy link
Member

rjernst commented Nov 16, 2017

@elasticmachine test this please

@rjernst
Copy link
Member

rjernst commented Nov 16, 2017

Thanks @alexshadow007, I'll merge once CI completes again.

@alexshadow007
Copy link
Contributor Author

@rjernst Rebased branch onto master

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.

LGTM too.

@rjernst Pinging you as you said you would merge it, but if you don't have time, I can do it.

@jpountz jpountz merged commit 43a91f4 into elastic:master Nov 24, 2017
jpountz pushed a commit that referenced this pull request Nov 24, 2017
jpountz pushed a commit that referenced this pull request Nov 24, 2017
martijnvg added a commit that referenced this pull request Nov 24, 2017
* es/master: (38 commits)
  Backport wait_for_initialiazing_shards to cluster health API
  Carry over version map size to prevent excessive resizing (#27516)
  Fix scroll query with a sort that is a prefix of the index sort (#27498)
  Delete shard store files before restoring a snapshot (#27476)
  Replace `delimited_payload_filter` by `delimited_payload` (#26625)
  CURRENT should not be a -SNAPSHOT version if build.snapshot is false (#27512)
  Fix merging of _meta field (#27352)
  Remove unused method (#27508)
  unmuted test, this has been fixed by #27397
  Consolidate version numbering semantics (#27397)
  Add wait_for_no_initializing_shards to cluster health API (#27489)
  [TEST] use routing partition size based on the max routing shards of the second split
  Adjust CombinedDeletionPolicy for multiple commits (#27456)
  Update composite-aggregation.asciidoc
  Deprecate `levenstein` in favor of `levenshtein` (#27409)
  Automatically prepare indices for splitting (#27451)
  Validate `op_type` for `_create` (#27483)
  Minor ShapeBuilder cleanup
  muted test
  Decouple nio constructs from the tcp transport (#27484)
  ...
martijnvg added a commit that referenced this pull request Nov 24, 2017
* es/6.x: (30 commits)
  Add wait_for_no_initializing_shards to cluster health API (#27489)
  Carry over version map size to prevent excessive resizing (#27516)
  Fix scroll query with a sort that is a prefix of the index sort (#27498)
  Delete shard store files before restoring a snapshot (#27476)
  CURRENT should not be a -SNAPSHOT version if build.snapshot is false (#27512)
  Fix merging of _meta field (#27352)
  test: do not run percolator query builder bwc test against 5.x versions
  Remove unused method (#27508)
  Consolidate version numbering semantics (#27397)
  Adjust CombinedDeletionPolicy for multiple commits (#27456)
  Minor ShapeBuilder cleanup
  [GEO] Deprecate ShapeBuilders and decouple geojson parse logic
  Improve docs for split API in 6.1/6.x (#27504)
  test: use correct pre 6.0.0-alpha1 format
  Update composite-aggregation.asciidoc
  Deprecate `levenstein` in favor of `levenshtein` (#27409)
  Decouple nio constructs from the tcp transport (#27484)
  Bump version from 6.1 to 6.2
  Fix whitespace in Security.java
  Tighten which classes can exit
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Search Foundations/Mapping Index mappings, including merging and defining field types v6.1.0 v6.2.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants