Encode field attributes as predicates

[not-napoleon]

Currently, the aggregations framework uses instanceof operations in many places to figure out what type of values we are getting. For example:

        if (indexFieldData instanceof IndexOrdinalsFieldData) {
            dataSource = new ValuesSource.Bytes.WithOrdinals.FieldData((IndexOrdinalsFieldData) indexFieldData);
        } else {
            dataSource = new ValuesSource.Bytes.FieldData(indexFieldData);
        }

(

elasticsearch/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java

Lines 353 to 357 in 4a66cfc

	if (indexFieldData instanceof IndexOrdinalsFieldData) {
	dataSource = new ValuesSource.Bytes.WithOrdinals.FieldData((IndexOrdinalsFieldData) indexFieldData);
	} else {
	dataSource = new ValuesSource.Bytes.FieldData(indexFieldData);
	}

)

As part of our effort to get away from hard coding type information in the core aggregations framework, we'd like to explore replacing these with predicates on the Field Data classes. For example, the above case could use a hasOrdinals() predicate instead of the instanceof check.

[elasticmachine]

Pinging @elastic/es-search (:Search/Mapping)

[elasticmachine]

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

[iverase]

@not-napoleon This seems to have been mostly dealt with the value source refactoring, does it make sense to have it opened or we can close it now?

[not-napoleon]

@iverase I didn't actually address either the specific case quoted in this ticket, or the general behavior, when I did the refactor. I just moved where we do it. The code block quoted above is now found in CoreValuesSourceType: https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java#L119-L123

So, I vote to keep this open for now.

[nik9000]

I think I just merged a hasOrdinals method. We don't use it in all the places though.

[nik9000]

Ah, but that this instanceof is what gets us to my hasOrdinals in the first place.

[not-napoleon]

Yeah, we've made a lot of progress getting rid of instanceofs around the various ValuesSource classes. This ticket is more about the level above that, where we're selecting which ValuesSource subclass to build based on the IndexFieldData &c.

[GrayFox1]

I am interested to pick this Issue, can i start to work on this?

[not-napoleon]

@GrayFox1 This is a pretty tricky and involved refactoring that touches a lot of the code. I would not suggest this as a starting point for contributing to Elasticsearch. If you're interested in getting some experience contributing to projects, might I suggest taking a look at our good first issue list: https://github.com/elastic/elasticsearch/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22