Skip to content

Conversation

@felixcheung
Copy link
Member

@felixcheung felixcheung commented May 9, 2017

What changes were proposed in this pull request?

Change it to check for relative count like in this test https://github.com/apache/spark/blame/master/R/pkg/inst/tests/testthat/test_sparkSQL.R#L3355 for catalog APIs

How was this patch tested?

unit tests, this needs to combine with another commit with SQL change to check

@falaki
Copy link
Contributor

falaki commented May 9, 2017

@felixcheung this approach is fine, but I think it is better if unit tests do not leave any side-effects to begin with. In this case every test should clean up state before and after (similar to beforeAach())

@felixcheung
Copy link
Member Author

felixcheung commented May 9, 2017

that I agree completely, @falaki - looks like tests fail apparently when running Scala tests before running R tests

@felixcheung
Copy link
Member Author

felixcheung commented May 9, 2017

I think this might be the reason? @yhuai @gatorsmile @ueshin
Am I reading these right that these tables are created but never dropped?
2269155#diff-1e55698cc579cbae676f827a89c2dc2eR186

@yhuai
Copy link
Contributor

yhuai commented May 9, 2017

@felixcheung you are right. That is the problem.

@yhuai
Copy link
Contributor

yhuai commented May 9, 2017

@falaki's PR did not actually trigger that test.

@yhuai
Copy link
Contributor

yhuai commented May 9, 2017

lgtm

@felixcheung
Copy link
Member Author

hmm, spoke too soon I think - looks to me like all the withTable clause are in place and complete.
not sure what can be leaking through then..

@yhuai
Copy link
Contributor

yhuai commented May 9, 2017

i see. I think d4c1a9d is good. How about we get it checked in first (after jenkins passes)?

@felixcheung
Copy link
Member Author

right. I think it's a good way to decouple R tests from any earlier states and also not to mask the error/leak. I'll get that in when Jenkins pass (and see if I could figure out what is leaked)

@gatorsmile
Copy link
Member

How about #17908? It tries to reset the cataloged metadata objects and temporary objects.

@SparkQA
Copy link

SparkQA commented May 9, 2017

Test build #76612 has finished for PR 17905 at commit 1aa17d8.

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

@felixcheung
Copy link
Member Author

ok Jenkins passes, I'm going to merge this in since there are a bunch of PR failing because of this, even when they say it's up-to-date with master.
I'm going to investigate further though.

@felixcheung
Copy link
Member Author

merged to master/2.2

asfgit pushed a commit that referenced this pull request May 9, 2017
## What changes were proposed in this pull request?

Change it to check for relative count like in this test https://github.com/apache/spark/blame/master/R/pkg/inst/tests/testthat/test_sparkSQL.R#L3355 for catalog APIs

## How was this patch tested?

unit tests, this needs to combine with another commit with SQL change to check

Author: Felix Cheung <[email protected]>

Closes #17905 from felixcheung/rtabletests.

(cherry picked from commit b952b44)
Signed-off-by: Felix Cheung <[email protected]>
@SparkQA
Copy link

SparkQA commented May 9, 2017

Test build #76634 has started for PR 17905 at commit b37a760.

@asfgit asfgit closed this in b952b44 May 9, 2017
@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/76634/
Test FAILed.

liyichao pushed a commit to liyichao/spark that referenced this pull request May 24, 2017
## What changes were proposed in this pull request?

Change it to check for relative count like in this test https://github.com/apache/spark/blame/master/R/pkg/inst/tests/testthat/test_sparkSQL.R#L3355 for catalog APIs

## How was this patch tested?

unit tests, this needs to combine with another commit with SQL change to check

Author: Felix Cheung <[email protected]>

Closes apache#17905 from felixcheung/rtabletests.
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.

6 participants