Use FallbackSyntheticSourceBlockLoader for point and geo_point#125816
Use FallbackSyntheticSourceBlockLoader for point and geo_point#125816lkts merged 11 commits intoelastic:mainfrom
Conversation
lkts
added
>enhancement
auto-backport
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Hi @lkts, I've created a changelog YAML for you. |
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
Thanks! This looks good, I just have two questions
- Maybe I missed something, but how is the
x-pack/plugin/logsdb/property-tests/.jqwik-databasefile being used? - The other one is on
GeoPointFieldMapper.
| return new BlockDocValuesReader.LongsBlockLoader(name()); | ||
| } | ||
|
|
||
| // _ignored_source field will only be present if there is no doc_values |
There was a problem hiding this comment.
I don't fully understand this, can someone not just use synthetic source and enable doc values? In that case there wouldn't be _source to fallback to?
There was a problem hiding this comment.
I am not sure i fully understand the question. This is needed due to the blContext.fieldExtractPreference() == DOC_VALUES check above. There are two scenarios possible once we arrive here:
- Stored source - we'll just use
blockLoaderFromSource - Synthetic source. However because of the
fieldExtractPreference()check above it is still possible that doc_values are present here. So we have two sub-cases:- doc_values are enabled -
_ignored_source_does not exist since we have doc_values and we useblockLoaderFromSourcewhich reads "classic" synthetic source. - doc_values are disabled - we know that
_ignored_sourcefield is present and use special block loader.
- doc_values are enabled -
I think the ideal scenario here is to implement yet another block loader that will directly read doc_values and transform them into a WKB format bypassing the synthetic source.
There was a problem hiding this comment.
Ok, cool, I get this now. Would you be able to add this comment to line 540?
It's not :) |
| if (syntheticSourceKeep.equals("all")) { | ||
| return exactValuesFromSource(values, nullValue); | ||
| } | ||
| if (syntheticSourceKeep.equals("arrays") && extractedFieldValues.documentHasObjectArrays()) { |
There was a problem hiding this comment.
This is an interesting case and i suspect it is not intentional.
There was a problem hiding this comment.
Can you elaborate why this isn't intentional?
There was a problem hiding this comment.
We don't apply custom logic of synthetic_source_keep: "arrays" to fields that have parsesArrayValue() set to true. But in this context we do.
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…125816) (#126079) * Use FallbackSyntheticSourceBlockLoader for point and geo_point (#125816) (cherry picked from commit f3ccde6) # Conflicts: # server/src/test/java/org/elasticsearch/index/mapper/blockloader/BooleanFieldBlockLoaderTests.java # test/framework/src/main/java/org/elasticsearch/index/mapper/BlockLoaderTestCase.java # test/framework/src/main/java/org/elasticsearch/index/mapper/NumberFieldBlockLoaderTestCase.java * compilation
4 participants
This PR introduces usage of
FallbackSyntheticSourceBlockLoaderforpointandgeo_pointfields and adds relevant tests.