Skip to content

Conversation

@ueshin
Copy link
Member

@ueshin ueshin commented Oct 18, 2018

What changes were proposed in this pull request?

Currently each test in SQLTest in PySpark is not cleaned properly.
We should introduce and use more @contextmanager to be convenient to clean up the context properly.

How was this patch tested?

Modified tests.

@SparkQA
Copy link

SparkQA commented Oct 18, 2018

Test build #97524 has finished for PR 22762 at commit 9f7901b.

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

@ueshin
Copy link
Member Author

ueshin commented Oct 18, 2018

Jenkins, retest this please.

@ueshin
Copy link
Member Author

ueshin commented Oct 18, 2018

cc @HyukjinKwon

@SparkQA
Copy link

SparkQA commented Oct 18, 2018

Test build #97526 has finished for PR 22762 at commit 9f7901b.

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

@HyukjinKwon
Copy link
Member

I like it!

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if the tests pass

"""
assert hasattr(self, "spark"), "it should have 'spark' attribute, having a spark session."

if len(databases) == 1 and isinstance(databases[0], (list, set)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @ueshin, how about we just allow database(a, b, ...) case only for now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like .. an assert by this condition all(map(lambda d: isinstance(d, str), databases)) should be good enough.

@SparkQA
Copy link

SparkQA commented Oct 18, 2018

Test build #97531 has finished for PR 22762 at commit c46693b.

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

@SparkQA
Copy link

SparkQA commented Oct 18, 2018

Test build #97536 has finished for PR 22762 at commit c405b0e.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 18, 2018

Test build #97537 has finished for PR 22762 at commit ed25864.

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

@HyukjinKwon
Copy link
Member

Merged to master.

def table(self, *tables):
"""
A convenient context manager to test with some specific tables. This drops the given tables
if exist when it exits.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait .. not a big deal but typo: if exist when it exits..

@asfgit asfgit closed this in e80f18d Oct 18, 2018
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
…e clean-up each test.

## What changes were proposed in this pull request?

Currently each test in `SQLTest` in PySpark is not cleaned properly.
We should introduce and use more `contextmanager` to be convenient to clean up the context properly.

## How was this patch tested?

Modified tests.

Closes apache#22762 from ueshin/issues/SPARK-25763/cleanup_sqltests.

Authored-by: Takuya UESHIN <[email protected]>
Signed-off-by: hyukjinkwon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants