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
Storing metadata information in GetResult #38373
Changes from 1 commit
e657574
3f0bca8
3233957
314c58a
2abfc80
5d80874
5f52b9e
6146d71
9f3308e
e1f42c6
748b8fc
f4fabd1
c4d2cdf
fa60aa0
e1e871e
3d2109a
ed39246
69db08e
c7335c2
94c6206
d9b2639
0550d7d
75e2519
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,6 @@ | |
import org.elasticsearch.common.xcontent.XContentBuilder; | ||
import org.elasticsearch.common.xcontent.XContentParser; | ||
import org.elasticsearch.index.get.GetResult; | ||
import org.elasticsearch.index.mapper.MapperService; | ||
import org.elasticsearch.search.SearchHit; | ||
|
||
import java.io.IOException; | ||
|
@@ -47,14 +46,16 @@ | |
public class DocumentField implements Streamable, ToXContentFragment, Iterable<Object> { | ||
|
||
private String name; | ||
private Boolean isMetadataField; | ||
private List<Object> values; | ||
|
||
private DocumentField() { | ||
} | ||
|
||
public DocumentField(String name, List<Object> values) { | ||
public DocumentField(String name, List<Object> values, boolean isMetadataField) { | ||
this.name = Objects.requireNonNull(name, "name must not be null"); | ||
this.values = Objects.requireNonNull(values, "values must not be null"); | ||
this.isMetadataField = isMetadataField; | ||
} | ||
|
||
/** | ||
|
@@ -85,7 +86,7 @@ public List<Object> getValues() { | |
* @return The field is a metadata field | ||
*/ | ||
public boolean isMetadataField() { | ||
return MapperService.isMetadataField(name); | ||
return this.isMetadataField; | ||
} | ||
|
||
@Override | ||
|
@@ -132,7 +133,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws | |
return builder; | ||
} | ||
|
||
public static DocumentField fromXContent(XContentParser parser) throws IOException { | ||
public static DocumentField fromXContent(XContentParser parser, boolean inMetadataArea) throws IOException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this parameter naming follows the intent to emphasize and make it explicit that we decide whether the field is metadata or not depending on whether it is located in metadata area of xcontent or not. I don't really have a strong opinion here, we can replace it with isMetadataField and add some comments, if you prefer that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for the explanation, |
||
ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.currentToken(), parser::getTokenLocation); | ||
String fieldName = parser.currentName(); | ||
XContentParser.Token token = parser.nextToken(); | ||
|
@@ -141,7 +142,7 @@ public static DocumentField fromXContent(XContentParser parser) throws IOExcepti | |
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { | ||
values.add(parseFieldsValue(parser)); | ||
} | ||
return new DocumentField(fieldName, values); | ||
return new DocumentField(fieldName, values, inMetadataArea); | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -338,7 +338,9 @@ public static GetResult fromXContentEmbedded(XContentParser parser, String index | |
} else if (FOUND.equals(currentFieldName)) { | ||
found = parser.booleanValue(); | ||
} else { | ||
fields.put(currentFieldName, new DocumentField(currentFieldName, Collections.singletonList(parser.objectText()))); | ||
// This fields is in metadata area of the xContent, thus should be treated as metadata | ||
fields.put(currentFieldName, new DocumentField(currentFieldName, | ||
Collections.singletonList(parser.objectText()), true)); | ||
} | ||
} else if (token == XContentParser.Token.START_OBJECT) { | ||
if (SourceFieldMapper.NAME.equals(currentFieldName)) { | ||
|
@@ -350,15 +352,15 @@ public static GetResult fromXContentEmbedded(XContentParser parser, String index | |
} | ||
} else if (FIELDS.equals(currentFieldName)) { | ||
while(parser.nextToken() != XContentParser.Token.END_OBJECT) { | ||
DocumentField getField = DocumentField.fromXContent(parser); | ||
DocumentField getField = DocumentField.fromXContent(parser, false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rjernst This DocumentField is not inside of |
||
fields.put(getField.getName(), getField); | ||
} | ||
} else { | ||
parser.skipChildren(); // skip potential inner objects for forward compatibility | ||
} | ||
} else if (token == XContentParser.Token.START_ARRAY) { | ||
if (IgnoredFieldMapper.NAME.equals(currentFieldName)) { | ||
fields.put(currentFieldName, new DocumentField(currentFieldName, parser.list())); | ||
fields.put(currentFieldName, new DocumentField(currentFieldName, parser.list(), false)); | ||
} else { | ||
parser.skipChildren(); // skip potential inner arrays for forward compatibility | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,7 +202,8 @@ private GetResult innerGetLoadFromStoredFields(String type, String id, String[] | |
fieldVisitor.postProcess(mapperService); | ||
fields = new HashMap<>(fieldVisitor.fields().size()); | ||
for (Map.Entry<String, List<Object>> entry : fieldVisitor.fields().entrySet()) { | ||
fields.put(entry.getKey(), new DocumentField(entry.getKey(), entry.getValue())); | ||
boolean isMetadataField = MapperService.isMetadataField(entry.getKey()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
fields.put(entry.getKey(), new DocumentField(entry.getKey(), entry.getValue(), isMetadataField)); | ||
} | ||
} | ||
} | ||
|
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.
can this be
final
? If you are adding a new member variable, you should modify all corresponding methods to incorporate it, methods such asreadFrom
,writeTo
,equals
,hashCode
,toString
. ForreadFrom
andwriteTo
the serialization should be a version dependent (an example here)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.
We need to think more how to organize
DocumentField
class. UsuallyfromXContent -> toXContent ->fromXContent
should produce an equal object (and this is what we test inDocumentFieldTests
). The way how you organized aDocumentField
class, it doesn't happen astoXContent
is usingisMetadataField
, andfromXContent
is not using it.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.
if it requires a lot of changes to use
isMetadataField
intoXContent
(we need to investigate this first), then we should document why we are not including it intoXContent
, and also exclude this field fromequals
,hashCode
- also documenting whyisMetadataField
is not participating in these functionsThere 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 for the review!
Regarding consistency of
fromXContent -> toXContent ->fromXContent
we had a relevant discussion in the issue description. There was a suggestion of using different json structures depending when serializing from new versions, while still being able to deserialize json from old versions, as suggested in #24422 (comment) . In such case it would have been possible to have enough information in the json to be able to make the conversions consistent without context. Yet there was a strong argument to avoid as mentioned here #24422 (comment)as changing such json structure may result in issues when serializing it in one version and parsing in another.
In short, looks like it is not a lot of changes, but a bwc concern that prevents us from storing the
isMetadataField
intoXContent
. It would be great if you share your thoughts on the matter.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.
here we need to make a trade-off between a)extending serialized json schema for document field, additional coding to guarantee backward compatibility for serialized content and b)guessing the meaning of json fields depending on the context, which may potentially introduce some bugs if our assumptions are incorrect in some case, e.g. it is not completely obvious if it is a right approach here https://github.com/elastic/elasticsearch/pull/38373/files/314c58a64e5c4f2fb3fc9ec6e3366a0209597dd5#r253675646
what is the process for deciding between
a
andb
here?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.
@sandmannn Thanks for clarification. Ok, lets leave your code for json serialization for
DocumentField
at it is now, but just put comments whyisMetadataField
is a special field, why it is not participating in equals, hashCode, toString, andtoXContent
.You still need to modify
readFrom
andwriteTo
methods to include this field depending on the version.