Improve block loader for source only runtime fields of type long.#134117
Improve block loader for source only runtime fields of type long.#134117martijnvg merged 7 commits intoelastic:mainfrom
Conversation
By using the FallbackSyntheticSourceBlockLoader instead of generic LongScriptBlockLoader.
…block loading, but field data loading instead. In the latter case, the optimization that this test is testing will remain.
|
Assessed the performance impact with the following query: Flamegraph without this change: 
Flamegraph with this change: 
As can be seen, the overhead that comes with scripts/runtime fields is now gone. |
| var block = (TestBlock) columnReader.read(TestBlock.factory(), TestBlock.docs(leafReaderContext), 0, false); | ||
| for (int i = 0; i < block.size(); i++) { | ||
| assertThat(block.get(i), equalTo((long) i)); | ||
| // Test that script runtime field data works as expected with the optimization: |
There was a problem hiding this comment.
Switched to field data in this test, because with change long script block loader no longer relies on scripts and the optimization that this test tests.
| /** | ||
| * @return whether a field is only mapped as runtime field. A runtime and normal field can share the same name. | ||
| */ | ||
| public boolean onlyMappedAsRuntimeField(String fieldName) { |
There was a problem hiding this comment.
This is because a runtime field mapping and normal field mapping can have the same name. The search api's behavior is to always use the runtime field.
With this change we can't load from ignored source in this case. Because with synthetic source, the field value would be stored in either doc values or stored field of the mapped field. For now, we fall back to current script based block loader. I don't we need to optimize for the case where runtime and mapped fields share the names. I attached a test for this in RuntimeFieldSourceProviderOptimizationTests.
| SourceProvider sourceProvider | ||
| ) { | ||
| this(fieldTypeLookup, fieldDataLookup, sourceProvider, LeafFieldLookupProvider.fromStoredFields()); | ||
| this(fieldTypeLookup, fieldName -> false, fieldDataLookup, sourceProvider, LeafFieldLookupProvider.fromStoredFields()); |
There was a problem hiding this comment.
This is constructor is used only in tests and benchmarks.
|
Hi @martijnvg, I've created a changelog YAML for you. |
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
jordan-powers
left a comment
There was a problem hiding this comment.
LGTM, thanks Martijn
By using the FallbackSyntheticSourceBlockLoader instead of generic LongScriptBlockLoader.