Skip to content

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Aug 6, 2025

Actually a few things:

  1. Add generation for ungroups FIRST/LAST
  2. Change the description of FIRST/LAST to be more readable
  3. Allow that description to be specified instead of generated
  4. Add grouped and ungrouped tests for FIRST/LAST
  5. Grouped tests weren't passing with null groups. Always enabled group id tracking to fix it.
  6. Reworked how grouped aggs trigger group id tracking for all grouped aggs to make them a little more consistent.

Relates to #108385

Actually a few things:

1. Add generation for ungroups FIRST/LAST
2. Change the description of FIRST/LAST to be more readable
3. Allow that description to be specified instead of generated
4. Add grouped and ungrouped tests for FIRST/LAST
5. Grouped tests weren't passing with `null` groups. Always enabled
   group id tracking to fix it.
6. Reworked how grouped aggs trigger group id tracking for all grouped
   aggs to make them a little more consistent.

Relates to elastic#108385
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 6, 2025
@elasticsearchmachine
Copy link
Collaborator

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

{
var properties = [:]
v1.forEach { k, v -> properties["v1_" + k] = v}
v2.forEach { k, v -> properties["v2_" + k] = v}
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand above. Would we have only a single v1 and v2 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a comment. we only need LongDouble, LongInt, LongLong, and LongFloat for the implementations we have now. I can drop LongBool - at least until we add more users.

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Nik!

import java.util.Set;
import java.util.stream.IntStream;

public class FirstLongByTimestampGroupingAggregatorFunctionTests extends GroupingAggregatorFunctionTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for adding these tests :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It found a bug! A bug that doesn't exist in TSDB, so it can't matter to anyone using it so far!

Copy link
Contributor

@limotova limotova left a comment

Choose a reason for hiding this comment

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

LGTM!

@nik9000 nik9000 merged commit 686a11a into elastic:main Aug 8, 2025
33 checks passed
@nik9000
Copy link
Member Author

nik9000 commented Aug 8, 2025

Thanks for the reviews friends!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants