[SPARK-32443][CORE] Use POSIX-compatible `command -v` in testCommandAvailable #29241

dongjoon-hyun · 2020-07-26T05:37:13Z

What changes were proposed in this pull request?

This PR aims to use command -v in non-Window operating systems instead of executing the given command.

Why are the changes needed?

command is POSIX-compatible
- POSIX.1-2017: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html
command is faster and safer than the direct execution
- command doesn't invoke another process.

scala> sys.process.Process("ls").run().exitValue()
LICENSE
NOTICE
bin
doc
lib
man
res1: Int = 0

The existing way behaves inconsistently.
- rm cannot be checked.

AS-IS

scala> sys.process.Process("rm").run().exitValue()
usage: rm [-f | -i] [-dPRrvW] file ...
       unlink file
res0: Int = 64

TO-BE

Welcome to Scala 2.13.3 (OpenJDK 64-Bit Server VM, Java 1.8.0_262).
Type in expressions for evaluation. Or try :help.
scala> sys.process.Process(Seq("sh", "-c", s"command -v ls")).run().exitValue()
/bin/ls
val res1: Int = 0

The existing logic is already broken in Scala 2.13 environment because it hangs like the following.

$ bin/scala
Welcome to Scala 2.13.3 (OpenJDK 64-Bit Server VM, Java 1.8.0_262).
Type in expressions for evaluation. Or try :help.

scala> sys.process.Process("cat").run().exitValue() // hang here.

Does this PR introduce any user-facing change?

No. Although this is inside main source directory, this is used for testing purpose.

