Skip to content

Correct the condition to use a multi field block loader in text field block loader #126718

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 22, 2025

Conversation

lkts
Copy link
Contributor

@lkts lkts commented Apr 11, 2025

Current condition of syntheticSourceDelegate.isIndexed() || syntheticSourceDelegate.isStored() seems to be stale. isStored() is always true for syntheticSourceDelegate and isIndexed() is actually not needed for keyword block loader to work. This PR removes excessive conditions.

@lkts lkts added >non-issue :Analytics/ES|QL AKA ESQL :StorageEngine/Mapping The storage related side of mappings labels Apr 11, 2025
@lkts lkts requested review from craigtaverner and nik9000 April 11, 2025 18:17
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:StorageEngine v9.1.0 labels Apr 11, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

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

@lkts lkts added auto-backport Automatically create backport pull requests when merged v8.19.0 labels Apr 11, 2025
@@ -967,12 +967,11 @@ public boolean isAggregatable() {
}

/**
* Returns true if the delegate sub-field can be used for loading and querying (ie. either isIndexed or isStored is true)
* Returns true if the delegate sub-field can be used for loading.
* A delegate by definition must have doc_values or be stored so most of the time it can be used for loading.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* A delegate by definition must have doc_values or be stored so most of the time it can be used for loading.
* A delegate by definition must have doc_values or be stored so most of the time it can be used for querying.

I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There already is canUseSyntheticSourceDelegateForQuerying() so "loading" distinguishes the two.

Copy link
Member

@dnhatn dnhatn 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 Sasha!

@lkts lkts merged commit 093ebd3 into elastic:main Apr 22, 2025
17 checks passed
@lkts lkts deleted the fix_text_block_loader_delegate branch April 22, 2025 16:01
lkts added a commit to lkts/elasticsearch that referenced this pull request Apr 22, 2025
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >non-issue :StorageEngine/Mapping The storage related side of mappings Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:StorageEngine v8.19.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants