Skip to content

Conversation

@martijnvg
Copy link
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
Collaborator

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

@elasticsearchmachine
Copy link
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
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
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
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
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
Member Author

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

Copy link
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

3 participants