Skip to content

Conversation

jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Jun 26, 2018

This PR also fixes some issues uncovered by the tests, largely times when the wrong field name was being used to construct a Query. I went with unit tests, except for certain query types where integration tests were needed to verify the right behavior.

@jtibshirani jtibshirani added >test Issues or PRs that are addressing/adding tests :Search Foundations/Mapping Index mappings, including merging and defining field types labels Jun 26, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@jtibshirani jtibshirani force-pushed the field-alias-tests branch 5 times, most recently from f03c51c to 0cf17d4 Compare July 3, 2018 06:45
@jtibshirani jtibshirani changed the title Add more tests for field aliases in special query and aggregation types. Add more comprehensive tests for field aliases in queries + aggregations. Jul 3, 2018
@jtibshirani jtibshirani removed the WIP label Jul 4, 2018
@jtibshirani jtibshirani force-pushed the field-alias-tests branch 5 times, most recently from 25c1fbe to f11b855 Compare July 4, 2018 05:54
@jtibshirani jtibshirani requested a review from jpountz July 4, 2018 07:11
@jtibshirani jtibshirani force-pushed the field-alias-tests branch 2 times, most recently from 0e1b3a2 to fc38631 Compare July 4, 2018 18:38
@jtibshirani
Copy link
Contributor Author

@jpountz this is ready for review (but is not time-sensitive, since I'll be away for a week).

Copy link
Contributor Author

@jtibshirani jtibshirani Jul 4, 2018

Choose a reason for hiding this comment

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

I've been a bit concerned that this requirement (to fetch the name of the concrete field type) can be 'trappy' for other developers. I haven't been able to see a way to avoid this, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks ok to me

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks ok to me

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe call it newLegacyExistsQuery instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@jtibshirani jtibshirani merged this pull request into elastic:field-aliases Jul 17, 2018
@jtibshirani jtibshirani deleted the field-alias-tests branch July 17, 2018 19:49
jtibshirani added a commit that referenced this pull request Jul 17, 2018
…ons. (#31565)

* Make sure that significant terms aggregations work with field aliases.
* Add a test for ValuesSourceConfig.
* Allow for subclasses of AggregatorTestCase to provide field aliases.
* Add tests for nested and reverse_nested aggregations.
* Add an integration test for nested queries.
* Add an integration test for 'more like this' queries.
* Add tests for querying and loading meta-fields.
* Add unit tests for the relevant query builders.
* Add integration tests for geo polygon and shape queries.
* Fix static analysis violations.
* Document that aliases cannot be used in a query lookup path.
jtibshirani added a commit that referenced this pull request Jul 18, 2018
…ons. (#31565)

* Make sure that significant terms aggregations work with field aliases.
* Add a test for ValuesSourceConfig.
* Allow for subclasses of AggregatorTestCase to provide field aliases.
* Add tests for nested and reverse_nested aggregations.
* Add an integration test for nested queries.
* Add an integration test for 'more like this' queries.
* Add tests for querying and loading meta-fields.
* Add unit tests for the relevant query builders.
* Add integration tests for geo polygon and shape queries.
* Fix static analysis violations.
* Document that aliases cannot be used in a query lookup path.
jtibshirani added a commit that referenced this pull request Jul 18, 2018
* Add basic support for field aliases in index mappings. (#31287)
* Allow for aliases when fetching stored fields. (#31411)
* Add tests around accessing field aliases in scripts. (#31417)
* Add documentation around field aliases. (#31538)
* Add validation for field alias mappings. (#31518)
* Return both concrete fields and aliases in DocumentFieldMappers#getMapper. (#31671)
* Make sure that field-level security is enforced when using field aliases. (#31807)
* Add more comprehensive tests for field aliases in queries + aggregations. (#31565)
* Remove the deprecated method DocumentFieldMappers#getFieldMapper. (#32148)
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Jul 24, 2018
jtibshirani added a commit that referenced this pull request Jul 24, 2018
* Add basic support for field aliases in index mappings. (#31287)
* Allow for aliases when fetching stored fields. (#31411)
* Add tests around accessing field aliases in scripts. (#31417)
* Return both concrete fields and aliases in DocumentFieldMappers#getMapper. (#31671)
* Add documentation around field aliases. (#31538)
* Add validation for field alias mappings. (#31518)
* Make sure that field-level security is enforced when using field aliases. (#31807)
* Add more comprehensive tests for field aliases in queries + aggregations. (#31565)
* Remove the deprecated method DocumentFieldMappers#getFieldMapper. (#32148)
* Ensure that field aliases cannot be used in multi-fields. (#32219)
* Make sure that field aliases count towards the total fields limit. (#32222)
* Fix a test bug around nested aggregations and field aliases. (#32287)
* Make sure the _uid field is correctly loaded in scripts.
* Fix the failing test case FieldLevelSecurityTests#testParentChild_parentField.
* Enforce that field aliases can only be specified on indexes with a single type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search Foundations/Mapping Index mappings, including merging and defining field types >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants