Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Mar 4, 2025
Merged

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

merged 9 commits into from
Mar 4, 2025

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 ESQL: Ensure non-zero row size in EstimatesRowSize 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 <[email protected]>
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 <[email protected]>
Co-authored-by: Costin Leau <[email protected]>
Co-authored-by: Iván Cea Fontenla <[email protected]>
Co-authored-by: Iván Cea Fontenla <[email protected]>
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 <[email protected]>
Co-authored-by: Costin Leau <[email protected]>
Co-authored-by: Iván Cea Fontenla <[email protected]>
Co-authored-by: Iván Cea Fontenla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dnhatn dnhatn dnhatn approved these changes

@nik9000 nik9000 nik9000 approved these changes

Assignees

@dnhatn dnhatn

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ES|QL "ROW a=null | SORT a" crashes with AssertionError

5 participants

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