-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix intermittent SQL logic test failure in limit.slt by adding ORDER BY clause #16257
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
alamb
left a comment
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.
Thank you @kosiew
| with selection as ( | ||
| select * | ||
| from test_limit_with_partitions | ||
| order by part_key |
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.
Thank you @kosiew -- can you please also update the query above (I think the EXPLAIN and query are supposed to remain in sync
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.
@alamb
Thanks for pointing this out.
Updated.
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.
This looks good to me, nice find @kosiew!
|
Thanks @jonathanc-n for the review. |
alamb
left a comment
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.
Thanks @kosiew and @jonathanc-n
…BY clause (apache#16257) * Add order by clause to limit query for consistent results * test: update explain plan
…BY clause (apache#16257) * Add order by clause to limit query for consistent results * test: update explain plan
…ing ORDER BY clause (apache#16257) * Add order by clause to limit query for consistent results * test: update explain plan (cherry picked from commit 4cf1148)
…BY clause (apache#16257) * Add order by clause to limit query for consistent results * test: update explain plan
…ing ORDER BY clause (apache#16257) * Add order by clause to limit query for consistent results * test: update explain plan (cherry picked from commit 4cf1148)
Which issue does this PR close?
test_files/limit.slt#16180.Rationale for this change
Intermittent test failures were observed in the
limit.sltSQL logic test file on themainbranch, specifically related to a query that uses aLIMITclause without anORDER BY. Since SQLLIMITbehavior is non-deterministic without a defined order, this led to flaky test results.This change explicitly adds an
ORDER BY part_keyto theLIMITsubquery to ensure deterministic results and resolve test flakiness.What changes are included in this PR?
ORDER BY part_keybefore theLIMITclause.Sortoperator before the limit.SortPreservingMergeExecto maintain global ordering across partitions.DataSourceExecwith a combination ofSortExecandTopKon each partition to correctly apply the limit post-sort.Are these changes tested?
Yes. The change modifies an existing SQL logic test to enforce a deterministic result, which now consistently passes in CI runs.
Tested with:
Are there any user-facing changes?
No. This change only affects internal test stability and does not impact the user-facing API or behavior.