You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/05/11 07:32:13 UTC
[GitHub] [spark] morvenhuang opened a new pull request, #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
morvenhuang opened a new pull request, #36507:
URL: https://github.com/apache/spark/pull/36507
### What changes were proposed in this pull request?
Use count() instead of filter().size, use df.count() instead of df.collect().size.
### Why are the changes needed?
Code simplification.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Pass GA.
--
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] morvenhuang commented on pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
morvenhuang commented on PR #36507:
URL: https://github.com/apache/spark/pull/36507#issuecomment-1124390062
@srowen thank you Sean.
--
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] srowen closed pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
URL: https://github.com/apache/spark/pull/36507
--
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] morvenhuang commented on pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
morvenhuang commented on PR #36507:
URL: https://github.com/apache/spark/pull/36507#issuecomment-1124332111
The GC overhead limit exceeded error in AppVeyor build deesn't seem relevant.
--
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] morvenhuang commented on a diff in pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
morvenhuang commented on code in PR #36507:
URL: https://github.com/apache/spark/pull/36507#discussion_r870343693
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -318,31 +318,31 @@ class JDBCSuite extends QueryTest
}
test("SELECT *") {
- assert(sql("SELECT * FROM foobar").collect().size === 3)
+ assert(sql("SELECT * FROM foobar").count() === 3)
}
test("SELECT * WHERE (simple predicates)") {
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE THEID < 1")).collect().size == 0)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE THEID != 2")).collect().size == 2)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE THEID = 1")).collect().size == 1)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE NAME = 'fred'")).collect().size == 1)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE NAME <=> 'fred'")).collect().size == 1)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE NAME > 'fred'")).collect().size == 2)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE NAME != 'fred'")).collect().size == 2)
+ assert(checkPushdown(sql("SELECT * FROM foobar WHERE THEID < 1")).count() == 0)
Review Comment:
Thanks for the review. Reverted the change in this test case, so that it won't cause any behavior change.
--
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] morvenhuang commented on a diff in pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
morvenhuang commented on code in PR #36507:
URL: https://github.com/apache/spark/pull/36507#discussion_r870358151
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -318,7 +318,7 @@ class JDBCSuite extends QueryTest
}
test("SELECT *") {
- assert(sql("SELECT * FROM foobar").collect().size === 3)
+ assert(sql("SELECT * FROM foobar").count() === 3)
Review Comment:
Thanks Hyukjin, for pointing out this, I've reverted changes in this file.
--
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 #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #36507:
URL: https://github.com/apache/spark/pull/36507#discussion_r870342302
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -318,7 +318,7 @@ class JDBCSuite extends QueryTest
}
test("SELECT *") {
- assert(sql("SELECT * FROM foobar").collect().size === 3)
+ assert(sql("SELECT * FROM foobar").count() === 3)
Review Comment:
@morvenhuang Just to clarify, the whole `collect().size` vs `count()` at `DataFrame` is completely different. For example, this will prune all, and won't actually test if the data in foobar is correctly ser/de or not.
--
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] srowen commented on pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #36507:
URL: https://github.com/apache/spark/pull/36507#issuecomment-1123844096
Oh I see, good catch!
--
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] AmplabJenkins commented on pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #36507:
URL: https://github.com/apache/spark/pull/36507#issuecomment-1124312107
Can one of the admins verify this patch?
--
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] morvenhuang commented on a diff in pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
morvenhuang commented on code in PR #36507:
URL: https://github.com/apache/spark/pull/36507#discussion_r870343137
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/StreamingJoinHelper.scala:
##########
@@ -132,8 +132,8 @@ object StreamingJoinHelper extends PredicateHelper with Logging {
leftExpr.collect { case a: AttributeReference => a } ++
rightExpr.collect { case a: AttributeReference => a }
)
- if (attributesInCondition.filter { attributesToFindStateWatermarkFor.contains(_) }.size > 1 ||
- attributesInCondition.filter { attributesWithEventWatermark.contains(_) }.size > 1) {
+ if (attributesInCondition.count { attributesToFindStateWatermarkFor.contains(_) } > 1 ||
Review Comment:
Thank you, you're right, I've updated these 2 lines.
--
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] srowen commented on pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #36507:
URL: https://github.com/apache/spark/pull/36507#issuecomment-1124380458
Yes I'm confident it is not related. The others passed. Merged to master
--
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] srowen commented on a diff in pull request #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #36507:
URL: https://github.com/apache/spark/pull/36507#discussion_r870317661
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/StreamingJoinHelper.scala:
##########
@@ -132,8 +132,8 @@ object StreamingJoinHelper extends PredicateHelper with Logging {
leftExpr.collect { case a: AttributeReference => a } ++
rightExpr.collect { case a: AttributeReference => a }
)
- if (attributesInCondition.filter { attributesToFindStateWatermarkFor.contains(_) }.size > 1 ||
- attributesInCondition.filter { attributesWithEventWatermark.contains(_) }.size > 1) {
+ if (attributesInCondition.count { attributesToFindStateWatermarkFor.contains(_) } > 1 ||
Review Comment:
Probably just `.count(attributesToFindStateWatermarkFor.contains)` even?
--
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 #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #36507:
URL: https://github.com/apache/spark/pull/36507#discussion_r870012534
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -318,31 +318,31 @@ class JDBCSuite extends QueryTest
}
test("SELECT *") {
- assert(sql("SELECT * FROM foobar").collect().size === 3)
+ assert(sql("SELECT * FROM foobar").count() === 3)
}
test("SELECT * WHERE (simple predicates)") {
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE THEID < 1")).collect().size == 0)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE THEID != 2")).collect().size == 2)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE THEID = 1")).collect().size == 1)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE NAME = 'fred'")).collect().size == 1)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE NAME <=> 'fred'")).collect().size == 1)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE NAME > 'fred'")).collect().size == 2)
- assert(checkPushdown(sql("SELECT * FROM foobar WHERE NAME != 'fred'")).collect().size == 2)
+ assert(checkPushdown(sql("SELECT * FROM foobar WHERE THEID < 1")).count() == 0)
Review Comment:
This case we need to be careful because the optimizer might trigger another optimization per count aggregation
--
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 #36507: [SPARK-39147][SQL] Code simplification, use count() instead of filter().size, etc.
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on code in PR #36507:
URL: https://github.com/apache/spark/pull/36507#discussion_r870342302
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -318,7 +318,7 @@ class JDBCSuite extends QueryTest
}
test("SELECT *") {
- assert(sql("SELECT * FROM foobar").collect().size === 3)
+ assert(sql("SELECT * FROM foobar").count() === 3)
Review Comment:
@morvenhuang Just to clarify, the whole `collect().size` vs `count()` at `DataFrame` is completely different. For example, this will prune all, and won't actually test if the data in foobar is correctly ser/de or not.
We should better revert the changes here. Or you might have to justify that each fix here does not change the test coverage.
--
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