Skip to content

[ES|QL] Unify matching signature detection#234665

Merged
drewdaemon merged 7 commits intoelastic:mainfrom
drewdaemon:unify-matching-signature-detection
Sep 11, 2025
Merged

[ES|QL] Unify matching signature detection#234665
drewdaemon merged 7 commits intoelastic:mainfrom
drewdaemon:unify-matching-signature-detection

Conversation

@drewdaemon
Copy link
Contributor

@drewdaemon drewdaemon commented Sep 10, 2025

Summary

Reliably getting a list of call signatures that match a function's arguments is a key operation used in both

  • Detecting if the arguments are valid (validation)
  • Detecting the return type of a function (expression type detection)

I noticed that we had vastly improved this logic for validation in #230139, but getExpressionType was still using a separate, inferior implementation, leading to bugs related to incorrect expression type detection.

This PR extends the benefits of our improvements to expression type detection.

Example bug

Before

Screenshot 2025-09-10 at 11 15 54 AM

here MV_SLICE actually returns a keyword, but the expression evaluator says it returns a date

After

Screenshot 2025-09-10 at 11 55 28 AM

Checklist

@drewdaemon drewdaemon added enhancement New value added to drive a business result Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana t// release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting labels Sep 10, 2025
@drewdaemon drewdaemon marked this pull request as ready for review September 11, 2025 00:30
@drewdaemon drewdaemon requested a review from a team as a code owner September 11, 2025 00:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-esql (Team:ESQL)

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Looks good!

Comment on lines 217 to 221
if (expectedType === 'any') return true;

if (givenType === 'param') return true;

if (givenType === 'unknown') return acceptUnknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

Proposed change for readability:

if (expectedType === 'any' || givenType === 'param' || givenType === 'null') {
    return true;
  }
Copy link
Contributor

Choose a reason for hiding this comment

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

oh you just copied pasted it I see. It is a nit but it would need some readability improvements 😺

Copy link
Contributor Author

@drewdaemon drewdaemon Sep 11, 2025

Choose a reason for hiding this comment

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

lol actually this is what I had at first. I changed it so I could see code coverage reporting easier. Not a great reason to keep it that way. Will improve it!

Edit: I thought we were talking about different code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... I was too quick on the draw... I thought you were talking about different code... let me consider this suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I went with this in 0fe1b75

@drewdaemon drewdaemon enabled auto-merge (squash) September 11, 2025 13:28
@drewdaemon drewdaemon merged commit 53509ec into elastic:main Sep 11, 2025
12 checks passed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

KodeRad pushed a commit to KodeRad/kibana that referenced this pull request Sep 15, 2025
## Summary

Reliably getting a list of call signatures that match a function's
arguments is a key operation used in both
- Detecting if the arguments are valid (validation)
- Detecting the return type of a function (expression type detection)

I noticed that we had vastly improved this logic for validation in
elastic#230139, but `getExpressionType`
was still using a separate, inferior implementation, leading to bugs
related to incorrect expression type detection.

This PR extends the benefits of our improvements to expression type
detection.

### Example bug

#### Before

<img width="493" height="227" alt="Screenshot 2025-09-10 at 11 15 54 AM"
src="https://github.com/user-attachments/assets/69b4f544-be1f-4e8a-ba99-e3ace7d53e2b"
/>

_here `MV_SLICE` actually returns a keyword, but the expression
evaluator says it returns a `date`_

#### After
 
<img width="490" height="95" alt="Screenshot 2025-09-10 at 11 55 28 AM"
src="https://github.com/user-attachments/assets/1dac9a9e-a690-4e63-b166-3f468513cedc"
/>


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Sep 24, 2025
## Summary

Reliably getting a list of call signatures that match a function's
arguments is a key operation used in both
- Detecting if the arguments are valid (validation)
- Detecting the return type of a function (expression type detection)

I noticed that we had vastly improved this logic for validation in
elastic#230139, but `getExpressionType`
was still using a separate, inferior implementation, leading to bugs
related to incorrect expression type detection.

This PR extends the benefits of our improvements to expression type
detection.

### Example bug

#### Before

<img width="493" height="227" alt="Screenshot 2025-09-10 at 11 15 54 AM"
src="https://github.com/user-attachments/assets/69b4f544-be1f-4e8a-ba99-e3ace7d53e2b"
/>

_here `MV_SLICE` actually returns a keyword, but the expression
evaluator says it returns a `date`_

#### After
 
<img width="490" height="95" alt="Screenshot 2025-09-10 at 11 55 28 AM"
src="https://github.com/user-attachments/assets/1dac9a9e-a690-4e63-b166-3f468513cedc"
/>


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
niros1 pushed a commit that referenced this pull request Sep 30, 2025
## Summary

Reliably getting a list of call signatures that match a function's
arguments is a key operation used in both
- Detecting if the arguments are valid (validation)
- Detecting the return type of a function (expression type detection)

I noticed that we had vastly improved this logic for validation in
#230139, but `getExpressionType`
was still using a separate, inferior implementation, leading to bugs
related to incorrect expression type detection.

This PR extends the benefits of our improvements to expression type
detection.

### Example bug

#### Before

<img width="493" height="227" alt="Screenshot 2025-09-10 at 11 15 54 AM"
src="https://github.com/user-attachments/assets/69b4f544-be1f-4e8a-ba99-e3ace7d53e2b"
/>

_here `MV_SLICE` actually returns a keyword, but the expression
evaluator says it returns a `date`_

#### After
 
<img width="490" height="95" alt="Screenshot 2025-09-10 at 11 55 28 AM"
src="https://github.com/user-attachments/assets/1dac9a9e-a690-4e63-b166-3f468513cedc"
/>


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
@drewdaemon drewdaemon deleted the unify-matching-signature-detection branch October 7, 2025 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting enhancement New value added to drive a business result Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:ESQL ES|QL related features in Kibana t// v9.2.0

4 participants