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/03/21 09:46:12 UTC
[GitHub] [spark] beliefer opened a new pull request #35921: [SPARK-37483][SQL][FOLLOEUP] Refactor JDBCV2Suite by checkPushedInfo
beliefer opened a new pull request #35921:
URL: https://github.com/apache/spark/pull/35921
### What changes were proposed in this pull request?
This PR fix two issues.
First, create method checkPushedInfo to reuse code.
Second, rename pushedTopN to PushedTopN, so as consistent with other pushed information.
### Why are the changes needed?
Reuse code and let pushed information more correctly.
### Does this PR introduce _any_ user-facing change?
'No'. New feature and improve the tests.
### How was this patch tested?
Adjust existing 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 commented on pull request #35921: [SPARK-37483][SQL][FOLLOEUP] Rename `pushedTopN` to `PushedTopN` and improve JDBCV2Suite
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35921:
URL: https://github.com/apache/spark/pull/35921#issuecomment-1074909646
> @beliefer can you fix the merge conflicts? thanks!
OK
--
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 closed pull request #35921: [SPARK-37483][SQL][FOLLOWUP] Rename `pushedTopN` to `PushedTopN` and improve JDBCV2Suite
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #35921:
URL: https://github.com/apache/spark/pull/35921
--
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 change in pull request #35921: [SPARK-37483][SQL][FOLLOEUP] Refactor JDBCV2Suite by checkPushedInfo
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35921:
URL: https://github.com/apache/spark/pull/35921#discussion_r831691249
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala
##########
@@ -185,17 +185,28 @@ class JDBCV2Suite extends QueryTest with SharedSparkSession with ExplainSuiteHel
}
}
+ private def checkPushedInfo(df: DataFrame, expectedPlanFragment: String): Unit = {
+ df.queryExecution.optimizedPlan.collect {
+ case _: DataSourceV2ScanRelation =>
+ checkKeywordsExistsInExplain(df, expectedPlanFragment)
+ }
+ }
+
test("simple scan with top N") {
val df1 = spark.read
.table("h2.test.employee")
.sort("salary")
.limit(1)
checkPushedLimit(df1, Some(1), createSortValues())
Review comment:
Seems we don't need these methods anymore? `checkPushedInfo` can cover all of them
--
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] huaxingao commented on a change in pull request #35921: [SPARK-37483][SQL][FOLLOEUP] Rename `pushedTopN` to `PushedTopN` and improve JDBCV2Suite
Posted by GitBox <gi...@apache.org>.
huaxingao commented on a change in pull request #35921:
URL: https://github.com/apache/spark/pull/35921#discussion_r831777137
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala
##########
@@ -689,14 +616,8 @@ class JDBCV2Suite extends QueryTest with SharedSparkSession with ExplainSuiteHel
}
assert(filters.nonEmpty) // filter over aggregate not pushed down
checkAggregateRemoved(df)
Review comment:
Should this be `checkAggregateRemoved(query)`? Not related to this PR, though.
--
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 change in pull request #35921: [SPARK-37483][SQL][FOLLOEUP] Rename `pushedTopN` to `PushedTopN` and improve JDBCV2Suite
Posted by GitBox <gi...@apache.org>.
beliefer commented on a change in pull request #35921:
URL: https://github.com/apache/spark/pull/35921#discussion_r831783574
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala
##########
@@ -689,14 +616,8 @@ class JDBCV2Suite extends QueryTest with SharedSparkSession with ExplainSuiteHel
}
assert(filters.nonEmpty) // filter over aggregate not pushed down
checkAggregateRemoved(df)
Review comment:
I see.
--
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 #35921: [SPARK-37483][SQL][FOLLOWUP] Rename `pushedTopN` to `PushedTopN` and improve JDBCV2Suite
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35921:
URL: https://github.com/apache/spark/pull/35921#issuecomment-1076936544
@cloud-fan @huaxingao Thank you for 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
[GitHub] [spark] cloud-fan commented on pull request #35921: [SPARK-37483][SQL][FOLLOEUP] Rename `pushedTopN` to `PushedTopN` and improve JDBCV2Suite
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35921:
URL: https://github.com/apache/spark/pull/35921#issuecomment-1074846330
@beliefer can you fix the merge conflicts? 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
[GitHub] [spark] cloud-fan commented on pull request #35921: [SPARK-37483][SQL][FOLLOWUP] Rename `pushedTopN` to `PushedTopN` and improve JDBCV2Suite
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35921:
URL: https://github.com/apache/spark/pull/35921#issuecomment-1076388735
thanks, merging to master/3.3!
--
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 #35921: [SPARK-37483][SQL][FOLLOWUP] Rename `pushedTopN` to `PushedTopN` and improve JDBCV2Suite
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35921:
URL: https://github.com/apache/spark/pull/35921#issuecomment-1075857942
> @beliefer can you fix the conflicts?
OK
--
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 #35921: [SPARK-37483][SQL][FOLLOWUP] Rename `pushedTopN` to `PushedTopN` and improve JDBCV2Suite
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35921:
URL: https://github.com/apache/spark/pull/35921#issuecomment-1075823856
@beliefer can you fix the conflicts?
--
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 #35921: [SPARK-37483][SQL][FOLLOEUP] Refactor JDBCV2Suite by checkPushedInfo
Posted by GitBox <gi...@apache.org>.
beliefer commented on pull request #35921:
URL: https://github.com/apache/spark/pull/35921#issuecomment-1074557144
ping @huaxingao cc @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