[SPARK-27445][SQL][TEST] Update SQLQueryTestSuite to process files ending with `.sql` #24333

dilipbiswal · 2019-04-10T05:27:34Z

What changes were proposed in this pull request?

While using vi or vim to edit the test files the .swp or .swo files are created and attempt to run the test suite in the presence of these files causes errors like below :

nfo] - subquery/exists-subquery/.exists-basic.sql.swp *** FAILED *** (117 milliseconds)
[info]   java.io.FileNotFoundException: /Users/dbiswal/mygit/apache/spark/sql/core/target/scala-2.12/test-classes/sql-tests/results/subquery/exists-subquery/.exists-basic.sql.swp.out (No such file or directory)
[info]   at java.io.FileInputStream.open0(Native Method)
[info]   at java.io.FileInputStream.open(FileInputStream.java:195)
[info]   at java.io.FileInputStream.<init>(FileInputStream.java:138)
[info]   at org.apache.spark.sql.catalyst.util.package$.fileToString(package.scala:49)
[info]   at org.apache.spark.sql.SQLQueryTestSuite.runQueries(SQLQueryTestSuite.scala:247)
[info]   at org.apache.spark.sql.SQLQueryTestSuite.$anonfun$runTest$11(SQLQueryTestSuite.scala:192)

~~This minor pr adds these temp files in the ignore list.~~
While computing the list of test files to process, only consider files with .sql extension. This makes sure the unwanted temp files created from various editors are ignored from processing.

How was this patch tested?

Verified manually.

SparkQA · 2019-04-10T07:05:01Z

Test build #104467 has finished for PR 24333 at commit 7fc82d7.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

dilipbiswal · 2019-04-10T07:05:38Z

retest this please

SparkQA · 2019-04-10T11:17:11Z

Test build #104472 has finished for PR 24333 at commit 7fc82d7.

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

attilapiros

I am happy somebody thinks about vim users (like me) and the change is fine but I think a solution which combines backlisting with whitelisting would be better: like every file with ".sql" extension and not backlisted.

This way much more editors which creates temporary files here could be safe to be used.
Even would be better for Vim itself:

The more .%.swX files are created whenever there are .%.swp and .%.swo and .%.swY files, where X and Y are characters defined in order by Vim.

And this condition is already satisfied:

$ find  sql/core/src/test/resources/sql-tests/inputs/ -type f | wc -l
     107
$ find  sql/core/src/test/resources/sql-tests/inputs/ -type f -name "*.sql" | wc -l
     107

What is your opinion?

srowen · 2019-04-10T13:35:49Z

These files aren't in the repo, and a .swp file only exists if you have the file open in vim. I don't think we should work around it for this reason. However, white-listing .sql files seems cleaner anyway so I'm OK with that.

dilipbiswal · 2019-04-10T23:49:06Z

@attilapiros @srowen White-listing .sql files sounds good to me :-)

