You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2023/03/17 09:58:02 UTC
[GitHub] [spark] beliefer opened a new pull request, #40466: [SPARK-42835][SQL][TESTS] Add test cases for Column.explain
beliefer opened a new pull request, #40466:
URL: https://github.com/apache/spark/pull/40466
### What changes were proposed in this pull request?
Recently, I found Column.explain missing test cases.
This PR want add these test cases for easy to find the change if the `def toString` or `def sql` changed.
### Why are the changes needed?
Add test cases for Column.explain
### Does this PR introduce _any_ user-facing change?
'No'.
Just add test cases for Column.explain.
### How was this patch tested?
New test cases.
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40466:
URL: https://github.com/apache/spark/pull/40466#discussion_r1141966325
##########
sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala:
##########
@@ -921,6 +922,132 @@ class ColumnExpressionSuite extends QueryTest with SharedSparkSession {
}
}
+ private def captureStdOut(block: => Unit): String = {
+ val capturedOut = new ByteArrayOutputStream()
+ Console.withOut(capturedOut)(block)
+ capturedOut.toString()
+ }
+
+ test("explain") {
Review Comment:
shall we use golden file tests? We don't expect it to be stable but we should be aware if it gets changed.
--
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
[GitHub] [spark] amaliujia commented on pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for Column.explain
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on PR #40466:
URL: https://github.com/apache/spark/pull/40466#issuecomment-1474277110
Just a general question which is for my self education: do we expect the results of `Column.explain` are stable?
--
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
[GitHub] [spark] beliefer commented on a diff in pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40466:
URL: https://github.com/apache/spark/pull/40466#discussion_r1141536788
##########
sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala:
##########
@@ -921,6 +922,132 @@ class ColumnExpressionSuite extends QueryTest with SharedSparkSession {
}
}
+ private def captureStdOut(block: => Unit): String = {
+ val capturedOut = new ByteArrayOutputStream()
+ Console.withOut(capturedOut)(block)
+ capturedOut.toString()
+ }
+
+ test("explain") {
Review Comment:
> Hm, I wouldn't add a bunch of tests though. `expr.sql` isn't actually strictly providing the compatibility in its string representation, and this test case potentially fails often.
In fact, explain not only reactor `expr.sql`, and `expr.toString` too.
--
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
[GitHub] [spark] beliefer commented on pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for Column.explain
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #40466:
URL: https://github.com/apache/spark/pull/40466#issuecomment-1474622202
> Just a general question which is for my self education: do we expect the results of `Column.explain` are stable?
Personally, I think Spark should keep the stable output and do not break change for users.
--
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
[GitHub] [spark] beliefer commented on pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #40466:
URL: https://github.com/apache/spark/pull/40466#issuecomment-1475479633
ping @cloud-fan
--
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
[GitHub] [spark] beliefer commented on pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #40466:
URL: https://github.com/apache/spark/pull/40466#issuecomment-1482193211
> EDIT: sorry I just realized that this PR is to test the result of `Column.explain`. Why do we care about its stability? The API doc says `Prints the expression to the console for debugging purposes`. We don't have stability tests for the plan EXPLAIN result either.
Got it. I will close this PR.
--
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
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #40466:
URL: https://github.com/apache/spark/pull/40466#discussion_r1141519364
##########
sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala:
##########
@@ -921,6 +922,132 @@ class ColumnExpressionSuite extends QueryTest with SharedSparkSession {
}
}
+ private def captureStdOut(block: => Unit): String = {
+ val capturedOut = new ByteArrayOutputStream()
+ Console.withOut(capturedOut)(block)
+ capturedOut.toString()
+ }
+
+ test("explain") {
Review Comment:
Hm, I wouldn't add a bunch of tests though. `expr.sql` isn't actually strictly providing the compatibility in its string representation, and this test case potentially fails often.
--
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
[GitHub] [spark] cloud-fan commented on pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40466:
URL: https://github.com/apache/spark/pull/40466#issuecomment-1481119954
Can we use the existing golden file framework? I think we just need to add a bunch of test queries like `SELECT a + b`.
--
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
[GitHub] [spark] amaliujia commented on a diff in pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #40466:
URL: https://github.com/apache/spark/pull/40466#discussion_r1141519677
##########
sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala:
##########
@@ -921,6 +922,132 @@ class ColumnExpressionSuite extends QueryTest with SharedSparkSession {
}
}
+ private def captureStdOut(block: => Unit): String = {
+ val capturedOut = new ByteArrayOutputStream()
+ Console.withOut(capturedOut)(block)
+ capturedOut.toString()
+ }
+
+ test("explain") {
Review Comment:
yeah that was what I was wondering if this output is expected stable enough. If not then probably we should not add such tests.
--
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
[GitHub] [spark] beliefer closed pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer closed pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
URL: https://github.com/apache/spark/pull/40466
--
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
[GitHub] [spark] amaliujia commented on a diff in pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
Posted by "amaliujia (via GitHub)" <gi...@apache.org>.
amaliujia commented on code in PR #40466:
URL: https://github.com/apache/spark/pull/40466#discussion_r1142414407
##########
sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala:
##########
@@ -921,6 +922,132 @@ class ColumnExpressionSuite extends QueryTest with SharedSparkSession {
}
}
+ private def captureStdOut(block: => Unit): String = {
+ val capturedOut = new ByteArrayOutputStream()
+ Console.withOut(capturedOut)(block)
+ capturedOut.toString()
+ }
+
+ test("explain") {
Review Comment:
+1 if we still want be aware it's changing, golden files is a better way.
--
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
[GitHub] [spark] beliefer commented on a diff in pull request #40466: [SPARK-42835][SQL][TESTS] Add test cases for `Column.explain`
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40466:
URL: https://github.com/apache/spark/pull/40466#discussion_r1142825198
##########
sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala:
##########
@@ -921,6 +922,132 @@ class ColumnExpressionSuite extends QueryTest with SharedSparkSession {
}
}
+ private def captureStdOut(block: => Unit): String = {
+ val capturedOut = new ByteArrayOutputStream()
+ Console.withOut(capturedOut)(block)
+ capturedOut.toString()
+ }
+
+ test("explain") {
Review Comment:
It is hears better.
--
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