Skip to content

ESQL: Ensure non-zero row size in EstimatesRowSize#122762

Merged
dnhatn merged 9 commits intoelastic:mainfrom
kanoshiou:remove-estimated-row-size-assertion
Mar 4, 2025
Merged

ESQL: Ensure non-zero row size in EstimatesRowSize#122762
dnhatn merged 9 commits intoelastic:mainfrom
kanoshiou:remove-estimated-row-size-assertion

Conversation

@kanoshiou
Copy link
Contributor

@kanoshiou kanoshiou commented Feb 17, 2025

Remove the assertion of rowSize in planTopN, because in LocalExecutionPlannerContext.pageSize(Integer), there is already a check for estimatedRowSize to ensure it is not null and not zero.

Closes #121535
@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Feb 17, 2025
@kanoshiou
Copy link
Contributor Author

kanoshiou commented Feb 17, 2025 •
edited
Loading

I think the query ROW a=null | SORT a should return correctly, instead of throwing an exception. However, since the DataType of a is NULL and its estimatedSize is 0, the query does not pass the check. Perhaps changing the estimatedSize of NULL to a non-zero value (such as 1) could solve this problem, but I am unsure if there would be any side effects.
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed needs:triage Requires assignment of a team area label labels Feb 17, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Feb 17, 2025

Pinging @elastic/es-analytical-engine (Team:Analytics)
@astefan astefan requested a review from dnhatn February 26, 2025 09:05
@dnhatn dnhatn self-assigned this Mar 3, 2025
dnhatn
dnhatn reviewed Mar 3, 2025
View reviewed changes
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.

@kanoshiou Thank you for looking into it. I think we should not remove this assertion, but ensure that at least one byte is returned in EstimatesRowSize#consumeAllFields. Can you also add the query ROW a=null | SORT a to EsqlActionIT?
@kanoshiou
Copy link
Contributor Author

kanoshiou commented Mar 3, 2025

Thank you for reviewing, @dnhatn! The assertion has been restored, and I've added the test you specified. Please review when you have the time.😊
@kanoshiou kanoshiou changed the title ESQL: Remove estimated row size assertion Mar 3, 2025
nik9000
nik9000 approved these changes Mar 3, 2025
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Looks good to me.
@nik9000 nik9000 added >bug and removed >non-issue labels Mar 3, 2025
@nik9000 nik9000 enabled auto-merge (squash) March 3, 2025 15:39
@dnhatn
Copy link
Member

dnhatn commented Mar 3, 2025

test this please
@dnhatn dnhatn added the auto-backport Automatically create backport pull requests when merged label Mar 3, 2025
dnhatn
dnhatn approved these changes Mar 3, 2025
View reviewed changes
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.

Great investigation. Thanks @kanoshiou!
@dnhatn
Copy link
Member

dnhatn commented Mar 3, 2025

@kanoshiou There is another issue with FragmentExec. Would you mind moving the new logic to TopNExec#estimateRowSize instead? I will address the FragmentExec issue later. However, if you're interested in fixing it, feel free to give it a try.

CI failure: https://gradle-enterprise.elastic.co/s/4p6fd4lzayp66
@dnhatn
Copy link
Member

dnhatn commented Mar 3, 2025

And this query too row a = null | STATS BY a
@kanoshiou
Copy link
Contributor Author

kanoshiou commented Mar 4, 2025

Thanks for reviewing @nik9000 @dnhatn, and I‘m happy to help to resolve the new issue!
auto-merge was automatically disabled March 4, 2025 03:25

Head branch was pushed to by a user without write access

@kanoshiou
Copy link
Contributor Author

kanoshiou commented Mar 4, 2025

Thank you for your review, @dnhatn! But I have no idea what's wrong with FragmentExec because I don't have access to the content in the link you posted above, so any additional information would be very helpful. However, I was able to review the test failure run by Buildkite, and I have tested the failed PhysicalPlanOptimizerTests on my machine.
@dnhatn
Copy link
Member

