Skip to content

[SPARK-12629] [SparkR] Fixes for DataFrame saveAsTable method #10580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

[SPARK-12629] [SparkR] Fixes for DataFrame saveAsTable method #10580

wants to merge 9 commits into from

Conversation

@NarineK
Copy link
Contributor

@NarineK NarineK commented Jan 4, 2016

I've tried to solve some of the issues mentioned in: https://issues.apache.org/jira/browse/SPARK-12629
Please, let me know what do you think.
Thanks!

@SparkQA
Copy link

SparkQA commented Jan 4, 2016

Test build #48692 has finished for PR 10580 at commit 62872f1.

  • This patch fails R style tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 5, 2016

Test build #48697 has finished for PR 10580 at commit d55cc13.

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

@felixcheung
Copy link
Member

felixcheung commented Jan 5, 2016

I think the direction we are heading is to have multiple sqlContext sessions and be able to specify which one to use, instead of automatically picking one?

sun-rui
sun-rui reviewed Jan 5, 2016
View reviewed changes
R/pkg/R/DataFrame.R
Copy link
Contributor

@sun-rui sun-rui Jan 5, 2016

Choose a reason for hiding this comment

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

I think the same problem exists in write.df()? Could you extract the following logic into a util function, and update write.df() also to use it?

Copy link
Member

@felixcheung felixcheung Jan 5, 2016

Choose a reason for hiding this comment

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

See #9192 :)

Copy link
Contributor Author

@NarineK NarineK Jan 5, 2016

Choose a reason for hiding this comment

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

@felixcheung , @sun-rui , @shivaram : that's a good idea, I can use "getSqlContext()" method proposed in https://github.com/apache/spark/pull/9192/files#diff-b11442485f6b77bf47b58b4747321638R36

I guess I need to wait that pull request to go in and then I can merge it with mine .

Copy link
Contributor Author

@NarineK NarineK Jan 5, 2016

Choose a reason for hiding this comment

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

@felixcheung, it seems that you have it for saveAsTable too. After merge I can add my test cases ...

@sun-rui
Copy link
Contributor

sun-rui commented Jan 5, 2016

@felixcheung, as we discussed before, I don't think it makes sense to support multiple sessions in SparkR now. Let's keep the current way until there is need to support it.

sun-rui
sun-rui reviewed Jan 5, 2016
View reviewed changes
R/pkg/R/DataFrame.R Outdated
Copy link
Contributor

@sun-rui sun-rui Jan 5, 2016

Choose a reason for hiding this comment

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

Please use df.write.saveAsTable() instead of deprecated df.saveAsTable().

Copy link
Contributor

@sun-rui sun-rui Jan 5, 2016

Choose a reason for hiding this comment

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

this is done in #10584.

Copy link
Member

@felixcheung felixcheung Jan 5, 2016

Choose a reason for hiding this comment

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

Yes, let's merge with PR #10584 to unblock SparkR

@shivaram
Copy link
Contributor

shivaram commented Jan 6, 2016

@NarineK FYI #10584 has been merged. If you rebase / merge with master you should be able to see the changes.

@NarineK
Copy link
Contributor Author

NarineK commented Jan 6, 2016

Thanks for letting us know @shivaram
How about the #9192 ?
I was thinking to reuse getSqlContext() utility method

@sun-rui
Copy link
Contributor

sun-rui commented Jan 7, 2016

I think we may need more discussion on #9192, so let's merge this first.

@SparkQA
Copy link

SparkQA commented Jan 7, 2016

Test build #48951 has finished for PR 10580 at commit 7de1894.

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

@SparkQA
Copy link

SparkQA commented Jan 7, 2016

Test build #48952 has finished for PR 10580 at commit faa4971.

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

@NarineK
Copy link
Contributor Author

NarineK commented Jan 7, 2016

I see. I've pushed all my changes and left the piece of code which retrieves the context in case source is not passed by the argument. Later, you can replace it with the getContext utility method.

@shivaram
Copy link
Contributor

shivaram commented Jan 19, 2016

LGTM. Thanks @NarineK -- @sun-rui Do you have any more comments on this ?

@felixcheung
Copy link
Member

felixcheung commented Jan 20, 2016

looks fine to me

@sun-rui
Copy link
Contributor

sun-rui commented Jan 20, 2016

@NarineK, could you update the write.df() function as well?

@NarineK
Copy link
Contributor Author

NarineK commented Jan 21, 2016

updated write.df and saveDF

@SparkQA
Copy link

SparkQA commented Jan 21, 2016

Test build #49840 has finished for PR 10580 at commit fdd38c5.

  • This patch fails R style tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 21, 2016

Test build #49884 has finished for PR 10580 at commit 3c24314.

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

@NarineK
Copy link
Contributor Author

NarineK commented Jan 21, 2016

added ! Let me know what do you think!

@sun-rui
Copy link
Contributor

sun-rui commented Jan 22, 2016

LGTM

@shivaram
Copy link
Contributor

shivaram commented Jan 22, 2016

Merging this to master.

@asfgit asfgit closed this in 8a88e12 Jan 22, 2016
@shivaram
Copy link
Contributor

shivaram commented Feb 2, 2016

@sun-rui @felixcheung I'm going to backport this to branch-1.6 so that the bug fix is available in 1.6.1. Let me know if you think there is a problem with it

asfgit pushed a commit that referenced this pull request Feb 2, 2016
@NarineK @shivaram
[SPARK-12629][SPARKR] Fixes for DataFrame saveAsTable method
53f518a 
I've tried to solve some of the issues mentioned in: https://issues.apache.org/jira/browse/SPARK-12629
Please, let me know what do you think.
Thanks!

Author: Narine Kokhlikyan <[email protected]>

Closes #10580 from NarineK/sparkrSavaAsRable.

(cherry picked from commit 8a88e12)
Signed-off-by: Shivaram Venkataraman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@NarineK @SparkQA @felixcheung @sun-rui @shivaram