Additional tests for broadcast hash join #313
Conversation
|
build |
|
|
||
| test("broadcast hint isn't propagated after a join") { | ||
| import spark.sqlContext.implicits._ | ||
| spark.conf.set("spark.sql.autoBroadcastJoinThreshold", "-1") |
There was a problem hiding this comment.
Won't setting configs on the shared spark session be problematic with the potential of config settings from one test leaking into other tests? That's why Spark's tests use something like withSQLConf to only temporary change configurations for a specific test.
|
build |
|
Addressed review comments. Ready for second round of review. |
| @@ -97,4 +99,45 @@ class JoinsSuite extends SparkQueryCompareTestSuite { | |||
| mixedDfWithNulls, mixedDfWithNulls, sortBeforeRepart = true) { | |||
| (A, B) => A.join(B, A("longs") === B("longs"), "LeftAnti") | |||
| } | |||
|
|
|||
| test("broadcast hint isn't propagated after a join") { | |||
There was a problem hiding this comment.
How are these integration tests? They don't fit with that model, and look much more like "unit" tests. Could you please move them to the tests directory instead of integration_tests
There was a problem hiding this comment.
Thanks @revans2 for reviewing. My bad, since all the join tests were in this file, I added these here as well not realizing these do not qualify here. I have moved these tests to a new file under tests directory. Please take a look and let me know if it's okay to keep there or should be put in any existing file.
|
build |
|
The concerns for @jlowe have been addressed so I think we can merge this in now instead of waiting for him. |
* Additional tests for broadcast hash join * addressed review comments * Moved these tests under tests/ directory Co-authored-by: Niranjan Artal <nartal>
* Additional tests for broadcast hash join * addressed review comments * Moved these tests under tests/ directory Co-authored-by: Niranjan Artal <nartal>
Signed-off-by: spark-rapids automation <70000568+nvauto@users.noreply.github.com>
This is part of audit work #237 . Took a look at Spark tests that could be ported over to spark-rapids plugin. Most of them are not required. Have added tests that seemed relevant. Metrics related tests can be added in #198. Please take a look and comment if these tests could be added.