[SPARK-1455] [SPARK-3534] [Build] When possible, run SQL tests only.

[nchammas]

If the only files changed are related to SQL, then only run the SQL tests.

This patch includes some cosmetic/maintainability refactoring. I would be more than happy to undo some of these changes if they are inappropriate.

We can accept this patch mostly as-is and address the immediate need documented in SPARK-3534, or we can keep it open until a satisfactory solution along the lines discussed here is reached.

Note: I had to hack this patch up to test it locally, so what I'm submitting here and what I tested are technically different.

[SparkQA]

QA tests have started for PR 2420 at commit f9e23f6.

• This patch merges cleanly.

[SparkQA]

QA tests have started for PR 2420 at commit db3fa2d.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2420 at commit f9e23f6.

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

[SparkQA]

QA tests have finished for PR 2420 at commit db3fa2d.

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

[nchammas]

cc @marmbrus @pwendell Dunno if you guys got notified via JIRA of this pull request or not.

[marmbrus]

Yeah, thanks for working on this! :) What do you think about making a dummy commit in this PR that changes SQL to make sure everything tests as expected? You can commit, trigger jenkins, then revert.

In the past we've had bugs where we silently stop testing SQL and yet think everything is working.

[nchammas]

Since dev/run-tests has also been modified, Jenkins will still run all the tests.

If you want, I can have the dummy SQL commit also temporarily hack dev/run-tests to ignore the change to itself in the diff. That should trigger the SQL tests only. Note, though, that we would also be changing the script that we're trying to test. :)

[marmbrus]

oh hmmm, I guess its also okay to do a dummy follow up PR to make sure things are working.

@rxin do you have a second to look this over? I think its mostly changing code you wrote.

[rxin]

This looks good to me, but I'm not very good at reviewing bash code (who is?). I guess we can merge and watch closely the next few PRs to see if they are functioning properly.

At some point maybe we should consider writing these in Python instead of bash...

[nchammas]

At some point maybe we should consider writing these in Python instead of bash...

Amen.

So shall we go the dummy commit route first, and then revert, merge, and watch closely?

[rxin]

What's the dummy route?

[nchammas]

Dummy commit to change a SQL file and also hack dev/run-tests to ignore the changes to itself in the diff. That should trigger a SQL-only set of tests.

[rxin]

Yea that makes sense to me.

[nchammas]

I'm not very good at reviewing bash code (who is?)

Dunno if that was a rhetorical question (I'm no expert by any means!), but @pwendell has reviewed some of my bash patches in the past.

[marmbrus]

@pwendell @JoshRosen FYI, I'm going to merge this. I'll try and watch jenkins to see if anything breaks.

[nchammas]

Whoops! I was making a fix right now for a bug that I found in testing locally (and that I was going to confirm got fixed on Jenkins with the dummy commit.)

[nchammas]

$SBT_MAVEN_TEST_ARGS needs to be an array, otherwise this won't work as expected.

[nchammas]

@marmbrus I just pushed a fix to nchammas:selective-testing for this issue, and I also have the dummy commits cooking locally. Could we somehow revert the merge and reopen this PR so I can test it out on Jenkins?

[marmbrus]

Can you just make a follow up PR with the change? I don' think we have permission to reopen things and we can't roll back merges in apache only add new commits that revert them.

[nchammas]

OK, will do!