Skip to content

Add tests around accessing field aliases in scripts.#31417

Merged
jtibshirani merged 3 commits intoelastic:field-aliasesfrom
jtibshirani:scripts
Jun 25, 2018
Merged

Add tests around accessing field aliases in scripts.#31417
jtibshirani merged 3 commits intoelastic:field-aliasesfrom
jtibshirani:scripts

Conversation

@jtibshirani
Copy link
Copy Markdown
Contributor

@jtibshirani jtibshirani commented Jun 19, 2018

No description provided.

@jtibshirani jtibshirani mentioned this pull request Jun 19, 2018
Closed
9 tasks
@rjernst
Copy link
Copy Markdown
Member

rjernst commented Jun 19, 2018

Can these be unit tests on LeafFieldLookup instead of integration tests?

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

elasticmachine commented Jun 19, 2018

Pinging @elastic/es-search-aggs

@jtibshirani
Copy link
Copy Markdown
Contributor Author

jtibshirani commented Jun 19, 2018

That makes sense -- I took a stab at converting them to unit tests. In addition to LeafFieldLookup, I needed to test LeafDocLookup (for accessing fields through doc[...] in painless scripts), and ExpressionScriptEngine, which seems to use its own loading pathway.

@jtibshirani jtibshirani requested a review from jpountz June 20, 2018 17:03
@jtibshirani jtibshirani force-pushed the field-aliases branch 2 times, most recently from b71de74 to b62c73a Compare June 21, 2018 23:54
jpountz
jpountz suggested changes Jun 25, 2018
View reviewed changes
Copy link
Copy Markdown
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.

Do we need to test expressions explicitly? If everything goes through LeafDocLookup, it shouldn't buy more than LeafDocLookupTests?

Not to be done in that PR, but I wonder that we might want to remove IndexFieldData.getName, which is one way that the info that an alias is queried rather than a concrete field is leaked, yet it doesn't seem much used.

@jtibshirani
Copy link
Copy Markdown
Contributor Author

jtibshirani commented Jun 25, 2018

I added the dedicated test for ExpressionScriptEngine, because it actually uses its own loading pathway outside of LeafDocLookup: https://github.com/elastic/elasticsearch/blob/master/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java#L194-L200. It would be better if they shared a common code path, but refactoring it to use LeafDocLookup looks fairly involved and probably shouldn't be tackled here.

And thanks for the pointer -- I'll take a closer look at IndexFieldData.

jpountz
jpountz approved these changes Jun 25, 2018
View reviewed changes
Copy link
Copy Markdown
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 for the explanation. LGTM

@jtibshirani jtibshirani merged commit 7947fd3 into elastic:field-aliases Jun 25, 2018
@jtibshirani jtibshirani deleted the scripts branch June 25, 2018 19:24
jtibshirani added a commit that referenced this pull request Jun 26, 2018
@jtibshirani
Add tests around accessing field aliases in scripts. (#31417)
d1393ff 
* Make sure aliases can be referenced through params._fields.
* Make sure aliases can be accessed through 'doc'.
* Convert ExpressionTests to a unit test.
@jtibshirani
Copy link
Copy Markdown
Contributor Author

jtibshirani commented Jun 28, 2018

Following up on IndexFieldData#getName -- on master, I only see IndexFieldData#getFieldName, should always correspond to the name of the concrete field. Hopefully I'm not missing something.

jtibshirani added a commit that referenced this pull request Jul 4, 2018
@jtibshirani
Add tests around accessing field aliases in scripts. (#31417)
d3813fc 
* Make sure aliases can be referenced through params._fields.
* Make sure aliases can be accessed through 'doc'.
* Convert ExpressionTests to a unit test.
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Jul 16, 2018
@jtibshirani
Add tests around accessing field aliases in scripts. (elastic#31417)
55a4f92 
* Make sure aliases can be referenced through params._fields.
* Make sure aliases can be accessed through 'doc'.
* Convert ExpressionTests to a unit test.
jtibshirani added a commit that referenced this pull request Jul 17, 2018
@jtibshirani
Add tests around accessing field aliases in scripts. (#31417)
1f21034 
* Make sure aliases can be referenced through params._fields.
* Make sure aliases can be accessed through 'doc'.
* Convert ExpressionTests to a unit test.
jtibshirani added a commit that referenced this pull request Jul 18, 2018
@jtibshirani
Add tests around accessing field aliases in scripts. (#31417)
dddb76b 
* Make sure aliases can be referenced through params._fields.
* Make sure aliases can be accessed through 'doc'.
* Convert ExpressionTests to a unit test.
jtibshirani added a commit that referenced this pull request Jul 18, 2018
@jtibshirani
Add support for field aliases. (#32172)
15ff3da 
* 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
Add tests around accessing field aliases in scripts. (elastic#31417)
01b4918 
* Make sure aliases can be referenced through params._fields.
* Make sure aliases can be accessed through 'doc'.
* Convert ExpressionTests to a unit test.
jtibshirani added a commit that referenced this pull request Jul 24, 2018
@jtibshirani
Add support for field aliases to 6.x. (#32184)
6672f53 
* 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

Reviewers

1 more reviewer

@jpountz jpountz jpountz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Search Foundations/Mapping Index mappings, including merging and defining field types >test Issues or PRs that are addressing/adding tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jtibshirani @rjernst @elasticmachine @jpountz @colings86