dnhatn commented Mar 4, 2025

test this please
@dnhatn
Copy link
Member

dnhatn commented Mar 4, 2025

@kanoshiou Thanks for your contribution.
@dnhatn dnhatn merged commit 4d2cb53 into elastic:main Mar 4, 2025
18 checks passed
This was referenced Mar 4, 2025
Merged
Merged
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Mar 4, 2025

💚 Backport successful

Status Branch Result
✅ 8.18
✅ 9.0
dnhatn pushed a commit to dnhatn/elasticsearch that referenced this pull request Mar 4, 2025
@kanoshiou @dnhatn
ESQL: Ensure non-zero row size in EstimatesRowSize (elastic#122762)
8ce0d6c 
Closes elastic#121535
@dnhatn dnhatn added the v8.19.0 label Mar 4, 2025
@dnhatn dnhatn mentioned this pull request Mar 4, 2025
Merged
@dnhatn
Copy link
Member

dnhatn commented Mar 4, 2025

💚 All backports created successfully

Status Branch Result
✅ 8.x

Questions ?

Please refer to the Backport tool documentation
dnhatn pushed a commit to dnhatn/elasticsearch that referenced this pull request Mar 4, 2025
@kanoshiou
ESQL: Ensure non-zero row size in EstimatesRowSize (elastic#122762)
476073f 
Closes elastic#121535

(cherry picked from commit 4d2cb53)
elasticsearchmachine pushed a commit that referenced this pull request Mar 4, 2025
@dnhatn @kanoshiou
ESQL: Ensure non-zero row size in EstimatesRowSize (#122762) (#123958)
8e409d6 
Closes #121535

Co-authored-by: kanoshiou <73424326+kanoshiou@users.noreply.github.com>
georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Mar 11, 2025
@kanoshiou @georgewallace
ESQL: Ensure non-zero row size in EstimatesRowSize (elastic#122762)
176015c 
Closes elastic#121535
ivancea pushed a commit to ivancea/elasticsearch that referenced this pull request Apr 11, 2025
@kanoshiou @dnhatn
ESQL: Ensure non-zero row size in EstimatesRowSize (elastic#122762)
5e3f34f 
Closes elastic#121535
elasticsearchmachine pushed a commit that referenced this pull request Apr 11, 2025
@dnhatn @kanoshiou
@costin @ivancea
[8.x] ESQL: Ensure non-zero row size in EstimatesRowSize (#122762) (#…
ca45080 
…123959)

* ESQL: Ensure non-zero row size in `EstimatesRowSize` (#122762)

Closes #121535

(cherry picked from commit 4d2cb53)

* Removed getFirst() usages

---------

Co-authored-by: kanoshiou <73424326+kanoshiou@users.noreply.github.com>
Co-authored-by: Costin Leau <costin@users.noreply.github.com>
Co-authored-by: Iván Cea Fontenla <ivancea96@outlook.com>
Co-authored-by: Iván Cea Fontenla <ivan.cea@elastic.co>
elasticsearchmachine pushed a commit that referenced this pull request Apr 11, 2025
@dnhatn @kanoshiou
@costin @ivancea
[8.18] ESQL: Ensure non-zero row size in EstimatesRowSize (#122762) (…
47bda42 
…#123957)

* ESQL: Ensure non-zero row size in `EstimatesRowSize` (#122762)

Closes #121535

* Removed getFirest() usages

---------

Co-authored-by: kanoshiou <73424326+kanoshiou@users.noreply.github.com>
Co-authored-by: Costin Leau <costin@users.noreply.github.com>
Co-authored-by: Iván Cea Fontenla <ivancea96@outlook.com>
Co-authored-by: Iván Cea Fontenla <ivan.cea@elastic.co>
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 >bug external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.18.1 v8.19.0 v9.0.1 v9.1.0

5 participants

@kanoshiou @elasticsearchmachine @dnhatn @nik9000 @gareth-ellis