Fixed geo point block loader slowness

[Kubik42]

This addresses #135891

[elasticsearchmachine]

Hi @Kubik42, I've created a changelog YAML for you.

[Kubik42]

The reason for failing tests is here. Whenever there is no FieldExtractPreference (ie. FieldExtractPreference.NONE, just like in the SDH), geo_points are mapped to BytesRef. This means that when the block loader sanity checker runs, it expects the underlying element type to be BytesRef. However, since we've now started to rely on doc_values, the type is actually LONG.

Perhaps the bigger problem here is that NONE is being assigned when DOC_VALUES should be the one being assigned. This would normally not be an issue since field types generally use the same underlying types for storing values, regardless of whether they're using doc_values vs stored fields. However, geo_points are special and the way we store them changes. More specifically, if doc_values are available, we store them as Longs and when the stored flag is toggled, we store them as BytesRef; although technically its Strings, but more on that later.

[Kubik42]

In my rabbit hole of understanding how BlockLoaders work, I found a somewhat of a bug thats in main. When a geo_point is specified with store = true and doc_values = false, we end up returning an imprecise result.

For example, the following command fails:

./gradlew ":x-pack:plugin:logsdb:yamlRestTest" --tests "org.elasticsearch.xpack.logsdb.LogsdbTestSuiteIT.test {yaml=/51_esql_synthetic_source/Simple from}" -Dtests.seed=76E814C77C112204 -Dtests.locale=xh -Dtests.timezone=Europe/Saratov -Druntime.java=25

with:

        Expected: "POINT (-74.00600004941225 40.712799984030426)"
             but: was "POINT (-74.006 40.7128)"
            at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
            at org.junit.Assert.assertThat(Assert.java:964)
            at org.junit.Assert.assertThat(Assert.java:930)
            at org.elasticsearch.test.rest.yaml.section.MatchAssertion.doAssert(MatchAssertion.java:100)
            at org.elasticsearch.test.rest.yaml.section.Assertion.execute(Assertion.java:68)
            at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.executeSection(ESClientYamlSuiteTestCase.java:522)
            ... 41 more

Based on what I've read in the code, this precision loss is expected. That being said, its not documented publicly.

[Kubik42]

There is also a problem with using BlockStoredFieldsReader.BytesFromStringsBlockLoader(name()) even when the field is stored. This is despite the fact that during indexing, the StoredField is created with a String representation of the geo_point; code.

When block loading, I get the error: Unknown geometry type: 825699888, which comes from here.

My guess is that it has something to do with the geo_point being stored as a String, rather than being converted to WKT first. That said, even when I used WellKnownText.toWKT() I still received a similar error - Unknown geometry type: 1414416719.

For now, I've just followed what we do in GeoShapeWithDocValuesFieldMapper and changed the block loader to BlockStoredFieldsReader.BytesFromBytesRefsBlockLoader, which has worked.

This needs further discussion.

[Kubik42]

I did a little more investigating and found that the reason we end up with FieldExtractPreference.NONE is because we initialize FieldExtractExec here, where both docValuesAttributes and boundsAttributes are empty sets. This leads to the following check failing, which ultimately returns defaultPreference, which is FieldExtractPreference.NONE.

This got me thinking, why are we setting the preference to NONE for attributes with doc_values? Shouldn't it be DOC_VALUES? If we think about it, NONE implies we can load the values however we want, so we're not restricted to using doc_values for that. This made me notice that inside this function, the attribute itself contains a DataType, which in turns exposes hasDocValues(). So, ultimately, wouldn't it make more sense to just do:

if (docValuesAttributes.contains(attr) || attr.dataType().hasDocValues()) {
    return MappedFieldType.FieldExtractPreference.DOC_VALUES;
}

However, doing so causes some type inequality exceptions.

Nhat pointed out this code, which suggests that doc_values shouldn't be always be used. However, that conflicts with the definition of FieldExtractPreference.NONE, which states that we can use anything.

[martijnvg]

I left a few comments.

[Kubik42]

Just adding what I've learned:

The issue is that BlockLoader.bytesRefsFromDocValues() is called differently for single doc Ordinals vs everything else. For everything else, the parameter expectedCount equals the number of documents, while for single docs, its the number of values within the first document.

Perhaps using BlockLoader.bytesRefsFromDocValues() is wrong here since I'm not actually loading BytesRefs from doc_values, but longs, that I then convert to BytesRefs. I will try using BlockLoader.bytesRefs() instead. Nothing in the javadoc screams at me that this is wrong, but at the same time, the javadoc for bytesRefsFromDocValues() isn't explicitly stating that bytesRefsFromDocValues() must be used either.

All of this can be avoided if we just stick to reading longs and not doing conversions. But to facilitate that, we need to be able to correctly set the FieldExtractPreference to be from doc_values whenever synthetic source is enabled and doc_values are available; and I suppose store = false. Afaik, we don't have this extra context when deciding on the preference.

[Kubik42]

Severless checks are failing bc AWS is experiencing an LSE

[martijnvg]

Great work 👍

[Kubik42]

💚 All backports created successfully

Status	Branch	Result
✅	9.2
✅	9.1
✅	8.19

Questions ?

Please refer to the Backport tool documentation