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 multi_field support for Mapper externalValue (plugins) #6867

Merged
merged 1 commit into from Jul 25, 2014

Conversation

Projects
None yet
4 participants
@dadoonet
Copy link
Member

commented Jul 15, 2014

In context of mapper attachment and other mapper plugins, when dealing with multi fields, sub fields never get the externalValue although it was set.

Related to elastic/elasticsearch-mapper-attachments#57

Closes #5402.

@dadoonet dadoonet self-assigned this Jul 15, 2014

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2014

The change looks good to me but I think it would be good to have another pair of eyes on it.

@dadoonet dadoonet added the bug label Jul 15, 2014

@s1monw s1monw added v1.4.0 and removed blocker labels Jul 15, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2014

sorry @dadoonet I left some comments on the commit here dadoonet@4542fa4
I didn't realize I am not on the PR :)

@s1monw s1monw removed the review label Jul 15, 2014

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2014

@s1monw PR updated with your comments. May I push?

@dadoonet dadoonet added the review label Jul 15, 2014

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2014

I think it's good too but this particular part of the mappers I think @kimchy should look at too please

public void reset(XContentParser parser, Document document, SourceToParse source, DocumentMapper.ParseListener listener) {
this.parser = parser;
this.document = document;
if (document != null) {

This comment has been minimized.

Copy link
@s1monw

s1monw Jul 23, 2014

Contributor

I wonder if we can in a second PR make the documents list final and just call clear on it?

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2014

I did another review and this LGTM feel free to push

@s1monw s1monw removed the review label Jul 23, 2014

Add multi_field support for Mapper externalValue (plugins)
In context of mapper attachment and other mapper plugins, when dealing with multi fields, sub fields never get the `externalValue` although it was set.

Here is a full script which reproduce the issue when used with mapper attachment plugin:

```
DELETE /test

PUT /test
{
    "mappings": {
        "test": {
            "properties": {
                "f": {
                    "type": "attachment",
                    "fields": {
                        "f": {
                            "analyzer": "english",
                            "fields": {
                                "no_stemming": {
                                    "type": "string",
                                    "store": "yes",
                                    "analyzer": "standard"
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

PUT /test/test/1
{
    "f": "VGhlIHF1aWNrIGJyb3duIGZveGVz"
}

GET /test/_search
{
    "query": {
        "match": {
           "f": "quick"
        }
    }
}

GET /test/_search
{
    "query": {
        "match": {
           "f.no_stemming": "quick"
        }
    }
}

GET /test/test/1?fields=f.no_stemming
```

Related to elastic/elasticsearch-mapper-attachments#57

Closes #5402.

@dadoonet dadoonet merged commit 11eced0 into elastic:master Jul 25, 2014

@dadoonet dadoonet deleted the dadoonet:issue/5402-externalvalue branch Jul 25, 2014

@dadoonet

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2014

Pushed in master and 1.x

dadoonet added a commit to elastic/elasticsearch-mapper-attachments that referenced this pull request Jul 25, 2014

Add support for multi-fields
Now elastic/elasticsearch#6867 is merged in elasticsearch core code (branch 1.x - es 1.4),
we can support multi fields in mapper attachment plugin.

```
DELETE /test
PUT /test
{
  "settings": {
    "number_of_shards": 1
  }
}
PUT /test/person/_mapping
{
  "person": {
    "properties": {
      "file": {
        "type": "attachment",
        "path": "full",
        "fields": {
          "file": {
            "type": "string",
            "fields": {
              "store": {
                "type": "string",
                "store": true
              }
            }
          },
          "content_type": {
            "type": "string",
            "fields": {
              "store": {
                "type": "string",
                "store": true
              },
              "untouched": {
                "type": "string",
                "index": "not_analyzed",
                "store": true
              }
            }
          }
        }
      }
    }
  }
}

PUT /test/person/1?refresh=true
{
  "file": "IkdvZCBTYXZlIHRoZSBRdWVlbiIgKGFsdGVybmF0aXZlbHkgIkdvZCBTYXZlIHRoZSBLaW5nIg=="
}

GET /test/person/_search
{
  "fields": [
    "file.store",
    "file.content_type.store"
  ],
  "aggs": {
    "store": {
      "terms": {
        "field": "file.content_type.store"
      }
    },
    "untouched": {
      "terms": {
        "field": "file.content_type.untouched"
      }
    }
  }
}
```

It gives:

```js
{
   "took": 3,
   "timed_out": false,
   "_shards": {
      "total": 1,
      "successful": 1,
      "failed": 0
   },
   "hits": {
      "total": 1,
      "max_score": 1,
      "hits": [
         {
            "_index": "test",
            "_type": "person",
            "_id": "1",
            "_score": 1,
            "fields": {
               "file.store": [
                  "\"God Save the Queen\" (alternatively \"God Save the King\"\n"
               ],
               "file.content_type.store": [
                  "text/plain; charset=ISO-8859-1"
               ]
            }
         }
      ]
   },
   "aggregations": {
      "store": {
         "doc_count_error_upper_bound": 0,
         "buckets": [
            {
               "key": "1",
               "doc_count": 1
            },
            {
               "key": "8859",
               "doc_count": 1
            },
            {
               "key": "charset",
               "doc_count": 1
            },
            {
               "key": "iso",
               "doc_count": 1
            },
            {
               "key": "plain",
               "doc_count": 1
            },
            {
               "key": "text",
               "doc_count": 1
            }
         ]
      },
      "untouched": {
         "doc_count_error_upper_bound": 0,
         "buckets": [
            {
               "key": "text/plain; charset=ISO-8859-1",
               "doc_count": 1
            }
         ]
      }
   }
}
```

Note that using shorter definition works as well:

```
DELETE /test
PUT /test
{
  "settings": {
    "number_of_shards": 1
  }
}
PUT /test/person/_mapping
{
  "person": {
    "properties": {
      "file": {
        "type": "attachment"
      }
    }
  }
}
PUT /test/person/1?refresh=true
{
  "file": "IkdvZCBTYXZlIHRoZSBRdWVlbiIgKGFsdGVybmF0aXZlbHkgIkdvZCBTYXZlIHRoZSBLaW5nIg=="
}

GET /test/person/_search
{
  "query": {
    "match": {
      "file": "king"
    }
  }
}
```

gives:

```js
{
   "took": 53,
   "timed_out": false,
   "_shards": {
      "total": 1,
      "successful": 1,
      "failed": 0
   },
   "hits": {
      "total": 1,
      "max_score": 0.095891505,
      "hits": [
         {
            "_index": "test",
            "_type": "person",
            "_id": "1",
            "_score": 0.095891505,
            "_source": {
               "file": "IkdvZCBTYXZlIHRoZSBRdWVlbiIgKGFsdGVybmF0aXZlbHkgIkdvZCBTYXZlIHRoZSBLaW5nIg=="
            }
         }
      ]
   }
}
```

Closes #57.

(cherry picked from commit 432d7c0)

dadoonet added a commit to elastic/elasticsearch-mapper-attachments that referenced this pull request Jul 25, 2014

Add support for multi-fields
Now elastic/elasticsearch#6867 is merged in elasticsearch core code (branch 1.x - es 1.4),
we can support multi fields in mapper attachment plugin.

```
DELETE /test
PUT /test
{
  "settings": {
    "number_of_shards": 1
  }
}
PUT /test/person/_mapping
{
  "person": {
    "properties": {
      "file": {
        "type": "attachment",
        "path": "full",
        "fields": {
          "file": {
            "type": "string",
            "fields": {
              "store": {
                "type": "string",
                "store": true
              }
            }
          },
          "content_type": {
            "type": "string",
            "fields": {
              "store": {
                "type": "string",
                "store": true
              },
              "untouched": {
                "type": "string",
                "index": "not_analyzed",
                "store": true
              }
            }
          }
        }
      }
    }
  }
}

PUT /test/person/1?refresh=true
{
  "file": "IkdvZCBTYXZlIHRoZSBRdWVlbiIgKGFsdGVybmF0aXZlbHkgIkdvZCBTYXZlIHRoZSBLaW5nIg=="
}

GET /test/person/_search
{
  "fields": [
    "file.store",
    "file.content_type.store"
  ],
  "aggs": {
    "store": {
      "terms": {
        "field": "file.content_type.store"
      }
    },
    "untouched": {
      "terms": {
        "field": "file.content_type.untouched"
      }
    }
  }
}
```

It gives:

```js
{
   "took": 3,
   "timed_out": false,
   "_shards": {
      "total": 1,
      "successful": 1,
      "failed": 0
   },
   "hits": {
      "total": 1,
      "max_score": 1,
      "hits": [
         {
            "_index": "test",
            "_type": "person",
            "_id": "1",
            "_score": 1,
            "fields": {
               "file.store": [
                  "\"God Save the Queen\" (alternatively \"God Save the King\"\n"
               ],
               "file.content_type.store": [
                  "text/plain; charset=ISO-8859-1"
               ]
            }
         }
      ]
   },
   "aggregations": {
      "store": {
         "doc_count_error_upper_bound": 0,
         "buckets": [
            {
               "key": "1",
               "doc_count": 1
            },
            {
               "key": "8859",
               "doc_count": 1
            },
            {
               "key": "charset",
               "doc_count": 1
            },
            {
               "key": "iso",
               "doc_count": 1
            },
            {
               "key": "plain",
               "doc_count": 1
            },
            {
               "key": "text",
               "doc_count": 1
            }
         ]
      },
      "untouched": {
         "doc_count_error_upper_bound": 0,
         "buckets": [
            {
               "key": "text/plain; charset=ISO-8859-1",
               "doc_count": 1
            }
         ]
      }
   }
}
```

Note that using shorter definition works as well:

```
DELETE /test
PUT /test
{
  "settings": {
    "number_of_shards": 1
  }
}
PUT /test/person/_mapping
{
  "person": {
    "properties": {
      "file": {
        "type": "attachment"
      }
    }
  }
}
PUT /test/person/1?refresh=true
{
  "file": "IkdvZCBTYXZlIHRoZSBRdWVlbiIgKGFsdGVybmF0aXZlbHkgIkdvZCBTYXZlIHRoZSBLaW5nIg=="
}

GET /test/person/_search
{
  "query": {
    "match": {
      "file": "king"
    }
  }
}
```

gives:

```js
{
   "took": 53,
   "timed_out": false,
   "_shards": {
      "total": 1,
      "successful": 1,
      "failed": 0
   },
   "hits": {
      "total": 1,
      "max_score": 0.095891505,
      "hits": [
         {
            "_index": "test",
            "_type": "person",
            "_id": "1",
            "_score": 0.095891505,
            "_source": {
               "file": "IkdvZCBTYXZlIHRoZSBRdWVlbiIgKGFsdGVybmF0aXZlbHkgIkdvZCBTYXZlIHRoZSBLaW5nIg=="
            }
         }
      ]
   }
}
```

Closes #57.

@dadoonet dadoonet changed the title Add multi_field support for Mapper externalValue (plugins) Mapping: add multi_field support for Mapper externalValue (plugins) Jul 29, 2014

@clintongormley clintongormley changed the title Mapping: add multi_field support for Mapper externalValue (plugins) Mapping: Add multi_field support for Mapper externalValue (plugins) Sep 8, 2014

@clintongormley clintongormley changed the title Mapping: Add multi_field support for Mapper externalValue (plugins) Add multi_field support for Mapper externalValue (plugins) Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.