handle empty object field

[luhhujbb]

In some indexes mapping, I've some field with this structure:

"myEmptyObjectField": {
"type": "object"
}

Since mapping is dynamic on these indexes, and object is empty elasticsearch cannot guess a type for sub fields, so it has "object" type. (ie empty object).

Because presto row type need at least one field, I just add a condition to drop the field, so that index is queryable.

[luhhujbb]

Same PR as #2209 but with right commit signature and test added.

[martint]

I don't think this is correct. According to Elasticsearch docs (https://www.elastic.co/guide/en/elasticsearch/reference/current/object.html):

You are not required to set the field type to object explicitly, as this is the default value.

This seems to imply that a mapping that contains fields can have an explicit type = "object", which means it would be excluded. Instead, we should check whether the field has type object and empty properties to skip it.

[luhhujbb]

I agree with the way you understand documentation, however in elasticsearch mapping implementation, we have :
https://github.com/elastic/elasticsearch/blob/d1334b812226acbe48fdae84bbf841c484d6e675/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java#L534

So when you put a mapping, the type object is optional, and when you read it, it is removed except when mapping of this object is empty.

Anyway, if you want I can add the condition on properties also, as a double check.

[martint]

Thanks for the pointers!

This whole section can now be simplified to make the checks more explicit and the flow easier to follow:

String type = "object";
if (value.has("type")) {
    type = value.get("type").asText();
}

switch (type) {
    case "date":
        ...
        break;
    case "object":
        if (value.has("properties")) {
            result.add(new IndexMetadata.Field(name, parseType(value.get("properties"))));
        }
        else {
            LOG.debug("Ignoring empty object field: %s", name);
        }
        break;
    default:
        result.add(new IndexMetadata.Field(name, new IndexMetadata.PrimitiveType(type)));
}

[luhhujbb]

Hi, many thanks for your review. I will implement this on Thursday, merry Christmas ! On mardi, 24 décembre, 2019 Martin Traverso <reply@reply.github.com> wrote: @martint commented on this pull request. In presto-elasticsearch/src/main/java/io/prestosql/elasticsearch/client/ElasticsearchClient.java:

@@ -456,9 +456,12 @@ public IndexMetadata getIndexMetadata(String index)

} result.add(new IndexMetadata.Field(name, new IndexMetadata.DateTimeType(formats))); } - else { + else if (!type.equals("object")) { Thanks for the pointers! This whole section can now be simplified to make the checks more explicit and the flow easier to follow: String type = "object"; if (value.has("type")) { type = value.get("type").asText(); } switch (type) { case "date": ... break; case "object": if (value.has("properties")) { result.add(new IndexMetadata.Field(name, parseType(value.get("properties")))); } else { LOG.debug("Ignoring empty object field: %s", name); } break; default: result.add(new IndexMetadata.Field(name, new IndexMetadata.PrimitiveType(type))); } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[martint]

Merged as c84e70f. Thanks for your contribution!