You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "mihailoale-db (via GitHub)" <gi...@apache.org> on 2025/02/24 13:53:23 UTC
[PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
mihailoale-db opened a new pull request, #50069:
URL: https://github.com/apache/spark/pull/50069
### What changes were proposed in this pull request?
I propose that we extend `order-by-all.sql` and add `order-by.sql` which extend the coverage of `ORDER 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1970266534
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1,2,3)")
+ checkAnswer(df.select("col1").orderBy("col2").orderBy("col3"), Row(1))
+ }
+
+ test("Column reference takes precedence over ALL keyword") {
+ val df1 = sql("select * from values(1,2,4) as t1(a, b, all1)")
+ val df2 = sql("select * from values(1,3,2) as t2(a, b, all)")
+ val df3 = df1.join(df2, df1("a") === df2("a"))
+ checkAnswer(df3.select(df1("all").as("all1")).orderBy("all"), Row(2))
Review Comment:
No need for that. it is obvious here that we order by column instead of `all` keyword. adding more rows would be redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1970038998
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1,2,3)")
+ checkAnswer(df.select("col1").orderBy("col2").orderBy("col3"), Row(1))
Review Comment:
Let's also add another case with 4 nested `orderBy`
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1,2,3)")
+ checkAnswer(df.select("col1").orderBy("col2").orderBy("col3"), Row(1))
Review Comment:
Let's also add an error path where `col2` or `col3` cannot be referenced above `SubqueryAlias` (`.as("q1")`)
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
Review Comment:
Can we add another test case "Column reference takes precedence over ALL keyword with nested ORDER BY", where we would have the same setup with nested `orderBy`, but the table has column "all", and the top `orderBy` references this lower column?
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1,2,3)")
+ checkAnswer(df.select("col1").orderBy("col2").orderBy("col3"), Row(1))
+ }
+
+ test("Column reference takes precedence over ALL keyword") {
+ val df1 = sql("select * from values(1,2,4) as t1(a, b, all1)")
+ val df2 = sql("select * from values(1,3,2) as t2(a, b, all)")
+ val df3 = df1.join(df2, df1("a") === df2("a"))
+ checkAnswer(df3.select(df1("all").as("all1")).orderBy("all"), Row(2))
Review Comment:
Let's add more rows to make sure we are ordering correctly
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1,2,3)")
+ checkAnswer(df.select("col1").orderBy("col2").orderBy("col3"), Row(1))
+ }
+
+ test("Column reference takes precedence over ALL keyword") {
Review Comment:
```suggestion
test("Column reference above JOIN takes precedence over ALL keyword") {
```
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1,2,3)")
+ checkAnswer(df.select("col1").orderBy("col2").orderBy("col3"), Row(1))
+ }
+
+ test("Column reference takes precedence over ALL keyword") {
+ val df1 = sql("select * from values(1,2,4) as t1(a, b, all1)")
+ val df2 = sql("select * from values(1,3,2) as t2(a, b, all)")
+ val df3 = df1.join(df2, df1("a") === df2("a"))
+ checkAnswer(df3.select(df1("all").as("all1")).orderBy("all"), Row(2))
Review Comment:
Let's also test the `Sort` right above the `Join`: `df3.orderBy("all")`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1971295846
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,24 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
Review Comment:
@MaxGekk could you please address this? Should we drop it or no
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1970261517
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1,2,3)")
+ checkAnswer(df.select("col1").orderBy("col2").orderBy("col3"), Row(1))
+ }
+
+ test("Column reference takes precedence over ALL keyword") {
+ val df1 = sql("select * from values(1,2,4) as t1(a, b, all1)")
+ val df2 = sql("select * from values(1,3,2) as t2(a, b, all)")
+ val df3 = df1.join(df2, df1("a") === df2("a"))
+ checkAnswer(df3.select(df1("all").as("all1")).orderBy("all"), Row(2))
Review Comment:
It throws: `[AMBIGUOUS_COLUMN_OR_FIELD] Column or field `a` is ambiguous and has 2 matches.`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1971260413
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,24 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
Review Comment:
Maybe it is just a view?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1967915773
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,24 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
+(1, 1), (1, 2), (2, 1), (2, 2), (3, 1), (3, 2), (null, 1), (3, null), (null, null)
+AS testData(a, b);
+
+-- ORDER BY a column from a child's output
+SELECT a FROM testData ORDER BY a;
+SELECT a FROM testData GROUP BY a, b ORDER BY a;
+
+-- ORDER BY a column from an output below a child's one
+SELECT b FROM testData WHERE a > 1 ORDER BY a;
+
+-- ORDER BY a column from grouping expressions list
+SELECT b FROM testData GROUP BY a, b ORDER BY a;
+
+-- ORDER BY a nested column from an output below a child's one
+SELECT col1 FROM VALUES (1, named_struct('f1', 1)) ORDER BY col2.f1;
+SELECT col1 FROM VALUES (1, named_struct('f1', named_struct('f2', 1))) ORDER BY col2.f1.f2;
+
+-- ORDER BY column can't reference an outer scope
+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;
Review Comment:
We don't resolve it as `ALL` keyword but as a column, that's why I put it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1967916766
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,24 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
Review Comment:
We don't do that in any of the tests. @MaxGekk why is that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1970270797
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1,2,3)")
+ checkAnswer(df.select("col1").orderBy("col2").orderBy("col3"), Row(1))
+ }
+
+ test("Column reference takes precedence over ALL keyword") {
+ val df1 = sql("select * from values(1,2,4) as t1(a, b, all1)")
+ val df2 = sql("select * from values(1,3,2) as t2(a, b, all)")
+ val df3 = df1.join(df2, df1("a") === df2("a"))
+ checkAnswer(df3.select(df1("all").as("all1")).orderBy("all"), Row(2))
Review Comment:
What do you mean "obvious"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1971387629
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,24 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
Review Comment:
Let's add it as it doesn't add any overhead and is more robust.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1967880226
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,24 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
Review Comment:
You should drop this view at the end of the test.
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,24 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
+(1, 1), (1, 2), (2, 1), (2, 2), (3, 1), (3, 2), (null, 1), (3, null), (null, null)
+AS testData(a, b);
+
+-- ORDER BY a column from a child's output
+SELECT a FROM testData ORDER BY a;
+SELECT a FROM testData GROUP BY a, b ORDER BY a;
+
+-- ORDER BY a column from an output below a child's one
+SELECT b FROM testData WHERE a > 1 ORDER BY a;
+
+-- ORDER BY a column from grouping expressions list
+SELECT b FROM testData GROUP BY a, b ORDER BY a;
+
+-- ORDER BY a nested column from an output below a child's one
+SELECT col1 FROM VALUES (1, named_struct('f1', 1)) ORDER BY col2.f1;
+SELECT col1 FROM VALUES (1, named_struct('f1', named_struct('f2', 1))) ORDER BY col2.f1.f2;
+
+-- ORDER BY column can't reference an outer scope
+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;
Review Comment:
This should be in order-by-all.sql, right?
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,24 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
+(1, 1), (1, 2), (2, 1), (2, 2), (3, 1), (3, 2), (null, 1), (3, null), (null, null)
+AS testData(a, b);
+
+-- ORDER BY a column from a child's output
+SELECT a FROM testData ORDER BY a;
Review Comment:
We probably have these simple test cases somewhere already? I would be surprised if we don't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1970036326
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1,2,3)")
Review Comment:
Let's add more rows in unsorted order to make sure that this test checks the ordering :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on PR #50069:
URL: https://github.com/apache/spark/pull/50069#issuecomment-2688377111
@MaxGekk @cloud-fan Failures don't seem related to changes? Please check when you have time. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50069:
URL: https://github.com/apache/spark/pull/50069#issuecomment-2689447332
This is a test only PR and other test failures are definitely unrelated. Thanks, merging to master/4.0!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1969442581
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,27 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
+(1, 1), (1, 2), (2, 1), (2, 2), (3, 1), (3, 2), (null, 1), (3, null), (null, null)
+AS testData(a, b);
+
+-- ORDER BY a column from a child's output
+SELECT a FROM testData ORDER BY a;
+SELECT a FROM testData GROUP BY a, b ORDER BY a;
+
+-- ORDER BY a column from an output below a child's one
+SELECT b FROM testData WHERE a > 1 ORDER BY a;
+
+-- ORDER BY a column from grouping expressions list
+SELECT b FROM testData GROUP BY a, b ORDER BY a;
+
+-- ORDER BY a nested column from an output below a child's one
+SELECT col1 FROM VALUES (1, named_struct('f1', 1)) ORDER BY col2.f1;
+SELECT col1 FROM VALUES (1, named_struct('f1', named_struct('f2', 1))) ORDER BY col2.f1.f2;
+
+-- ORDER BY column can't reference an outer scope
+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;
+
+-- Nested ORDER BY calls
+SELECT * FROM testData ORDER BY (SELECT a FROM testData ORDER BY b);
Review Comment:
Let's add some more:
`SELECT * FROM testData ORDER BY (SELECT * FROM testData ORDER BY (SELECT a FROM testData ORDER BY b));`
`SELECT * FROM testData ORDER BY (SELECT a FROM VALUES (1)); -- Fails because correlation is not allowed in ORDER BY`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1970090352
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,31 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
+(1, 1), (1, 2), (2, 1), (2, 2), (3, 1), (3, 2), (null, 1), (3, null), (null, null)
+AS testData(a, b);
+
+-- ORDER BY a column from a child's output
+SELECT a FROM testData ORDER BY a;
+SELECT a FROM testData GROUP BY a, b ORDER BY a;
+
+-- ORDER BY a column from an output below a child's one
+SELECT b FROM testData WHERE a > 1 ORDER BY a;
+
+-- ORDER BY a column from grouping expressions list
+SELECT b FROM testData GROUP BY a, b ORDER BY a;
+
+-- ORDER BY a nested column from an output below a child's one
+SELECT col1 FROM VALUES (1, named_struct('f1', 1)) ORDER BY col2.f1;
+SELECT col1 FROM VALUES (1, named_struct('f1', named_struct('f2', 1))) ORDER BY col2.f1.f2;
+
+-- ORDER BY column can't reference an outer scope
+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;
+
+-- Nested ORDER BY calls
Review Comment:
```suggestion
-- ORDER BY with scalar subqueries
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on PR #50069:
URL: https://github.com/apache/spark/pull/50069#issuecomment-2682906496
Please address this: https://github.com/apache/spark/pull/50069#discussion_r1970270797
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1971262627
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,24 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
Review Comment:
We do (e.g. `union.sql`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1971323995
##########
sql/core/src/test/resources/sql-tests/inputs/order-by.sql:
##########
@@ -0,0 +1,24 @@
+-- Test data.
+CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES
Review Comment:
The test suite creates new Spark session per every `.sql`, so at the end session-level objects should disappear, but permanent won't, and should be dropped explicitly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1970267788
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,18 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1,2,3)")
+ checkAnswer(df.select("col1").orderBy("col2").orderBy("col3"), Row(1))
+ }
+
+ test("Column reference takes precedence over ALL keyword") {
+ val df1 = sql("select * from values(1,2,4) as t1(a, b, all1)")
+ val df2 = sql("select * from values(1,3,2) as t2(a, b, all)")
+ val df3 = df1.join(df2, df1("a") === df2("a"))
+ checkAnswer(df3.select(df1("all").as("all1")).orderBy("all"), Row(2))
Review Comment:
Hmm, interesting, but we don't have two matches in that case? Can you add it nevertheless under `intercept` so we can dual-run it with the new single-pass Analyzer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #50069: [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage
URL: https://github.com/apache/spark/pull/50069
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on PR #50069:
URL: https://github.com/apache/spark/pull/50069#issuecomment-2679037294
@MaxGekk Could you PTAL when you have time? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1970290340
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,39 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
+ val df = sql("SELECT * FROM VALUES(1, 5, 3), (4, 2, 6)")
Review Comment:
These rows are already ordered, so your test data doesn't check the transformation applied by `orderBy`
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,39 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
Review Comment:
One more example that we should add:
```
val df1 = spark.sql("SELECT * FROM VALUES (4,5,6), (1,2,3)")
val df2 = df1.select("col1").orderBy("col2")
// Cannot order by missing attribute under SubqueryAlias
val df3 = df2.as("q1").orderBy("col3")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51303] [SQL] [TESTS] Extend `ORDER BY` testing coverage [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50069:
URL: https://github.com/apache/spark/pull/50069#discussion_r1970308036
##########
sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala:
##########
@@ -2721,6 +2721,39 @@ class DataFrameSuite extends QueryTest
parameters = Map("name" -> ".whatever")
)
}
+
+ test("Nested order by") {
Review Comment:
One more example that we should add:
```
val df1 = spark.sql("SELECT * FROM VALUES (4,5,6), (1,2,3)")
val df2 = df1.select("col1").orderBy("col2")
val df3 = df1.select("col1").orderBy(df1("col2"))
// Cannot order by missing attribute under SubqueryAlias
val df4 = df3.as("q1").orderBy("col3")
val df5 = df3.as("q1").orderBy(df1("col3"))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org