[SPARK-33084][SQL][TEST][FOLLOWUP] Add ResetSystemProperties trait to SQLQuerySuite to avoid clearing ivy.home

[xkrogen]

What changes were proposed in this pull request?

Add the ResetSystemProperties trait to SQLQuerySuite so that system property changes made by any of the tests will not affect other suites/tests. Specifically, the system property changes made by SPARK-33084: Add jar support Ivy URI in SQL -- jar contains udf class are targeted here (which sets and then clears ivy.home).

Why are the changes needed?

PR #29966 added a new test case that adjusts the ivy.home system property to force Ivy to resolve an artifact from a custom location. At the end of the test, the value is cleared. Clearing the value meant that, if a custom value of ivy.home was configured externally, it would not apply for tests run after this test case.

Does this PR introduce any user-facing change?

No, this is only in tests.

How was this patch tested?

Existing unit tests continue to pass, whether or not spark.jars.ivySettings is configured (which adjusts the behavior of Ivy w.r.t. handling of ivy.home and ivy.default.ivy.user.dir properties).

[xkrogen]

cc folks from PR #29966: @viirya @maropu @wangyum @HyukjinKwon @dongjoon-hyun

[SparkQA]

Test build #135604 has finished for PR 31694 at commit 0c64be6.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[maropu]

Looks like a valid failure, so could you check it?

[xkrogen]

Hm. I'm having trouble running the CliSuite locally; when I run it via build/mvn, I get failures around Jackson version mismatch.

I don't really feel the failure is related; it is a timeout being exceeded in the sql/hive-thriftserver module, but my PR is a test-only change in the sql/core module. Of course you never know, but it seems unlikely.

Is there any good way for me to re-trigger the tests? Should I just push an empty commit?

[maropu]

Is there any good way for me to re-trigger the tests? Should I just push an empty commit?

Yea, can you push an empty commit?

[maropu]

retest this please

[SparkQA]

Test build #135616 has finished for PR 31694 at commit 89c88e8.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[xkrogen]

The CliSuite test passed in the new run, but the modified SQLQuerySuite test failed. Not sure why that wasn't showing up in the first test execution.

I dug a bit deeper and it turns out what I said in the description wasn't quite right (will update now). The issue is that the behavior of IvySettings#getDefaultIvyUserDir is different from that of IvySettings#load when it comes to ivy.home and ivy.default.ivy.user.dir. For getDefaultIvyUserDir, only ivy.home is used, even though the logs claim it is using ivy.default.ivy.user.dir:

    public synchronized File getDefaultIvyUserDir() {
        if (defaultUserDir == null) {
            if (getVariable("ivy.home") != null) {
                setDefaultIvyUserDir(Checks.checkAbsolute(getVariable("ivy.home"), "ivy.home"));
                Message.verbose("using ivy.default.ivy.user.dir variable for default ivy user dir: "
                        + defaultUserDir);
            } else {
                setDefaultIvyUserDir(new File(System.getProperty("user.home"), ".ivy2"));
                Message.verbose("no default ivy user dir defined: set to " + defaultUserDir);
            }
        }
        return defaultUserDir;
    }

But for load, ivy.default.ivy.user.dir is checked, and if present, it is used to set the value ivy.home (within setDefaultIvyUserDir):

    public synchronized void load(File settingsFile) throws ParseException, IOException {
        ...
        if (getVariable("ivy.default.ivy.user.dir") != null) {
            setDefaultIvyUserDir(Checks.checkAbsolute(getVariable("ivy.default.ivy.user.dir"),
                "ivy.default.ivy.user.dir"));
        } else {
            getDefaultIvyUserDir();
        }
        ...
    }

So for the new IvySettings case, ivy.home takes precedence, but for the (new IvySettings).load(...) case, ivy.default.ivy.user.dir takes precedence. In our environment we configure a custom Ivy settings file so I was only testing the latter case initially.

I have updated the patch to leave the system property as ivy.home. The test will still break if spark.jars.ivySettings and ivy.default.ivy.user.dir are used in tandem, but this should be a corner case and I don't think there is a way to resolve it further without modifying Ivy itself.

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40254/

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40254/

[SparkQA]

Test build #135672 has finished for PR 31694 at commit 840b630.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[maropu]

cc: @HyukjinKwon

[HyukjinKwon]

LGTM2

[HyukjinKwon]

Merged to master

[xkrogen]

Thanks @HyukjinKwon and @maropu ! Much appreciated.