Skip to content

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
/**
* 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