$ git grep testCommandAvailable | grep -v 'def testCommandAvailable'
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("wc"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable(envCommand))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(!TestUtils.testCommandAvailable("some_nonexistent_command"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable(envCommand))
sql/core/src/test/scala/org/apache/spark/sql/IntegratedUDFTestUtils.scala:  private lazy val isPythonAvailable: Boolean = TestUtils.testCommandAvailable(pythonExec)
sql/core/src/test/scala/org/apache/spark/sql/IntegratedUDFTestUtils.scala:    if (TestUtils.testCommandAvailable(pythonExec)) {
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("python"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("echo | sed"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("/bin/bash"))

How was this patch tested?

Scala 2.12: Pass the Jenkins with the existing tests and one modified test.
Scala 2.13: Do the following manually. It should pass instead of hang.

$ dev/change-scala-version.sh 2.13
$ build/mvn test -pl core --am -Pscala-2.13 -Dtest=none -DwildcardSuites=org.apache.spark.rdd.PipedRDDSuite
...
Tests: succeeded 12, failed 0, canceled 0, ignored 0, pending 0
All tests passed.

dongjoon-hyun · 2020-07-26T06:01:33Z

Hi, @srowen .
This PR will fix PipedRDDSuite hang situation at Scala 2.13, but I'd like to have this fix in older branches, too.

$ git grep testCommandAvailable | grep -v 'def testCommandAvailable' | wc -l
      36

dongjoon-hyun · 2020-07-26T07:26:59Z

Retest this please

srowen

If it's test-only, sure that seems fine. That's great if this is why the spark-submit tests were failing.
These are all the occurrences?

I wonder if this will affect other places in non-test code though? hm.

dongjoon-hyun · 2020-07-26T14:56:09Z

Yes. Everything is test.

$ git grep testCommandAvailable | grep -v 'def testCommandAvailable'
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("wc"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable(envCommand))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(!TestUtils.testCommandAvailable("some_nonexistent_command"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable("cat"))
core/src/test/scala/org/apache/spark/rdd/PipedRDDSuite.scala:    assume(TestUtils.testCommandAvailable(envCommand))
sql/core/src/test/scala/org/apache/spark/sql/IntegratedUDFTestUtils.scala:  private lazy val isPythonAvailable: Boolean = TestUtils.testCommandAvailable(pythonExec)
sql/core/src/test/scala/org/apache/spark/sql/IntegratedUDFTestUtils.scala:    if (TestUtils.testCommandAvailable(pythonExec)) {
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala:    skip = !TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("python"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveScriptTransformationSuite.scala:    assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("echo | sed"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("/bin/bash"))
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala:      assume(TestUtils.testCommandAvailable("/bin/bash"))

dongjoon-hyun · 2020-07-26T19:26:08Z

For now, IntegratedUDFTestUtils.isPythonAvailable function seems to work differently.

In GitHub Action environment, it returns true.
In Jenkins environment, it returns false.

IntegratedUDFTestUtils.isPythonAvailable was false

dongjoon-hyun · 2020-07-26T19:46:26Z

It turns out that

python is used in Jenkins.
python3 is used in GitHub Action.

org.scalatest.exceptions.TestCanceledException: org.apache.spark.sql.IntegratedUDFTestUtils.pythonExec.equals("python3") was false

Let me take a look at that.

sql/core/src/test/scala/org/apache/spark/sql/execution/python/PythonUDFSuite.scala

sql/core/src/test/scala/org/apache/spark/sql/IntegratedUDFTestUtils.scala

…vailable

dongjoon-hyun · 2020-07-27T04:17:21Z

The PR is rebased to the master and reverted to the first commit.

HyukjinKwon

LGTM

dongjoon-hyun · 2020-07-27T04:18:47Z

Thank you, @HyukjinKwon !

dongjoon-hyun · 2020-07-27T05:26:24Z

Sorry guys. I updated this PR once more to add a fallback for Windows

cmd doesn't support command.
PowerShell has command, but -v is not supported.

For Windows, let's revisit after we get a complete run on Linux first. PR description is also updated.

dongjoon-hyun · 2020-07-27T06:27:29Z

I'll continue tomorrow because this should pass in both Jenkins/GitHub Action environment.

HyukjinKwon · 2020-07-27T15:34:27Z

@dongjoon-hyun, I debugged it and looks basically it fails as it can't find command for some reasons:

[info] org.apache.spark.sql.SQLQueryTestSuite *** ABORTED *** (121 milliseconds)
[info]   java.io.IOException: Cannot run program "command": error=2, No such file or directory
[info]   at java.lang.ProcessBuilder.start(ProcessBuilder.java:1048)
[info]   at scala.sys.process.ProcessBuilderImpl$Simple.run(ProcessBuilderImpl.scala:75)
[info]   at scala.sys.process.ProcessBuilderImpl$AbstractBuilder.run(ProcessBuilderImpl.scala:106)
[info]   at scala.sys.process.ProcessBuilderImpl$AbstractBuilder.run(ProcessBuilderImpl.scala:105)
[info]   at org.apache.spark.TestUtils$.$anonfun$testCommandAvailable$5(TestUtils.scala:250)
[info]   at scala.runtime.java8.JFunction0$mcI$sp.apply(JFunction0$mcI$sp.java:23)
[info]   at scala.util.Try$.apply(Try.scala:213)

I did a quick fix to try out and pushed here but feel free to revert if it doesn't look good. I just wanted to trigger the tests to show it works (considering the timezone between me and you guys)

srowen · 2020-07-27T15:35:56Z

command is a shell built-in, right? maybe that's the issue, but then I don't know the fix other than to try to launch a new shell to run this, but then, does that still work?

HyukjinKwon · 2020-07-27T15:42:33Z

I didn't check all the tests yet but at least some tests (at HyukjinKwon#14).. we could wait and see ..

dongjoon-hyun · 2020-07-27T15:50:46Z

AFAIK, command is built-in. Which environment did you get the log? Before rebasing, the first commit passed GitHub Action.

dongjoon-hyun · 2020-07-27T15:58:46Z

@HyukjinKwon . Thank you for your help, but new commits look weird to me. May I reset those to start from the beginning to reinvestigate?

I'll add your co-authorship later~

HyukjinKwon · 2020-07-27T15:59:43Z

I got it from GitHub Actions - HyukjinKwon#14. That was exactly why I was confused about the logs and the issue here. Feel free to revert @dongjoon-hyun. It was just quick try to show.

dongjoon-hyun · 2020-07-27T16:05:47Z

BTW, I'm try to catch up your trials. So, you want to use sh always because Process will invoke a shell in anyway. Did I understand correctly?

HyukjinKwon · 2020-07-27T16:13:17Z

Yup, basically yes. I just thought for some reasons command is located in /usr/bin and that's not in the path when JVM launches a process. I had to investigate more to clarify though. Feel free to revert. It was kind of in the middle of the debug.

SparkQA · 2020-07-27T18:27:15Z

Test build #126653 has finished for PR 29241 at commit c7154c9.

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

dongjoon-hyun · 2020-07-27T18:57:35Z

Thanks, @HyukjinKwon . I must admit that @HyukjinKwon 's final try is the best. I'll merge this with his authorship and my coauthorship. Thanks!

dongjoon-hyun · 2020-07-27T18:58:23Z

Oh, let me test with Scala 2.13 again.

dongjoon-hyun · 2020-07-27T19:00:40Z

Yes. It works without Hang. I'll update the PR description.

Welcome to Scala 2.13.3 (OpenJDK 64-Bit Server VM, Java 1.8.0_262).
Type in expressions for evaluation. Or try :help.
scala> sys.process.Process(Seq("sh", "-c", s"command -v ls")).run().exitValue()
/bin/ls
val res1: Int = 0

HyukjinKwon · 2020-07-28T00:23:47Z

Thank you @dongjoon-hyun and @srowen.

probot-autolabeler bot added CORE SQL labels Jul 26, 2020

dongjoon-hyun changed the title ~~[SPARK-32443][CORE] Use POSIX compatible command -v in testCommandAvailable~~ [SPARK-32443][CORE] Use POSIX-compatible command -v in testCommandAvailable Jul 26, 2020

This comment has been minimized.

Sign in to view

srowen reviewed Jul 26, 2020

View reviewed changes

probot-autolabeler bot added the PYTHON label Jul 26, 2020

This comment has been minimized.

Sign in to view

srowen reviewed Jul 26, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/python/PythonUDFSuite.scala Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

HyukjinKwon reviewed Jul 27, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/IntegratedUDFTestUtils.scala Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

dongjoon-hyun added 9 commits July 26, 2020 21:16

[SPARK-32443][CORE] Use POSIX compatible command -v in testCommandA…

789edde

…vailable

test

62e5e6e

Check pythonExec

7e472c7

a

5d6e149

fix typo

d9f5949

remove extra assumes

e549e19

always python3

dd0888d

test

1333a7f

clean up

46abc40

HyukjinKwon approved these changes Jul 27, 2020

View reviewed changes

Use the old behavior for Windows

acf87a2

empty commit

0bf0773

This comment has been minimized.

Sign in to view

Quick try

c7154c9

dongjoon-hyun closed this in c114066 Jul 27, 2020

[SPARK-32443][CORE] Use POSIX-compatible command -v in testCommandAvailable #29241

[SPARK-32443][CORE] Use POSIX-compatible command -v in testCommandAvailable #29241

Uh oh!

Conversation

dongjoon-hyun commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

dongjoon-hyun commented Jul 26, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

dongjoon-hyun commented Jul 26, 2020

Uh oh!

This comment has been minimized.

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 26, 2020

Uh oh!

This comment has been minimized.

dongjoon-hyun commented Jul 26, 2020

Uh oh!

This comment has been minimized.

dongjoon-hyun commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

This comment has been minimized.

dongjoon-hyun commented Jul 27, 2020

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 27, 2020

Uh oh!

dongjoon-hyun commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 27, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

HyukjinKwon commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented Jul 27, 2020

Uh oh!

HyukjinKwon commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Jul 27, 2020

Uh oh!

dongjoon-hyun commented Jul 27, 2020

Uh oh!

HyukjinKwon commented Jul 27, 2020

Uh oh!

dongjoon-hyun commented Jul 27, 2020

Uh oh!

HyukjinKwon commented Jul 27, 2020

Uh oh!

SparkQA commented Jul 27, 2020

Uh oh!

dongjoon-hyun commented Jul 27, 2020

Uh oh!

dongjoon-hyun commented Jul 27, 2020

Uh oh!

dongjoon-hyun commented Jul 27, 2020

Uh oh!

HyukjinKwon commented Jul 28, 2020

Uh oh!

[SPARK-32443][CORE] Use POSIX-compatible `command -v` in testCommandAvailable #29241

[SPARK-32443][CORE] Use POSIX-compatible `command -v` in testCommandAvailable #29241

dongjoon-hyun commented Jul 26, 2020 •

edited

Loading

dongjoon-hyun commented Jul 26, 2020 •

edited

Loading

dongjoon-hyun commented Jul 27, 2020 •

edited

Loading

HyukjinKwon commented Jul 27, 2020 •

edited

Loading

HyukjinKwon commented Jul 27, 2020 •

edited

Loading