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

Refuse to load fields from _source when using the fields option and support wildcards. #15017

Merged
merged 1 commit into from
Nov 30, 2015
Merged

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Nov 25, 2015

Fixes #14489
Do not to load fields from _source when using the fields option.
Non stored (non existing) fields are ignored by the fields visitor when using the fields option.

Fixes #10783
Support * wildcard to retrieve stored fields when using the fields option.
Supported pattern styles are "xxx_", "_xxx", "xxx" and "xxx*yyy".

* fields by names or by patterns.
* Supported pattern styles "xxx*", "*xxx", "*xxx*" and "xxx*yyy".
*/
public class WildcardFieldsVisitor extends CustomFieldsVisitor {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we fold the functionality directly into CustomFieldsVisitor?

@jpountz
Copy link
Contributor

jpountz commented Nov 25, 2015

Thanks Jim, I'm happily surprised that there were not too many tests using the fields option to get fields from the _source! Could you add a note to docs/reference/migration/migrate_3_0.asciidoc to let users know that in 3.0 the fields option won't be able to load fields from _source anymore?

@jpountz jpountz self-assigned this Nov 25, 2015
@@ -87,7 +86,7 @@
body:
fields: [ include.field2, _source ]
query: { match_all: {} }
- match: { hits.hits.0.fields: { include.field2 : [v2] }}
- match: { hits.hits.0._source.include.field2: v2 }
- is_true: hits.hits.0._source
Copy link
Contributor

Choose a reason for hiding this comment

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

The point of the two above tests seems to be to test bw compat, I think we can remove them?

@jpountz
Copy link
Contributor

jpountz commented Nov 26, 2015

Thanks @jimferenczi this looks good to me. I just left a comment about two REST tests that I think we should remove. @bleskes would you mind having a quick look at this PR before it gets merged?

@bleskes
Copy link
Contributor

bleskes commented Nov 26, 2015

This looks good to me. Thanks @jimferenczi . My only question is whether we should resolve wild cards in the field visitor. Feels more natural for me to resolve it based on the mappings and get a concrete list to the visitor. I think we have the support we need in the MapperService (or will be easy to build). I'm worried that a wild card on the lucene level may expose system/hidden fields users didn't mean.. I don't think it's a problem in practice though. @jpountz wdyt?

}
}
if (loadAllStored) {
fieldsVisitor = new AllFieldsVisitor(); // load everything, including _source
Copy link
Contributor

Choose a reason for hiding this comment

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

is AllFieldsVisitor still used with your change? If not can you remove 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.

no it's not. I'll remove it.

@jpountz
Copy link
Contributor

jpountz commented Nov 27, 2015

@bleskes Good question. We already support fields=* in master (see mentions of loadAllStoredFields in the code) and I'm not aware of problems, so I think we should be fine.

@clintongormley clintongormley added >enhancement :Search/Search Search-related issues that do not fall into other categories labels Nov 28, 2015
@jpountz
Copy link
Contributor

jpountz commented Nov 30, 2015

@jimferenczi Let's merge this one?

  Do not to load fields from _source when using the `fields` option.
  Non stored (non existing) fields are ignored by the fields visitor when using the `fields` option.

Fixes #10783
  Support * wildcard to retrieve stored fields when using the `fields` option.
  Supported pattern styles are "xxx*", "*xxx", "*xxx*" and "xxx*yyy".
jimczi added a commit that referenced this pull request Nov 30, 2015
Refuse to load fields from _source when using the `fields` option and support wildcards.
@jimczi jimczi merged commit e182072 into elastic:master Nov 30, 2015
@jimczi jimczi deleted the fields_option branch November 30, 2015 10:01
@clintongormley
Copy link
Contributor

@jimferenczi please add version labels for each branch that you merged this PR into

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v5.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants