-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Conversation
2103d3e
to
c95caa2
Compare
Pinging @elastic/es-search-aggs |
c95caa2
to
9d98d5e
Compare
@colings86 @jpountz this is ready for review. |
9d98d5e
to
6234916
Compare
There was a problem hiding this 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?
Personally I would be fine with that trade-off |
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. |
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. |
ff00a58
to
7679011
Compare
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 |
7679011
to
3221b5a
Compare
|
||
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3221b5a
to
213d95b
Compare
…#32287) This issue affected both NestedAggregatorTest and ReverseNestedAggregatorTest.
92f1a5a
to
badbbe4
Compare
There was a problem hiding this 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. |
There was a problem hiding this comment.
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.
badbbe4
to
0fe8493
Compare
@elasticmachine test this please |
@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. |
Sounds good to me. Thanks @jtibshirani ! |
* 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)
An update: I opened #32482 to fix the error-message wording. I decided to stick with the current check for a couple reasons:
|
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.