-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Rename evaluators for FIRST and LAST #132466
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
I'm going to be reusing the evaluators from FIRST_OVER_TIME and LAST_OVER_TIME for FIRST and LAST. This renames the evaluators themselves to indicate that they are the actual evaluators for FIRST and LAST. Relates to elastic#108385
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
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! Just one comment
| * This class is generated. Edit `X-ValueByLongAggregator.java.st` instead. | ||
| */ | ||
| @GroupingAggregator( | ||
| { @IntermediateState(name = "timestamps", type = "LONG_BLOCK"), @IntermediateState(name = "values", type = "$TYPE$_BLOCK") } |
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.
A bit of a nitpick: I think it might make sense to change the name from timestamps to something else if we're thinking to generalize this aggregation (is longs too generic?), it might be confusing later on otherwise
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.
It does feel pretty generic. I'm not sure what's better though.
In my bring working on LAST and FIRST I still did it for just DATETIME or DATE_NANOS - though it could work for any long.... I just didn't plug it in.
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.
Oh I see, in that case leaving it as timestamps might be fine. I think I misunderstood because removing the OverTime bit made me think it would eventually be done over any long
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 thought we'd do it over any long. Still could, I guess.
Maybe I just rename to FirstByTimestamp for now and if we make it any long I'll rename again. it's not hard.
|
OK! Updated. The rename is now pretty tiny. I still like it, but it's not critical. |
…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) ...
I'm going to be reusing the evaluators from FIRST_OVER_TIME and LAST_OVER_TIME for FIRST and LAST. This renames the evaluators themselves to indicate that they are the actual evaluators for FIRST and LAST.
Relates to #108385