Skip to content

ESQL: Skip lookup fields when eliminating missing fields#118658

Merged
alex-spies merged 10 commits intoelastic:mainfrom
alex-spies:lookup-join-where-on-lookup-fields
Dec 17, 2024
Merged

ESQL: Skip lookup fields when eliminating missing fields#118658
alex-spies merged 10 commits intoelastic:mainfrom
alex-spies:lookup-join-where-on-lookup-fields

Conversation

@alex-spies
Copy link
Contributor

@alex-spies alex-spies commented Dec 13, 2024

We do not have SearchStats for fields from lookup indices. And unfortunately, these are hard to obtain.

For now, just do not apply ReplaceMissingFieldWithNull to fields coming from an index used in LOOKUP JOIN. These are identified via their indexmode.

/**
* LOOKUP JOIN
*/
JOIN_LOOKUP_V5(Build.current().isSnapshot()),
Copy link
Contributor Author

@alex-spies alex-spies Dec 13, 2024

Choose a reason for hiding this comment

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

To be merged after #118429, so I bumped the capability by 2 right away.

@alex-spies alex-spies added >non-issue auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.18.0 labels Dec 13, 2024
@alex-spies alex-spies marked this pull request as ready for review December 13, 2024 17:19
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 13, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@alex-spies
Copy link
Contributor Author

I contemplated adding also optimizer (unit) tests; not sure we want them, as those would have to test that a rule does nothing - which is not entirely correct, it just can't do anything at the moment due to missing SearchStats.

3 | Error
;

lookupWithFieldOnJoinKey-Ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has become filterOnJoinKeyAndRightSide, one test case below.

Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM

@alex-spies alex-spies added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 17, 2024
@alex-spies
Copy link
Contributor Author

CI only failed on JdbcDocCsvSpecIT » test {docs.testFilterToday}, which is in the process of being fixed - so this should be fine to merge.

@alex-spies alex-spies merged commit 8134c79 into elastic:main Dec 17, 2024
@alex-spies alex-spies deleted the lookup-join-where-on-lookup-fields branch December 17, 2024 12:05
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

alex-spies added a commit to alex-spies/elasticsearch that referenced this pull request Dec 17, 2024
)

We do not have SearchStats for fields from lookup indices. And unfortunately, these are hard to obtain.

For now, just do not apply ReplaceMissingFieldWithNull to fields coming from an index used in LOOKUP JOIN. These are identified via their indexmode.
elasticsearchmachine pushed a commit that referenced this pull request Dec 17, 2024
…118836)

We do not have SearchStats for fields from lookup indices. And unfortunately, these are hard to obtain.

For now, just do not apply ReplaceMissingFieldWithNull to fields coming from an index used in LOOKUP JOIN. These are identified via their indexmode.
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 auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants