New field type=alias including support for querying, aggs, suggestions + more read ops #30230
Conversation
|
Im going to guess because this is not a trivial change this review will potentially be quite involved, due to me not being accustomed with the elastic way of doing things etc. As a convenience in addition to the updated tests im including a fairly large list of operations to be used in kibana/curl to perform a quick smoke test of everything i "claim" has been done in my PR description. PUT /index-xyz
{
"mappings": {
"document": {
"properties": {
"integer1": {
"type": "integer"
},
"integer2": {
"type": "integer"
},
"alias3": {
"type": "alias",
"path": "integer2"
},
"text4" : {
"type": "text",
"fielddata": true,
"term_vector": "with_positions_offsets",
"store": true
},
"alias5": {
"type": "alias",
"path": "text4"
},
"text6" : {
"type": "text",
"fielddata": true,
"term_vector": "with_positions_offsets",
"store": true
}
}
}
}
}
PUT /index-xyz/document/1
{
"integer1": 11,
"integer2": 101,
"text4": "abc",
"text6": "qqq"
}
PUT /index-xyz/document/2
{
"integer1": 21,
"integer2": 201,
"text4": "def",
"text6": "rrr"
}
PUT /index-xyz/document/3
{
"integer1": 39,
"integer2": 309,
"text4": "abc",
"text6": "sss"
}
PUT /index-xyz/document/4
{
"text4": "abc",
"text6": "sss"
}
GET /index-xyz
GET /index-xyz/_search
GET index-xyz/_search
{
"query" : {
"term" : { "alias5" : "abc" }
}
}
GET /index-xyz/_search?q=alias5:abc
POST /index-xyz/_search?size=0
{
"aggs": {
"average-Integer1" : {
"avg": { "field": "integer2" }
}
}
}
POST /index-xyz/_search?
{
"aggs": {
"average-Integer1" : {
"avg": { "field": "alias3" }
}
}
}
POST /index-xyz/_search?
{
"aggs": {
"missing-Integer2" : {
"missing" : { "field" : "integer2" }
}
}
}
POST /index-xyz/_search?
{
"aggs": {
"missing-Integer2" : {
"missing" : { "field" : "alias3" }
}
}
}
POST /index-xyz/_search?
{
"aggs" : {
"integer2_ranges" : {
"range" : {
"field" : "integer2",
"ranges" : [
{ "from" : 1, "to" : 999 }
]
}
}
}
}
POST /index-xyz/_search?
{
"aggs" : {
"integer2_ranges" : {
"range" : {
"field" : "alias3",
"ranges" : [
{ "from" : 1, "to" : 999 }
]
}
}
}
}
POST /index-xyz/_search?size=0
{
"aggs" : {
"abc" : {
"filter" : {
"term": {
"text4": "abc"
}
},
"aggs" : {
"avg_integer2": {
"avg" : { "field" : "integer2" }
}
}
}
}
}
POST /index-xyz/_search?size=0
{
"aggs" : {
"abc" : {
"filter" : {
"term": {
"alias5": "abc"
}
},
"aggs" : {
"avg_alias3": {
"avg" : { "field" : "integer2" }
}
}
}
}
}
GET index-xyz/_search
{
"aggs" : {
"texttt" : {
"filters" : {
"filters" : {
"text44" : { "match" : { "text4" : "abc" }},
"text66" : { "match" : { "text6" : "qqq" }}
}
}
}
}
}
GET index-xyz/_search
{
"aggs" : {
"texttt" : {
"filters" : {
"filters" : {
"text44" : { "match" : { "alias5" : "abc" }}
}
}
}
}
}
GET /index-xyz/_search
{
"query" : {
"match_all": {}
},
"docvalue_fields" : ["integer2", "alias3", "text4"]
}
GET /index-xyz/_search
{
"query" : {
"match_all": {}
},
"docvalue_fields" : ["alias3"]
}
GET /index-xyz/_search
{
"query" : {
"match_all": {}
},
"stored_fields" : ["integer2", "alias3", "text4"]
}
GET /index-xyz/_search
{
"query" : {
"match_all": {}
},
"stored_fields" : ["text4111"]
}
GET /index-xyz/_search
{
"query" : {
"match_all": {}
},
"stored_fields" : ["alias5"]
}
GET /index-xyz/_search
{
"query" : {
"match_all": {}
},
"stored_fields" : 123
}
POST /index-xyz/_search
{
"suggest": {
"text4-suggest" : {
"text" : "abcd",
"term" : {
"field" : "text4"
}
},
"alias5-to-text4-suggest" : {
"text" : "abcd",
"term" : {
"field" : "alias5"
}
}
}
}
POST /index-xyz/_search
{
"suggest": {
"my-suggest-1" : {
"text" : "abcd",
"term" : {
"field" : "alias5"
}
}
}
}
GET /index-xyz/_search
{
"query": {
"match": {
"_all": "a"
}
},
"highlight": {
"fields": {
"text4": {
"require_field_match": false
}
}
}
}
GET /index-xyz/_search
{
"query": {
"match": {
"alias5": "abc"
}
},
"highlight": {
"fields": {
"text4": {
"require_field_match": false,
"type": "fvh"
}
}
}
}
GET /index-xyz/_search
{
"query": {
"match": {
"text4": "abc"
}
},
"highlight": {
"fields": {
"alias5": {
"require_field_match": false,
"type": "fvh"
}
}
}
}
GET /index-xyz/_search
{
"query": {
"match": {
"alias5": "abc"
}
},
"highlight": {
"fields": {
"text4": {
"require_field_match": false,
"type": "plain"
}
}
}
}
GET /index-xyz/_search
{
"query": {
"match": {
"text4": "abc"
}
},
"highlight": {
"fields": {
"alias5": {
"require_field_match": false,
"type": "plain"
}
}
}
}
GET /index-xyz/_search
{
"query": {
"match": {
"alias5": "abc"
}
},
"highlight": {
"fields": {
"text4": {
"require_field_match": false,
"type": "unified"
}
}
}
}
GET /index-xyz/_search
{
"query": {
"match": {
"text4": "abc"
}
},
"highlight": {
"fields": {
"alias5": {
"require_field_match": false,
"type": "unified"
}
}
}
} |
|
This comment will attempt to describe the "idea" in my approach to the support this new feature. Ive introduced a new alias field mapping and type. The mapping performs various checks at the mapping phase and reports errors to the user. The core of the functionality is the new AliasFieldType which knows holds both the alias details and alias target, and delegates most methods to the target. New methods were also defined on MappingFieldType, with defaults that use the this.name(), except for AliasFieldType which does the right thing for an alias. MappingFieldType.nameForIndex I have made changes to the system wherever it uses the "field" or "MappingFieldType" to build a lucene query, fetch a mapping and so on. MappingFieldType.nameForMessages. Thus if the user was attempting an index update for a field for an alias, this is an error, and we want to tell them that the alias field name along with the error message. As a convenience in AFT, i return both the alias and the target field. This would look something like this
The default for
Ive updated numerous places that build error messages removing the left/right bracket in their big string concat, and replaced I always try and pass the AliasFieldType around and resolve to the target as late as possible which enables me hopefully to keep both the alias and target for messages, building responses and doing index operations. By introducing the above and a few other methods i can let polymorphism do the right thing bet it an index or response or message. |
| @@ -111,7 +111,7 @@ public synchronized void clearField(final String fieldName) { | |||
|
|
|||
| @SuppressWarnings("unchecked") | |||
| public <IFD extends IndexFieldData<?>> IFD getForField(MappedFieldType fieldType, String fullyQualifiedIndexName) { | |||
| final String fieldName = fieldType.name(); | |||
| final String fieldName = fieldType.nameForIndex(); | |||
There was a problem hiding this comment.
this is an example where i always try and pass the real MappedFieldType even if its an AliasFieldType and let polymorphism pick the right "name" for the index operation.
| if (format != null) { | ||
| throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support custom formats"); | ||
| this.failUnsupportedFeature("custom formats"); |
There was a problem hiding this comment.
Quite a lot of places had the same "copy/paste" of this line so i extracted it into the a base class.
| * @param root The enclosing {@link RootObjectMapper} | ||
| */ | ||
| protected void rootObjectMapper(final RootObjectMapper root) { | ||
| } |
There was a problem hiding this comment.
This callback happens after all fields are processed and known to the context, and only then can we ask the alias to do its checks, does the target field exist etc.
| */ | ||
| public Mapper mapperForPath(final String path) { | ||
| return this.pathToMapper.get(path); | ||
| } |
There was a problem hiding this comment.
only really used by the alias to (validate) and fetch the target field its aliased too etc.
| package org.elasticsearch.index.mapper; | ||
|
|
||
| public class AliasFieldTypeBooleanFieldTypeTests extends BooleanFieldTypeTests { | ||
|
|
There was a problem hiding this comment.
basically for all XXXFieldType tests i wanted to repeat them but this time using an alias to the "XXXFieldType". This way i can leverage all the BooleanFieldTypeTests, but this time both via the boolean field name and an alias.
| @@ -24,6 +24,9 @@ | |||
| import org.junit.Before; | |||
|
|
|||
| public class BooleanFieldTypeTests extends FieldTypeTestCase { | |||
|
|
|||
| protected static final String BOOLEANFIELD = "booleanField1"; | |||
There was a problem hiding this comment.
the BFTT uses this so its clear the field is pointing to a boolean field. This is particular useful for AliasFieldTypeBooleanFieldTypeTests so we can clearly see in the mapping/error messages whether we are seeing the name of the boolean or the alias.
THis same "refactoring" has been applied to all XXXFieldTypeTests and the matching sub class that uses aliases.
| String date = "2015-10-12T14:10:55"; | ||
| long instant = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parser().parseDateTime(date).getMillis(); | ||
| ft.setIndexOptions(IndexOptions.DOCS); | ||
| Query expected = new IndexOrDocValuesQuery( | ||
| LongPoint.newRangeQuery("field", instant, instant + 999), |
There was a problem hiding this comment.
this is a horrible name, it doesnt mean anything especially when our tests get a bit more complicated than just a mapping with one field, eg like my new alias sub classes of this test.
| .endObject().endArray() | ||
| .endObject()); | ||
| public void testFieldName() throws Exception { | ||
| checkFieldNameAndValidation("root", "branch", "leaf"); |
There was a problem hiding this comment.
THis testsuite had a lot of copy paste code (horrible) and i wanted to repeat the same tests but with aliases. To avoid copying basically the entire class i extracted the common stuff so all these tests simply pass parameters (the different stuff).
|
|
||
| public abstract class FieldMapperTestCase extends ESSingleNodeTestCase { | ||
|
|
||
| public static String fieldPath(final String field, String...fields){ |
There was a problem hiding this comment.
THeres a lot of copy paste in all FieldMapperTests they really should be using a base class like this to avoid all that duplication and boilerplate noise.
| package org.elasticsearch.index.mapper; | ||
|
|
||
| public final class MapperTesting { | ||
|
|
There was a problem hiding this comment.
Introduced this so the ctor below can remain package private, but for the tests that need to create a AliasFieldType, they can call this "test only" factory.
| throw new IllegalArgumentException(pathStartOrEndingWithDotAmbiguous(makeFullPath(path, context))); | ||
| } | ||
|
|
||
| return new String[] {path}; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The pkg private static methods are used by some of my tests so they dont have to hard code literals in their assertions.
- #23714 - attempts to index an alias field fail. - introduced MappedFieldType.nameForMessages(), nameForIndex(), fieldTypeForIndex(), in the right places these are used rather than just a "raw" field or MappedFieldType.name() Goals and non goals. - Attempts to index into the alias field would result in an exception - it is read only - Queries - aggregations - "field" substituted with alias target. - "filter" fields substituted. - suggestions - scripts (using doc[]) (Not Done) - highlighting - fielddata_fields (deprecated/ Not Done) - docvalue_fields, - stored_fields would just get the data (and mapping) from the specified path - Source filtering would not work with the aliased field (Not done) - _source field when returned remains unmodified.
| @@ -135,7 +135,7 @@ public Query existsQuery(QueryShardContext context) { | |||
|
|
|||
| @Override | |||
| public Query termQuery(Object value, QueryShardContext context) { | |||
| throw new QueryShardException(context, "Murmur3 fields are not searchable: [" + name() + "]"); | |||
| throw new QueryShardException(context, "Murmur3 fields are not searchable: " + this.nameForMessages()); | |||
There was a problem hiding this comment.
this.nameForMessages auto includes the surround brackets. See other comments where i explain WHY this method was introduced.
| } | ||
|
|
||
| // intended to only be overridden by AliasFieldMapper | ||
| void init() |
There was a problem hiding this comment.
Introduced this method because AliasFieldType doesnt want to do the setTokenised and other defaults, so this is my way (via polymorphism) of stopping these defaults.
| return (IFD) indexFieldDataService.apply(fieldType, fullyQualifiedIndexName); | ||
| Objects.requireNonNull(fieldType, "fieldType is null"); | ||
|
|
||
| return (IFD) indexFieldDataService.apply(fieldType.fieldTypeForIndex(), fullyQualifiedIndexName); |
There was a problem hiding this comment.
if a non alias field type will simply return its name, for aliases it will return the name of the alias target, and the rest will just work!
| // we know its low level reader, and matching docId, since that's how we call the highlighter with | ||
| SourceLookup sourceLookup = searchContext.lookup().source(); | ||
| sourceLookup.setSegmentAndDocument((LeafReaderContext) reader.getContext(), docId); | ||
|
|
||
| List<Object> values = sourceLookup.extractRawValues(mapper.fieldType().name()); | ||
| final String fieldInIndex = mapper.fieldType().nameForIndex(); |
There was a problem hiding this comment.
does the right thing if an alias...
colings86
added
>feature
:Search Foundations/Mapping
|
Pinging @elastic/es-search-aggs |
mP1
changed the title
There was a problem hiding this comment.
Hi @mP1, thank you so much for your contribution! As you mentioned, this is quite a substantial feature.
I’m sorry it took so long to review. Before jumping into details, I have a few high-level comments about the PR that should be addressed:
- I agree that we should create something like an
AliasFieldMapperto parse the mappings for the alias type, but I don’t think it makes sense to have anAliasFieldType— instead aliases should be linked to an existing concreteMappedFieldType. Conceptually, there isn’t really such a thing as an ‘alias field type’ , and creating the classAliasFieldTypehas led to some complexity, like having to pull out aninitmethod, and introducing aMappedFieldType#fieldTypeForIndexmethod that consumers must remember to call. - The current way in which aliases are linked to their field types is a bit messy, with everything happening in the
RootObjectMapperconstructor. I also don’t think this approach lets you add a field mapping, then later add an alias to it in a separate mappings update. I think this linking needs to be pulled up closer to theMapperServicelevel. I did a quick prototype where I instead madeAliasFieldMapperextendMapperdirectly, and pulled up the aliasing logic intoFieldTypeLookup— this direction may be worth trying out. - I think that creating a
nameForMessagesvs.nameForIndexdistinction adds more complexity than it’s worth. Let’s stick with a single field typenamefor now, until we see a strong reason to hold onto the alias name in terms of debuggability, etc. - It would be good to add unit/ integration tests that cover all of the cases in your test script. We can likely add cases that focus on field aliases to existing tests. I don’t think the current set of tests you added covers important changes around suggestions, sorting, etc.
- The PR includes several refactors that aren’t core to the feature, like pulling out a
FieldMapperTestsbase class and the changes toHighlighterSearchIT.To keep this (already substantial) PR well-scoped and possible to review, it would be good to remove the non-core changes, and potentially save those refactors for a different PR.
The reason about AFM isnt about mappings, its about polymorphism to get the "right" field name when building queries, aggregations, error messages, processing results etc.
Sorry you are wrong, this entire ticket refers to alias field type as a concept.
No, thats called object orientated programming. Just like all the other field type sub classes, im creating a new sub class so i can hide custom behaviour based on the type.
The comment explains why this had to be done. The base class made some bad assumptions that arent always going to be true, such as introducing an AliasFieldType which doesnt want to clone those extra fields.
You are looking at this all wrong. You are forgetting that there are dozens of places that need to pick the right name when they work with the index, creating a query, aggregate etc. All those dozens of places no longer care if they have an alias reference, they just call nameForIndex() and it does the right thing regardless whether its an alias or not. Same goes for the dozens of places building error messages, they dont need to check, they call the nameForMessages() and they get the right answer and their error messages are correct from the users perspective. Any other solution is going to involve a lot more code, copy and pasted dozens of times. |
It works, my sample of kibana operations shows this.
Bit hard without any code for me to see to know exactly what you mean... I dont think you have looked hard at the problem of using the right "name" depending on whether the field is a simple field reference of an alias. If you followed it thru for all the use cases, its not as simple as what you think. |
|
Many good things in this PR, I like the proposed API and existing restrictions such as requiring concrete fields (no objects) that fields already exist in the mappings, failing documents that have a value for index fields, etc. There is one additional restriction that I would like to add which is to prevent source and targets of aliases to have different nested scopes. For instance it shouldn't be possible to alias a root field to the sub field of an object that uses a If someone creates mappings with an alias and then updates the mapping of the target field without re-submitting the alias field, will it be reflected on the alias field? I suspect not based on the diff, but maybe I missed the place where this is handled. I suspect we might need to introduce new cases to the merge logic of I understand your motivation to do proper OO programming with the field type wrapper, but I'm afraid this will cause issues given the number of places where we do I agree with @jtibshirani that we might want to stick with a single name for now, which I think should be the name of the target field. My reasoning is that things like queries and aggregations already know the name that was used to adress the field, which is usually a member of the
These two sentences look slightly aggressive. Written communication is hard, as the lack of tone to clarify your intent makes it easy for some statements to be misunderstood. Please feel free to state that your disagree, but in a way that targets a statement rather than someone (eg. |
One problem, the only example given by a ES team member, is exactly the opposite of what you say... The very first example given in the ticket seems to contradict what you ask...the "alias" is hanging off the root to a target field deep inside a nested objectg. clintongormley commented on 24 Mar 2017 We can introduce a new field type called alias which simply points to another field, eg: |
Will have to add tests to verify... Will do. |
Instance of tests in java code are a bad code smell, pretty sure its mentioned multiple times in Effective java etc. All those instanceof checks should be replaced by calls too methods isGeo(). Thats the proper OO way to know if something is a type. Can fix. |
Please read my previous response... The system needs to know both the alias AND the target field.... We want to return error messages with the alias name if the original request used the alias name, otherwise the user will be completely confused. If they do a search on a date field using an alias, and the format is invalid, how can the error message complaining about the format being invalid know what name to use in the text message if we dont pass it ? If we replace aliases with target very early on and work with that, our error messages will never have the alias when they need to be created. My code does the right thing, error messages use aliases if thats what was used for problem field. Same goes for aggregations, if we replace the alias with the target, our "answer" will not know the original alias because all it will have is the target. There are other examples. |
Sorry i was trying to keep this brief, as i had previously written up a screenful of text explaining the approach and the benefits it gives to this enhancement, and addressing the very subject of the comment. |
Unfortunately we need to wrap here, because we need to hold the alias and target field names for the reasons i mention in my large comment that accompanies the PR.
Exactly we need to enhance MFT to support alias fields. |
I don't think I disagree with Clinton. My point was only about objects that have a
I would agree in general, but sometimes things are a bit more complicated. This would require to add many more methods to
Maybe we can do differently. To reuse your example, let's assume a range query on a If
No worries. I read your explanations, I think there is just disagreement about trade-offs, eg. you seem to care about following oo programming principles, while I'm ok with not following them when it allows to keep APIs simpler. |
I will get back to these outstanding issues, im just trying to resolve the advantages of my introducing AliasFieldType, because the outcome of that changes many things. |
The code context at this stage has no way of knowing which field name appeared in the original query. Our error message is always going to be wrong half the time. In aggregation results its also going to be wrong because it doesn’t know the original field name, its “assuming” … because it doesn’t know. This is the very reason I introduced AliasFieldType so I don’t have both can I can pick the right “name” based on context, error messages, aggs, etc use FieldType.nameForMessages and “others” use FieldType.nameForIndex. There is never a “guess” or “mistake”, the code know exactly what the search query had etc. You can verify this by putting breakpoints at a few places where my code is calling FieldType.nameForMessage() and when the breakpoint hits, try the execute expression, there is no way of knowing whether the original query had the alias or the target for a field with both. Theres also another problem which leads to a LOT of changes.
The context object is not always available, please search my pull request for “nameForMessages” and check the method they exist. You will find a context is not present, and to “pass” on to that very spot would require many many changes along the code path. There are plenty more, maybe one or two are easy to fix, but there are STILL many that would require dozens ofchanges to pass the context in. |
|
Don't get me wrong, I understand your concerns and agree that displaying the right name may improve the user-experience. But on the other hand, this solution requires to add two new methods to MappedFieldType, and all implementations but one would just delegate to the name. This may look like a minor change, but we keep adding lots of features and if we had let them all add new APIs without questioning them, this software would be much harder to maintain than it is now. |
No its far more important than that, aggregations and other features that require the field name like highlights etc will ALL be wrong in the same ways i described above for the error message case.
I fail to see the problem of adding two methods, they actually read well and are self explaining. Javadoc could be added to summarise the problem they solve just to be sure. Im sorry but all the suggestions presented are far more confusing, and whats more important they are ALL broken for the use case i have finished explaining in my last comment. There is no other way to make this use case work.
So question away. However your approach doesnt work at all, and would require LOTS of changes to pass the context around and that is only the start. We pass A FieldType instance for all fields, so it makes sense to let the instance know both the what i call nameForMessages and nameForIndex. Theres nothing wrong with introducing a new type AliasFieldType and let polymorphism help is in giving answers about the field name for messages(and results like agg) and indexs (creating queries etc). I believe you guys are trying to hard to introduce AliasFieldType and thats just creating more and more hacks all the place and in the end its fundamentally broken. If we followed what you suggest as a foundation and try to make it work the diff will be at least 5x larger, with code scattered in many many places. My diff however is pretty simple all things considered. Basically the core of the code is in AliasFieldType, a few places use nameForMessage and nameForIndex and one or two other code blocks. I know there are a few small outstanding things ive skipped for a few comments, but they are relatively easy to fix, once my approach is accepted. |
|
I am aware of the points that you mentioned. To me the problem is that if the only way to implement this feature properly requires to add so much complexity to |
|
I wanted to clarify one of my original comments:
Here is a short reproduction which currently results in the error Ideally this sequence of commands would work, as put mappings requests should be allowed to reference other field mappings not in the request. One example where this pattern currently shows up is in field mappings with Finally, I'll plan to clean up the prototype I mentioned above and link it here so that we can get a better sense of our options + the associated trade-offs. |
|
Here is a prototype that explores the idea of creating a top-level |
|
Sounds good to me. Let's try to fix highligting to work on field types rather than mappers first maybe? Also how do we want aliases to interact with wildcards eventually? |
|
I will take a crack at the highlighter fix in a separate PR. Regardless of what happens with field aliases, moving highlighters to use I think that ideally we'd support matches on aliases in all places where field wildcards are supported (and aliases are supported). Both approaches proposed should support field wildcards in queries and field capabilities. Like I mentioned on the prototype PR, there is some trickiness around supporting wildcards when fetching fields -- I'll take a closer look at this as well. |
|
At this point, I think we should open a new PR with the alternative approach. @mP1 would you be interested in taking the approach in the prototype and trying to move it to completion (writing tests, adding mapping validation, etc.)? If not, I am happy to take that up. |
|
@jtibshirani I think it would be best if you complete the tests, given you know exactly from a conceptual viewpoint what needs to be done. |
|
Sounds good. Development has now been moved to the branch |
|
@jtibshirani |
in the right places these are used rather than just a "raw" field or MappedFieldType.name()
Goals and non goals.
Have you signed the contributor license agreement?
YES
Have you followed the contributor guidelines?
YES
If submitting code, have you built your formula locally prior to submission with
gradle check?YES
If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
YES
If submitting code, have you checked that your submission is for an OS that we support?
OSX(no platform dep code should work everywhere just fine)
If you are submitting this code for a class then read our policy for that.
N/A