-
Notifications
You must be signed in to change notification settings - Fork 228
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
refine UT framework to promote GPU evaluation, Part 1 #10851
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 |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Copyright (c) 2024, NVIDIA CORPORATION. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
/*** spark-rapids-shim-json-lines | ||
{"spark": "330"} | ||
spark-rapids-shim-json-lines ***/ | ||
package org.apache.spark.sql.rapids.suites | ||
|
||
import org.apache.spark.sql.catalyst.expressions.JsonExpressionsSuite | ||
import org.apache.spark.sql.rapids.utils.{RapidsJsonConfTrait, RapidsTestsTrait} | ||
|
||
class RapidsJsonExpressionsSuite | ||
extends JsonExpressionsSuite with RapidsTestsTrait with RapidsJsonConfTrait {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/* | ||
* Copyright (c) 2024, NVIDIA CORPORATION. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
/*** spark-rapids-shim-json-lines | ||
{"spark": "330"} | ||
spark-rapids-shim-json-lines ***/ | ||
package org.apache.spark.sql.rapids.utils | ||
|
||
import org.scalatest.{BeforeAndAfterAll, Suite} | ||
|
||
import org.apache.spark.sql.internal.SQLConf | ||
|
||
trait RapidsJsonConfTrait extends BeforeAndAfterAll { this: Suite => | ||
override def beforeAll(): Unit = { | ||
super.beforeAll() | ||
SQLConf.get.setConfString("spark.rapids.sql.expression.JsonTuple", true.toString) | ||
SQLConf.get.setConfString("spark.rapids.sql.expression.GetJsonObject", true.toString) | ||
SQLConf.get.setConfString("spark.rapids.sql.expression.JsonToStructs", true.toString) | ||
SQLConf.get.setConfString("spark.rapids.sql.expression.StructsToJson", true.toString) | ||
} | ||
|
||
override def afterAll(): Unit = { | ||
SQLConf.get.unsetConf("spark.rapids.sql.expression.JsonTuple") | ||
SQLConf.get.unsetConf("spark.rapids.sql.expression.GetJsonObject") | ||
SQLConf.get.unsetConf("spark.rapids.sql.expression.JsonToStructs") | ||
SQLConf.get.unsetConf("spark.rapids.sql.expression.StructsToJson") | ||
super.afterAll() | ||
} | ||
Comment on lines
+35
to
+41
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. It is not how I thought my previous comment would be addressed. Should not beforeAll and afterAll largely be the same implementation as in especially the cleanup after?
And we should just make sure that we reuse for new UT and 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. RapidsJsonConfTrait is an extra trait to inject more json related configs, and it will call the beforeAll/afterAll methods in its parent traits ( because of trait linerlization) 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 am talking about the fact that we already have the code cleaning out SparkSession via 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. As discussed with Gera offline, we'll use #10886 to track this |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,6 +121,12 @@ object RapidsSQLTestsBaseTrait { | |
"org.apache.spark.sql.rapids.ExecutionPlanCaptureCallback") | ||
.set("spark.sql.warehouse.dir", warehouse) | ||
.set("spark.sql.cache.serializer", "com.nvidia.spark.ParquetCachedBatchSerializer") | ||
.set("spark.sql.session.timeZone", "UTC") | ||
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. Do we really want to force UTC in the tests? We have partial support for timezones and it's likely more will be added later. Typically what we've done in the past is leave it up to the environment setup to specify the timezone desired to be tested (e.g.: CI scripts will set the TZ environment variable or other JVM settings to setup the timezone) rather than have the tests smash the timezone directly. That way the tests can be run across multiple timezones by changing the environment before running the tests. Comment applies to a couple of other places below where the timezone is being smashed with UTC. 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. the consideration of spark.sql.session.timeZone is explained in previous comment 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. @GaryShen2008 told me that they're already suggesting our customer to set timezone to UTC to facilitate GPU execution. So at least we're not testing a fictitious scenario 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. We should enable non-UTC test cases later, but in the first step, setting UTC here is to enable us testing in a fixed config which we support now. It can help us to detect the failure in UTC case. Without this UTC setting, the test cases may pass because of fallback to CPU. Currently we haven't been able to detect the wrong fallback in UT. 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'm not sure why we're not setting UTC in the environment of the test setup, which lets us reuse these tests. If we're going to leave this as-is, there needs to be a TODO comment linking to a tracking issue to fix this. 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 opened a TODO issue for this. #10874, will add it to code's comment. |
||
.set("spark.rapids.sql.explain", "ALL") | ||
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. This will make the tests very verbose. Intentional, or debug code accidentally left in? 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. both are Intentional.
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'm not questioning that we need to control the timezone for these tests, but rather the discussion is about how to control the timezone. Normally we don't want hardcoding like this, because now the test cannot check if we're doing the right thing in any timezone other that UTC, and like I said, we do have some timezone support with more on the way. Hardcoding this precludes using these tests to verify we're doing the right thing in any other timezone. Having the test environment setup script control that makes the test reusable. |
||
// uncomment below config to run `strict mode`, where fallback to CPU is treated as fail | ||
// .set("spark.rapids.sql.test.enabled", "true") | ||
// .set("spark.rapids.sql.test.allowedNonGpu", | ||
// "SerializeFromObjectExec,DeserializeToObjectExec,ExternalRDDScanExec") | ||
.setAppName("rapids spark plugin running Vanilla Spark UT") | ||
|
||
conf | ||
|
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.
I'm not sure we need this at all. The foldable check was put in place a long time ago when each expression had to have explicit support for scalar values. We refectored the code a while ago and removed that. Just the fact that it works for testing means that the refactor was correct and I think we can just delete the check entirely.
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.
should I do it in this PR or in another separate one?
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.
I am fine with doing it later as this is an internal config, and it might be a large-ish change. But at the same time I don't want to forget about it.
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.
this issue will be tracked in #10887