Skip to content

Improve block loader for source only runtime fields of type double.#134629

Merged
martijnvg merged 5 commits intoelastic:mainfrom
martijnvg:block_loader_source_only_double_runtime_field
Sep 12, 2025
Merged

Improve block loader for source only runtime fields of type double.#134629
martijnvg merged 5 commits intoelastic:mainfrom
martijnvg:block_loader_source_only_double_runtime_field

Conversation

@martijnvg
Copy link
Copy Markdown
Member

By using the FallbackSyntheticSourceBlockLoader instead of generic DoubleScriptBlockLoader.

Relates to #134121

By using the FallbackSyntheticSourceBlockLoader instead of generic DoubleScriptBlockLoader.

Relates to elastic#134121
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

public BlockLoader blockLoader(BlockLoaderContext blContext) {
return new DoubleScriptBlockDocValuesReader.DoubleScriptBlockLoader(leafFactory(blContext.lookup()));
var indexSettings = blContext.indexSettings();
if (isParsedFromSource && indexSettings.getIndexMappingSourceMode() == SourceFieldMapper.Mode.SYNTHETIC
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how spotless formatted it :) I fixed the indentation, but since I copied from long script field type, I generalized the logic, it should be reusable for other runtime field types as well.

// In that case there is no ignored source entry, and so we need to fail back to LongScriptBlockLoader.
// We could optimize this, but at this stage feels like a rare scenario.
&& blContext.lookup().onlyMappedAsRuntimeField(name())) {
var reader = new NumberType.NumberFallbackSyntheticSourceReader(NumberType.DOUBLE, null, true) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for this code path?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is tested in the testBlockLoaderSourceOnlyRuntimeFieldWithSyntheticSource() test, which is also included in this change.

@martijnvg
Copy link
Copy Markdown
Member Author

Note that this change should yield similar improvement as: #134117 (comment)

Copy link
Copy Markdown
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@martijnvg martijnvg enabled auto-merge (squash) September 12, 2025 15:48
@martijnvg martijnvg merged commit 96402aa into elastic:main Sep 12, 2025
34 checks passed
mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Sep 17, 2025
…lastic#134629)

By using the FallbackSyntheticSourceBlockLoader instead of generic DoubleScriptBlockLoader.

Relates to elastic#134121
gmjehovich pushed a commit to gmjehovich/elasticsearch that referenced this pull request Sep 18, 2025
…lastic#134629)

By using the FallbackSyntheticSourceBlockLoader instead of generic DoubleScriptBlockLoader.

Relates to elastic#134121
phananh1010 added a commit to phananh1010/elasticsearch that referenced this pull request Oct 2, 2025
BASE=abd8b324ee1890ff50e34db656d9ed1d3f3e62d4
HEAD=45a5e24aa565598fb91e2933213d81e462ab103e
Branch=main
phananh1010 added a commit to phananh1010/elasticsearch that referenced this pull request Oct 6, 2025
BASE=abd8b324ee1890ff50e34db656d9ed1d3f3e62d4
HEAD=45a5e24aa565598fb91e2933213d81e462ab103e
Branch=main
phananh1010 added a commit to phananh1010/elasticsearch that referenced this pull request Oct 17, 2025
BASE=abd8b324ee1890ff50e34db656d9ed1d3f3e62d4
HEAD=45a5e24aa565598fb91e2933213d81e462ab103e
Branch=main
phananh1010 added a commit to phananh1010/elasticsearch that referenced this pull request Oct 23, 2025
BASE=abd8b324ee1890ff50e34db656d9ed1d3f3e62d4
HEAD=45a5e24aa565598fb91e2933213d81e462ab103e
Branch=main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants