Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for field aliases to 6.x. #32184

Merged
merged 15 commits into from
Jul 24, 2018

Conversation

jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Jul 19, 2018

This backport is composed of the commits from the original PR (#32172), three backported bugfixes, plus three new commits that address issues specific to 6.x. The first batch of commits are direct cherry-picks from master, with minor conflict resolution. The additional commits come last, and don't have links to previous PRs in their title.

The backport went smoothly overall, but there were some conflicts relating to the fact that in 6.x., multiple mapping types can be present.

@jtibshirani jtibshirani force-pushed the field-aliases-backport branch 4 times, most recently from 2103d3e to c95caa2 Compare July 20, 2018 00:48
@colings86 colings86 added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Jul 20, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jtibshirani
Copy link
Contributor Author

@colings86 @jpountz this is ready for review.

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'm not a fan of the fact that we need more complex logic to handle the fact that there may be more types. Should we consider only allowing aliases on 6.x indices so that we don't have to deal with this case and have exactly the same logic as in master?

@colings86
Copy link
Contributor

I'm not a fan of the fact that we need more complex logic to handle the fact that there may be more types. Should we consider only allowing aliases on 6.x indices so that we don't have to deal with this case and have exactly the same logic as in master?

Personally I would be fine with that trade-off

@jtibshirani
Copy link
Contributor Author

jtibshirani commented Jul 23, 2018

Thanks @jpountz for the review! I also wasn't happy with the extra logic.

@ruflin do you have thoughts as to whether the suggestion above would work with your plans?

I'm also curious in general as to whether this would make it a lot more difficult for users to try out the feature. It would be particularly unfortunate if field aliases could help in the migration to one type, but they weren't available for pre-6.x indices.

@ruflin
Copy link
Member

ruflin commented Jul 23, 2018

All new indices in 6.x can only have 1 type anyway? I assume aliases will be mostly used on new indices created so I'm not too worried about this trade off. SGTM.

@jtibshirani jtibshirani force-pushed the field-aliases-backport branch 2 times, most recently from ff00a58 to 7679011 Compare July 23, 2018 19:58
@jtibshirani
Copy link
Contributor Author

jtibshirani commented Jul 23, 2018

Great, thank you! I've removed the extra logic and added validation to ensure that the index has a single type. I went with an analogous approach to the one taken for the join field. I left a question about the approach, and would appreciate if someone could take a look.


NOTE: Field aliases can only be specified on indexes with a single mapping type. To add a field
alias, the index must therefore have been created in 6.0 or later, or be an older index with
the setting `index.mapping.single_type: true`. Please see <<removal-of-types>> for more information.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure whether we should accept this setting, or if we should strictly enforce that the index be created in 6.0 or later. I'm concerned that there will be confusing behavior in the following case: this setting is enabled, a field alias is added, then the setting is disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

This setting can't be disabled, so it should be ok.

* When loading stored fields, resolve wildcard patterns against the mappings instead of the index.
* Minor simplifications to the logic in FetchPhase.
* Pull out a shared method FetchPhase#getSearchFields.
* Allow for aliases when fetching stored fields.
* Make sure aliases can be referenced through params._fields.
* Make sure aliases can be accessed through 'doc'.
* Convert ExpressionTests to a unit test.
…ses. (elastic#31807)

* Add basic unit tests for field level security with field aliases.
* Ensure that field caps information is filtered when a field alias is provided.
@jtibshirani jtibshirani force-pushed the field-aliases-backport branch 4 times, most recently from 92f1a5a to badbbe4 Compare July 24, 2018 04:58
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.

Thanks, it looks good to me. I'm wondering whether only allowing on 6.x rather than 6.x plus single-type 5.x indices would be less confusing. For instance right now error messages mention that aliases can't be added because there are multiple types, which is not exact. The single_type setting is something that users opt in at index creation time that tells Elasticsearch to reject mapping updates that would create more than one type. But if a 5.x index was created without this setting, then IndexSettings#isSingleType will return false even if there is only one type in the index.


NOTE: Field aliases can only be specified on indexes with a single mapping type. To add a field
alias, the index must therefore have been created in 6.0 or later, or be an older index with
the setting `index.mapping.single_type: true`. Please see <<removal-of-types>> for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

This setting can't be disabled, so it should be ok.

@jtibshirani
Copy link
Contributor Author

@elasticmachine test this please

@jtibshirani
Copy link
Contributor Author

@jpountz I'm going to merge this, but will give it more thought and follow-up with a change that either checks only for 6.x indexes, or at least tightens the wording of error messages.

@jtibshirani jtibshirani merged commit 6672f53 into elastic:6.x Jul 24, 2018
@jtibshirani jtibshirani deleted the field-aliases-backport branch July 24, 2018 14:45
@jpountz
Copy link
Contributor

jpountz commented Jul 24, 2018

Sounds good to me. Thanks @jtibshirani !

dnhatn added a commit that referenced this pull request Jul 25, 2018
* 6.x:
  Security: revert to old way of merging automata (#32254)
  Fix a test bug in RangeQueryBuilderTests introduced in the field aliases backport.
  Introduce Application Privileges with support for Kibana RBAC (#32309)
  Undo a debugging change that snuck in during the field aliases merge.
  [test] port linux package packaging tests (#31943)
  Painless: Update More Methods to New Naming Scheme (#32305)
  Tribe: Add error with secure settings copied to tribe (#32298)
  Add V_6_3_3 version constant
  Add ERR to ranking evaluation documentation (#32314)
  [DOCS] Added link to 6.3.2 RNs
  [DOCS] Updates 6.3.2 release notes with PRs from ml-cpp repo (#32334)
  [Kerberos] Add Kerberos authentication support (#32263)
  [ML] Extract persistent task methods from MlMetadata (#32319)
  Backport - Add Snapshots Status API to High Level Rest Client (#32295)
  Make release notes ignore the `>test-failure` label. (#31309)
  [DOCS] Adds release highlights for search for 6.4 (#32095)
  Allow Integ Tests to run in a FIPS-140 JVM (#32316)
  Add support for field aliases to 6.x. (#32184)
  Register ERR metric with NamedXContentRegistry (#32320)
  fixes broken build for third-party-tests (#32315) Relates #31918 / Closes infra/issues/6085
  [DOCS] Rollup Caps API incorrectly mentions GET Jobs API (#32280)
  Rest HL client: Add put watch action (#32026) (#32191)
  Add WeightedAvg metric aggregation (#31037)
  Consistent encoder names (#29492)
  Switch monitoring to new style Requests (#32255)
  specify subdirs of lib, bin, modules in package (#32253)
  Rename ranking evaluation `quality_level` to `metric_score` (#32168)
  Add new permission for JDK11 to load JAAS libraries (#32132)
  Switch x-pack:core to new style Requests (#32252)
  Watcher: Store username on watch execution (#31873)
  Silence SSL reload test that fails on JDK 11
  Painless: Clean up add methods in PainlessLookup (#32258)
  CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction (#32185)
  Fail shard if IndexShard#storeStats runs into an IOException (#32241)
  Fix `range` queries on `_type` field for singe type indices (#31756) (#32161)
  AwaitsFix RecoveryIT#testHistoryUUIDIsGenerated
  Add new fields to monitoring template for Beats state (#32085) (#32273)
  [TEST] improve REST high-level client naming conventions check (#32244)
  Check that client methods match API defined in the REST spec (#31825)
@jtibshirani
Copy link
Contributor Author

An update: I opened #32482 to fix the error-message wording. I decided to stick with the current check for a couple reasons:

  • For simplicity, it’s nice to keep parity with the join field in terms of when the datatypes can be used.
  • It provides some flexibility in terms of being able to try out the feature, at very little cost in terms of code complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>feature >non-issue :Search Foundations/Mapping Index mappings, including merging and defining field types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants