-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ES|QL] Support replacement range #190465
[ES|QL] Support replacement range #190465
Conversation
/ci |
/ci |
@elasticmachine merge upstream |
/ci |
/ci |
Pinging @elastic/kibana-esql (Team:ESQL) |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
); | ||
|
||
/** | ||
* @TODO — this string manipulation is crude and can't support all cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make an issue for this todo to keep track or is that already existing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. There is no issue. But, using the AST more is a key tenant of my vision for a re-architecture of this code. In other words, I am tracking this but I think an RFC/architecture document is the best first step here. From that, we can create more specific issues.
testSuggestions('FROM a | WHERE doubleField IS N/', [ | ||
{ text: 'IS NOT NULL', rangeToReplace: { start: 28, end: 31 } }, | ||
{ text: 'IS NULL', rangeToReplace: { start: 28, end: 31 } }, | ||
{ text: '!= $0', rangeToReplace: { start: 31, end: 31 } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing this behavior in the editor with from kibana_sample_data_flights | where AvgTicketPrice is n
. Makes sense that it suggests IS NOT NULL
and IS NULL
where the cursor is but why the rest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a choice: make it so that the range only gets added for IS NOT NULL
and IS NULL
or just add it in a generic way for all.
I opted for the latter because
- including an empty range is valid and does no harm
- it saves us adding a special case
- if new operators with spaces are ever added, it will work automatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Thanks for the context!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and LGTM
Summary
Fix #187184
Field names
Before
Screen.Recording.2024-08-15.at.12.18.38.PM.mov
After
Screen.Recording.2024-08-15.at.12.18.47.PM.mov
Functions with spaces
Before
Screen.Recording.2024-08-15.at.12.20.51.PM.mov
After
Screen.Recording.2024-08-15.at.12.20.14.PM.mov
Checklist