Uh oh!
There was an error while loading. Please reload this page.
[SPARK-58149][SQL][TESTS] Refactor some DSv2 tests to use SessionQueryTest - #56708
[SPARK-58149][SQL][TESTS] Refactor some DSv2 tests to use SessionQueryTest#56708fwc wants to merge 8 commits into
Conversation
cbde99f to
cb435caCompare
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 3 nits.
Clean test-only refactor onto the SessionQueryTest infra from #56190; checkAnswer/withTable correctly replace the custom connect helpers (CheckAnswerHelper already guards logicalPlan/queryExecution behind the classic-DataFrame check, so it stays Connect-safe). Three nits, all inline.
Nits: 3 minor items (see inline comments) — two refactor-leftover indentation slips (note scalafmt doesn't cover sql/core, so CI won't flag them) and one afterEach comment that overstates what the code does.
Also: the title still has a placeholder [SPARK-XXXXX]. Could you create a JIRA ticket and update the title/description with the real SPARK id before this is ready to merge?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
cloud-fan
left a comment
There was a problem hiding this comment.
2 addressed, 1 remaining, 0 new.
Thanks for the updates. Prior nit #1 (over-indentation in DSv2IncrementallyConstructedQueryTests) and nit #3 (the afterEach comment overstating a catalog reset) are both resolved.
Remaining (1 nit): nit #2 on DSv2CacheTableReadTests.scala is only partially fixed. The getTableCatalog[...] continuation was dedented correctly, but in the same (last) test the createTable( args — testIdent and new TableInfo.Builder() (~line 238) — still sit at 6 spaces, the same column as catalog.createTable(, instead of 8. Compare the correctly-indented sibling createTable earlier in the file (~line 197). See my prior inline thread. (Same scalafmt-doesn't-cover-sql/core caveat — CI won't flag it.)
Clean test-only refactor otherwise.
cloud-fan
commented
Jul 17, 2026
can you rebase to re-trigger CI? |
This reverts commit 176a528.
…/DataSourceV2DataFrameConnectSuite.scala Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>
d2c66be to
e9b6b70Compare…Test ### What changes were proposed in this pull request? This PR builds upon #56190 and refactors the existing classic/connect-targeting DSv2 tests to use `SessionQueryTest`. NOTE: this PR is stacked upon #56190 and WIP until that PR is merged and this PR is rebased. The actual changes are to the files in `sql/core/src/test/scala/org/apache/spark/sql/connector/` and to `sql/connect/server/src/test/scala/org/apache/spark/sql/connect/DataSourceV2DataFrameConnectSuite.scala` (currently ea62c55) ### Why are the changes needed? This PR reduces the amount of custom test helpers. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? This patch is test-only ### Was this patch authored or co-authored using generative AI tooling? No Closes#56708 from fwc/refactor-some-tests-to-sessionquerytest. Lead-authored-by: Matthis Gördel <matthis@posteo.de> Co-authored-by: Matthis Gördel <matthis.goerdel@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit b50454d) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
This PR builds upon #56190 and refactors the existing classic/connect-targeting DSv2 tests to use
SessionQueryTest.NOTE: this PR is stacked upon #56190 and WIP until that PR is merged and this PR is rebased. The actual changes are to the files in
sql/core/src/test/scala/org/apache/spark/sql/connector/and tosql/connect/server/src/test/scala/org/apache/spark/sql/connect/DataSourceV2DataFrameConnectSuite.scala(currently ea62c55)Why are the changes needed?
This PR reduces the amount of custom test helpers.
Does this PR introduce any user-facing change?
No
How was this patch tested?
This patch is test-only
Was this patch authored or co-authored using generative AI tooling?
No