-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-51303] [SQL] [TESTS] Extend ORDER BY
testing coverage
#50069
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,24 @@ | |||
-- Test data. | |||
CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES |
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.
You should drop this view at the end of the test.
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.
We don't do that in any of the tests. @MaxGekk why is that?
AS testData(a, b); | ||
|
||
-- ORDER BY a column from a child's output | ||
SELECT a FROM testData ORDER BY a; |
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.
We probably have these simple test cases somewhere already? I would be surprised if we don't.
SELECT a, (SELECT b FROM testData GROUP BY b HAVING b > 1 ORDER BY a) FROM testData; | ||
|
||
-- Column resolution from the child's output takes the precedence over `ORDER BY ALL` | ||
SELECT a, (SELECT b FROM VALUES (1, 2) AS innerTestData (all, b) ORDER BY ALL) FROM testData; |
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 should be in order-by-all.sql, right?
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.
We don't resolve it as ALL
keyword but as a column, that's why I put it here.
@MaxGekk Could you PTAL when you have time? Thanks |
a0927af
to
aff6f51
Compare
aff6f51
to
061c8c7
Compare
What changes were proposed in this pull request?
I propose that we extend
order-by-all.sql
andDataFrameSuite.scala
and addorder-by.sql
which extend the coverage ofORDER BY
feature.Why are the changes needed?
In order to catch potential bugs.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added tests.
Was this patch authored or co-authored using generative AI tooling?
No.