Elasticsearch: DocLookup has counter-intuitive behaviour

[jpountz]

DocLookup has several issues:

• containsKey only looks at the mappings regardless of the current document
• get throws an exception if the field is not mapped (should it return null instead?)
• get returns an empty list instead of null if the document has no values (while fielddata has the ability to know if a given field exists in a particular document)

[nik9000]

+1

[jdconrad]

@jpountz Are any of these concerns still relevant?

[rjernst]

I think this issue will be addressed by the work in https://github.com/elastic/elasticsearch/pull/29611

[mayya-sharipova]

@rjernst @jdconrad PR #29611 addresses only the 3rd point that Adrien raised here: "get returns an empty list instead of null..."

The 1st and 2nd points are still relevant.
In particular, for the 2nd point 500 is thrown for doc['missing_field']?.value

[nik9000]

The whole "throw an exception on unmapped field" behavior has grown on me somewhat. It is a little funky to because it isn't like any other map, but it is kind of useful to know that a field isn't mapped at all rather than if it doesn't have a value.

[mayya-sharipova]

Currently with PR #29611:

1. doc['mapped_field_no_value'] returns null
2. doc['unmapped_field'] throws 500.

I understand @nik9000 suggestion to distinguish between these cases, and don't return null for the 2nd case.

But I think it is incorrect to throw 500 Internal Server Error for the incorrect user action. For the unmapped field case, we can instead return 400 BAD_REQUEST or 404 NOT_FOUND

[rjernst]

doc['unmapped_field'] throws 500

Are you sure? It throws an IllegalArgumentException, which should map to a 400.

[mayya-sharipova]

@rjernst Thanks, Ryan. I see in the code IllegalArgumentException, but at the end a user will get 500 :

The following query:

GET  index-script/_search
 {
    "query" : {
            "match_all" : {} 
     },
     "script_fields" : {
         "fvalue" : {
             "script" : {
                 "source" : "doc['unmapped_field'].value"
             }
         }
       }
}

returns:

< HTTP/1.1 500 Internal Server Error
< content-type: application/json; charset=UTF-8

{
    "error": {
        "root_cause": [
            {
                "type": "script_exception",
                "reason": "runtime error",
                "script_stack": [
                    "org.elasticsearch.search.lookup.LeafDocLookup.get(LeafDocLookup.java:81)",
                    "org.elasticsearch.search.lookup.LeafDocLookup.get(LeafDocLookup.java:39)",
                    "doc['unmapped_field'].value",
                    "    ^---- HERE"
                ],
                "script": "doc['unmapped_field'].value",
                "lang": "painless"
            }
        ],
        "type": "search_phase_execution_exception",
        "reason": "all shards failed",
        "phase": "query",
        "grouped": true,
        "failed_shards": [
            {
                "shard": 0,
                "index": "index-script",
                "node": "mTG3MInFRD2Y4KnePrO-Xw",
                "reason": {
                    "type": "script_exception",
                    "reason": "runtime error",
                    "script_stack": [
                        "org.elasticsearch.search.lookup.LeafDocLookup.get(LeafDocLookup.java:81)",
                        "org.elasticsearch.search.lookup.LeafDocLookup.get(LeafDocLookup.java:39)",
                        "doc['unmapped_field'].value",
                        "    ^---- HERE"
                    ],
                    "script": "doc['unmapped_field'].value",
                    "lang": "painless",
                    "caused_by": {
                        "type": "illegal_argument_exception",
                        "reason": "No field found for [unmapped_field] in mapping with types []"
                    }
                }
            }
        ]
    },
    "status": 500
}

[jdconrad]

Seems like it should be a 400 for an IllegalArgumentException, maybe we should consider using an exception type that does output a 400 for these cases.

[rjernst]

The 500 is because SearchPhaseExecutionException assumes any shard failure is a 500. I think that needs to be (separately) fixed.

[stu-elastic]

containsKey only looks at the mappings regardless of the current document

If we don't have a mapping for the field, the current document can't have the field. We cannot get an accessor for the field otherwise.

get throws an exception if the field is not mapped (should it return null instead?)

This is desired behavior and allows user to differentiate between missing value and unmapped. It is not "mappy" behavior, agreed. We should change the type to something other than Map. See: https://github.com/elastic/elasticsearch/issues/51373

get returns an empty list instead of null if the document has no values (while fielddata has the ability to know if a given field exists in a particular document)

This is fixed by https://github.com/elastic/elasticsearch/pull/29611

doc['unmapped_field'] throws 500.

Pushed to https://github.com/elastic/elasticsearch/issues/51371