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 a GetFieldMapping API #3942

Closed
wants to merge 1 commit into from

Conversation

bleskes
Copy link
Contributor

@bleskes bleskes commented Oct 21, 2013

This new API allows to get the mapping for a specific set of fields rather than get the whole index mapping and traverse it.
The fields to be retrieved can be specified by their full path, index name and field name and will be resolved in this order.
In case multiple field match, the first one will be returned.

Since we are now generating the output (rather then fall back to the stored mapping), you can specify include_defaults=true on the request to have default values returned.

Closes #3941

public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.field("fullName", fullName);
builder.field("mapping");
builder.map(sourceAsMap());
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can write this field as "raw", see XContentHelper#rawField, so no need to convert it to a map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Required moving the source to a ByteReference which is ok imo.

@spinscale
Copy link
Contributor

I am in the same boat as luca with putting if (includeInDefaults) in front of everything inside of toXContent() in the mappers is adding up to complexity. I am in favor of having a toXContentEverything() and the old toXContent() methods or at least having sth like if (includeInDefaults) { } else { } in toXContent() - even at the price of duplicating code I think this makes it way more readable.

Apart from that: from functionality point of view this is a huge +1

@bleskes
Copy link
Contributor Author

bleskes commented Oct 23, 2013

@javanna @spinscale thx for the review. Going back to the point you made about potentially having a doXContentBodyWithDefaults() method that will remove the need for includeDefaults tests. I see your point but to me it's a choice between evils. The down side of such a method is that when we add something we need to remember to add it both places and the code doesn't remind us to (if you don't notice there is another method, you'd miss it). Same goes for bug fixes in existing code. Unless someone feels strongly about it, my choice goes to leave it as is.

@kimchy
Copy link
Member

kimchy commented Oct 23, 2013

on the fence on this as well, leaning towards the current solution, where the serialization of a single mapping attribute is done in a single place (compared to over several places).

@javanna
Copy link
Member

javanna commented Oct 23, 2013

I was more leaning towards a common method that does the if, as mentioned above, but there certainly are downsides to it as well (too many variations needed is the main one). I think I prefer those downsides anyway over the repeated if, but it's hard to tell without writing the code I guess... :)


private static final ToXContent.Params includeDefaultsParams = new ToXContent.Params() {

final String INCLUDE_DEFAULTS = "include_defaults";
Copy link
Member

Choose a reason for hiding this comment

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

private static too?

This new API allows to get the mapping for a specific set of fields rather than get the whole index mapping and traverse it.
The fields to be retrieved can be specified by their full path, index name and field name and will be resolved in this order.
In case multiple field match, the first one will be returned.

Since we are now generating the output (rather then fall back to the stored mapping), you can specify `include_defaults`=true on the request to have default values returned.

Closes elastic#3941
@bleskes bleskes closed this Oct 30, 2013
@bleskes bleskes deleted the feature/get_field_mapping branch November 22, 2013 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetFieldMapping API
5 participants