Skip to content

Fix empty VALUES with ordinals grouping #130861

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

Merged
merged 1 commit into from
Jul 8, 2025

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jul 8, 2025

We should not build the sorted structure for the ordinal grouping operator if the requested position is larger than maxGroupId. This situation occurs with nulls. We should benchmark the ordinal blocks and consider removing the ordinal grouping operator if performance is similar; otherwise, we need to integrate this operator with GroupingAggregatorFunctionTestCase.

Relates #130576

@dnhatn dnhatn requested a review from nik9000 July 8, 2025 22:08
@dnhatn dnhatn marked this pull request as ready for review July 8, 2025 22:08
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 8, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@dnhatn dnhatn added the auto-backport Automatically create backport pull requests when merged label Jul 8, 2025
@dnhatn dnhatn merged commit f58d291 into elastic:main Jul 8, 2025
10 of 33 checks passed
@dnhatn dnhatn deleted the fix-values-ordinals-on-empty branch July 8, 2025 22:13
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Jul 8, 2025
We should not build the sorted structure for the ordinal grouping 
operator if the requested position is larger than maxGroupId. This
situation occurs with nulls. We should benchmark the ordinal blocks and
consider removing the ordinal grouping operator if performance is
similar; otherwise, we need to integrate this operator with
GroupingAggregatorFunctionTestCase.

Relates elastic#130576
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
9.0 Commit could not be cherrypicked due to conflicts
8.18 Commit could not be cherrypicked due to conflicts
8.17 Commit could not be cherrypicked due to conflicts
9.1
8.19

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 130861

dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Jul 8, 2025
We should not build the sorted structure for the ordinal grouping 
operator if the requested position is larger than maxGroupId. This
situation occurs with nulls. We should benchmark the ordinal blocks and
consider removing the ordinal grouping operator if performance is
similar; otherwise, we need to integrate this operator with
GroupingAggregatorFunctionTestCase.

Relates elastic#130576
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Jul 8, 2025
We should not build the sorted structure for the ordinal grouping
operator if the requested position is larger than maxGroupId. This
situation occurs with nulls. We should benchmark the ordinal blocks and
consider removing the ordinal grouping operator if performance is
similar; otherwise, we need to integrate this operator with
GroupingAggregatorFunctionTestCase.

Relates elastic#130576

(cherry picked from commit f58d291)
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Jul 8, 2025
We should not build the sorted structure for the ordinal grouping
operator if the requested position is larger than maxGroupId. This
situation occurs with nulls. We should benchmark the ordinal blocks and
consider removing the ordinal grouping operator if performance is
similar; otherwise, we need to integrate this operator with
GroupingAggregatorFunctionTestCase.

Relates elastic#130576

(cherry picked from commit f58d291)
@dnhatn
Copy link
Member Author

dnhatn commented Jul 8, 2025

💚 All backports created successfully

Status Branch Result
9.0
8.18
8.17

Questions ?

Please refer to the Backport tool documentation

dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Jul 8, 2025
We should not build the sorted structure for the ordinal grouping
operator if the requested position is larger than maxGroupId. This
situation occurs with nulls. We should benchmark the ordinal blocks and
consider removing the ordinal grouping operator if performance is
similar; otherwise, we need to integrate this operator with
GroupingAggregatorFunctionTestCase.

Relates elastic#130576

(cherry picked from commit f58d291)

# Conflicts:
#	x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/OperatorTests.java
elasticsearchmachine pushed a commit that referenced this pull request Jul 8, 2025
We should not build the sorted structure for the ordinal grouping 
operator if the requested position is larger than maxGroupId. This
situation occurs with nulls. We should benchmark the ordinal blocks and
consider removing the ordinal grouping operator if performance is
similar; otherwise, we need to integrate this operator with
GroupingAggregatorFunctionTestCase.

Relates #130576
elasticsearchmachine pushed a commit that referenced this pull request Jul 9, 2025
* Fix empty VALUES with ordinals grouping (#130861)

We should not build the sorted structure for the ordinal grouping 
operator if the requested position is larger than maxGroupId. This
situation occurs with nulls. We should benchmark the ordinal blocks and
consider removing the ordinal grouping operator if performance is
similar; otherwise, we need to integrate this operator with
GroupingAggregatorFunctionTestCase.

Relates #130576

* Fix test
dnhatn added a commit that referenced this pull request Jul 9, 2025
We should not build the sorted structure for the ordinal grouping
operator if the requested position is larger than maxGroupId. This
situation occurs with nulls. We should benchmark the ordinal blocks and
consider removing the ordinal grouping operator if performance is
similar; otherwise, we need to integrate this operator with
GroupingAggregatorFunctionTestCase.

Relates #130576

(cherry picked from commit f58d291)

# Conflicts:
#	x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/OperatorTests.java
dnhatn added a commit that referenced this pull request Jul 9, 2025
We should not build the sorted structure for the ordinal grouping
operator if the requested position is larger than maxGroupId. This
situation occurs with nulls. We should benchmark the ordinal blocks and
consider removing the ordinal grouping operator if performance is
similar; otherwise, we need to integrate this operator with
GroupingAggregatorFunctionTestCase.

Relates #130576

(cherry picked from commit f58d291)
dnhatn added a commit that referenced this pull request Jul 9, 2025
We should not build the sorted structure for the ordinal grouping
operator if the requested position is larger than maxGroupId. This
situation occurs with nulls. We should benchmark the ordinal blocks and
consider removing the ordinal grouping operator if performance is
similar; otherwise, we need to integrate this operator with
GroupingAggregatorFunctionTestCase.

Relates #130576

(cherry picked from commit f58d291)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.17.9 v8.18.4 v8.19.1 v9.0.4 v9.1.1 v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants