-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Allow FIRST and LAST as method name #132469
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
Conversation
This modifies the ESQL grammar to allow FIRST and LAST as identifiers. We're going to make functions called that. Relates to elastic#108385
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Another option is to make |
|
The alternative is #132474 |
| : UNQUOTED_IDENTIFIER | ||
| | QUOTED_IDENTIFIER | ||
| | FIRST | ||
| | LAST |
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.
Is it worth restricting FIRST/LAST to only a function name opposed to any identifier? Although somebody might want to use | EVAL first=first(column, @timestamp). Not sure.
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.
Hey, I think we're adding this bit of grammar in the wrong place, see my comment below.
Additionally, the PR description mentions the syntax FIRST(v BY @timestamp). Having BY inside the function is also new grammar. Do we need this, or could we start with just FIRST(v, @timestamp) and decide later if/how functions should be extended to use the BY keyword in their arguments?
…o allow_function_named_first
|
This one's ready for another look, @alex-spies and @idegtiarenko . |
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.
LGTM!
|
Thanks @alex-spies and @idegtiarenko . |
| } | ||
|
|
||
| private String functionName(EsqlBaseParser.FunctionNameContext ctx) { | ||
| TerminalNode first = ctx.LAST(); |
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.
Looks like this should be
| TerminalNode first = ctx.LAST(); | |
| TerminalNode first = ctx.FIRST(); |
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.
yeah. #132503
I made a mistake in elastic#132469
…cking * upstream/main: (24 commits) Revert "[Fleet] add privileges to `kibana_system` to read integrations data (elastic#132400)" (elastic#132499) ESQL: Rename evaluators for FIRST and LAST (elastic#132466) Add inference fields to semantic text docs (elastic#132471) ESQL: Allow FIRST and LAST as method name (elastic#132469) ESQL: Add javadoc for PushDownAndCombineFilters (elastic#132484) Misc cleanups in Coordinator (elastic#132452) [DiskBBQ] Write the maximum posting list size to avoid resizing the docId array (elastic#132447) Improve exception handling for JsonXContentParser (elastic#123439) Clarify quantization on semantic_text BBQ dense vector default (elastic#132470) Fix test infra NPE in doEnsureClusterStateConsistency (elastic#131859) Stabilize CancellableTasksIT#testRemoveBanParentsOnDisconnect (elastic#131858) Move ClusterApplierService assertion after logging exception (elastic#132446) ESQL: Support for multi-argument aggs (elastic#132424) Update wolfi (versioned) (elastic#132457) ESQL: Fix Function javadoc (elastic#132399) [ML] Inference API disable partial search results (elastic#132362) Unmute testTermsQuery tests (elastic#132409) Fix index lookup when field-caps returns empty mapping (elastic#132138) CompressorFactory.compressor (elastic#132448) ESQL add formatting to plans in javadoc (elastic#132421) ...
This modifies the ESQL grammar to allow FIRST and LAST as identifiers. We're going to make functions called that.
Relates to #108385