-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-58555] Add new tests for JDBC DS Join pushdown #57714
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -229,8 +229,6 @@ trait JDBCV2JoinPushdownIntegrationSuiteBase | |
|
|
||
| protected val supportsColumnPruning: Boolean = true | ||
|
|
||
| protected val supportsJoinPushdown: Boolean = true | ||
|
|
||
| // Condition-less joins are not supported in join pushdown | ||
| test("Test that 2-way join without condition should not have join pushed down") { | ||
| val sqlQuery = | ||
|
|
@@ -497,6 +495,55 @@ trait JDBCV2JoinPushdownIntegrationSuiteBase | |
| } | ||
| } | ||
|
|
||
| test("Test aggregate with group by on top of join") { | ||
| val sqlQuery = | ||
| s""" | ||
| |SELECT t1.id, t1.address, min(t2.salary), count(1) | ||
| |FROM $catalogAndNamespace.$casedJoinTableName1 t1 | ||
| |JOIN $catalogAndNamespace.$casedJoinTableName2 t2 ON t1.id = t2.id | ||
| |WHERE t1.amount > 1000 | ||
| |GROUP BY t1.id, t1.address | ||
| |""".stripMargin | ||
|
|
||
| val rowsNoPushdown = withSQLConf(SQLConf.DATA_SOURCE_V2_JOIN_PUSHDOWN.key -> "false") { | ||
| sql(sqlQuery).collect().toSeq | ||
| } | ||
|
|
||
| assert(rowsNoPushdown.nonEmpty) | ||
|
|
||
| withSQLConf(SQLConf.DATA_SOURCE_V2_JOIN_PUSHDOWN.key -> "true") { | ||
| val df = sql(sqlQuery) | ||
| checkJoinPushed(df) | ||
| checkAggregateRemoved(df, supportsAggregatePushdown) | ||
| checkAnswer(df, rowsNoPushdown) | ||
| } | ||
| } | ||
|
|
||
| test("Test multi-way join with function in join condition") { | ||
| val sqlQuery = | ||
| s""" | ||
| |SELECT a.id, c.address, d.address, a.amount | ||
| |FROM $catalogAndNamespace.$casedJoinTableName1 a | ||
| |JOIN $catalogAndNamespace.$casedJoinTableName1 c | ||
| | ON a.address = c.address | ||
| |JOIN $catalogAndNamespace.$casedJoinTableName1 d | ||
| | ON LOWER(a.address) = LOWER(d.address) | ||
| |WHERE a.amount >= 1000 | ||
| |""".stripMargin | ||
|
|
||
| val rowsNoPushdown = withSQLConf(SQLConf.DATA_SOURCE_V2_JOIN_PUSHDOWN.key -> "false") { | ||
| sql(sqlQuery).collect().toSeq | ||
| } | ||
|
|
||
| assert(rowsNoPushdown.nonEmpty) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These asserts don't seem to be doing much at the moment, please consider removing them.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added it as additional guard against modifications on test dataset, so I would like to preserve some guard against that (to make our test still relevant in the future), it is not requirement ofc. Do you maybe suggest we make assertion on entire result then, or asserting on number of rows, etc? Or totally removing it? |
||
|
|
||
| withSQLConf(SQLConf.DATA_SOURCE_V2_JOIN_PUSHDOWN.key -> "true") { | ||
| val df = sql(sqlQuery) | ||
| checkJoinPushed(df) | ||
| checkAnswer(df, rowsNoPushdown) | ||
| } | ||
| } | ||
|
|
||
| test("Test sort limit on top of join is pushed down") { | ||
| val sqlQuery = s""" | ||
| |SELECT min(a.id + b.id), a.id, b.id | ||
|
|
||
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.
Same as https://github.com/apache/spark/pull/57714/changes#r3703572467.