srowen · 2019-04-11T01:07:43Z

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

  private def listFilesRecursively(path: File): Seq[File] = {
    val (dirs, files) = path.listFiles().partition(_.isDirectory)
-    files ++ dirs.flatMap(listFilesRecursively)
+    val filteredFiles = files.filter (_.getName matches validTestFileExtensionsRegEx)


Let's avoid postfix syntax.
Not that it matters, but it is probably a little more efficient to make the regex a regex above with .r and then match with that pattern here.

@srowen Thanks.. Sure... I will make the change. I wanted to confirm one thing.. so initially i had it as a regex as i thought, it may be easier for us to add more extensions o than just .sql in the future. But if we are going to stay with .sql, should we just do a endsWith check instead ? WDYT ?

All sounds fine. The regex could match more patterns later, or could be many regexes. Or could indeed just check for suffixes for now for simplicity and make it more complicated later.

SparkQA · 2019-04-11T03:54:52Z

Test build #104492 has finished for PR 24333 at commit 4c0f1bd.

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

SparkQA · 2019-04-11T04:04:44Z

Test build #104493 has finished for PR 24333 at commit eea0a25.

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

SparkQA · 2019-04-11T05:02:41Z

Test build #104494 has finished for PR 24333 at commit 78442e3.

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

dongjoon-hyun

Please update the PR title since this becomes a general approach. (Not specifically for .swp and .swo)

dilipbiswal · 2019-04-11T16:09:47Z

@dongjoon-hyun Thanks.. I have updated the title and description. Did we need a JIRA for this change ? Initially since i just added couple of cases to ignore list, i hadn't created a JIRA.

dongjoon-hyun · 2019-04-11T16:17:36Z

I updated the PR title, @dilipbiswal . Please create a JIRA with that title, too. Thanks!

attilapiros

pending tests otherwise LGTM

SparkQA · 2019-04-11T20:21:05Z

Test build #104519 has finished for PR 24333 at commit 7600b38.

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

dongjoon-hyun

+1, LGTM. Merged to master.
Thank you, @dilipbiswal , @srowen , @attilapiros , @HyukjinKwon !

dilipbiswal · 2019-04-11T22:56:57Z

Thanks a lot @dongjoon-hyun @srowen @HyukjinKwon @attilapiros

attilapiros reviewed Apr 10, 2019

View reviewed changes

dilipbiswal added 2 commits April 10, 2019 15:34

In SQLQueryTestSuite ignore .swp and .swo files from processinge

3c05e98

fix1

4c0f1bd

dilipbiswal force-pushed the dkb_sqlquerytest branch from 7fc82d7 to 4c0f1bd Compare April 10, 2019 23:48

dilipbiswal added 2 commits April 10, 2019 16:50

fix1

eea0a25

remove ds_store from blacklist

78442e3

srowen reviewed Apr 11, 2019

View reviewed changes

fix

d0eaec7

dongjoon-hyun reviewed Apr 11, 2019

View reviewed changes

endsWith

7600b38

dongjoon-hyun changed the title ~~[SQL][MINOR][TEST] In SQLQueryTestSuite ignore .swp and .swo files from processinge~~ [SQL][MINOR][TEST] Update SQLQueryTestSuite to process files endings with .sql Apr 11, 2019

dilipbiswal changed the title ~~[SQL][MINOR][TEST] Update SQLQueryTestSuite to process files endings with .sql~~ [SQL][MINOR][TEST] Update SQLQueryTestSuite to process files ending with .sql Apr 11, 2019

dilipbiswal changed the title ~~[SQL][MINOR][TEST] Update SQLQueryTestSuite to process files ending with .sql~~ [SPARK-27445][SQL][MINOR][TEST] Update SQLQueryTestSuite to process files ending with .sql Apr 11, 2019

attilapiros reviewed Apr 11, 2019

View reviewed changes

HyukjinKwon approved these changes Apr 11, 2019

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-27445][SQL][MINOR][TEST] Update SQLQueryTestSuite to process files ending with .sql~~ [SPARK-27445][SQL][TEST] Update SQLQueryTestSuite to process files ending with .sql Apr 11, 2019

dongjoon-hyun approved these changes Apr 11, 2019

View reviewed changes

dongjoon-hyun closed this in 5d8aee5 Apr 11, 2019

[SPARK-27445][SQL][TEST] Update SQLQueryTestSuite to process files ending with .sql #24333

[SPARK-27445][SQL][TEST] Update SQLQueryTestSuite to process files ending with .sql #24333

Uh oh!

Conversation

dilipbiswal commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 10, 2019

Uh oh!

dilipbiswal commented Apr 10, 2019

Uh oh!

SparkQA commented Apr 10, 2019

Uh oh!

attilapiros left a comment

Choose a reason for hiding this comment

Uh oh!

srowen commented Apr 10, 2019

Uh oh!

dilipbiswal commented Apr 10, 2019

Uh oh!

srowen Apr 11, 2019

Choose a reason for hiding this comment

Uh oh!

dilipbiswal Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen Apr 11, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 11, 2019

Uh oh!

SparkQA commented Apr 11, 2019

Uh oh!

SparkQA commented Apr 11, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dilipbiswal commented Apr 11, 2019

Uh oh!

dongjoon-hyun commented Apr 11, 2019

Uh oh!

attilapiros left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 11, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dilipbiswal commented Apr 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[SPARK-27445][SQL][TEST] Update SQLQueryTestSuite to process files ending with `.sql` #24333

[SPARK-27445][SQL][TEST] Update SQLQueryTestSuite to process files ending with `.sql` #24333

dilipbiswal commented Apr 10, 2019 •

edited

Loading

dilipbiswal Apr 11, 2019 •

edited

Loading