Skip to content

Conversation

@sun-rui
Copy link
Contributor

@sun-rui sun-rui commented Nov 19, 2015

@shivaram
Copy link
Contributor

cc @adrian555

@SparkQA
Copy link

SparkQA commented Nov 19, 2015

Test build #46316 has finished for PR 9835 at commit 8e36661.

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

@adrian555
Copy link
Contributor

I was going to work on this JIRA but was busy on other deliverable. :) Thanks @sun-rui

Copy link
Contributor

Choose a reason for hiding this comment

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

Since 'select' is an external API, should we not put the code for the function after the roxygen codes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a WIP, I will update doc and code once all test cases pass

@felixcheung
Copy link
Member

I think the goal is to have the same for not only select, but for agg and so on? Is there a way we generalize this? Also we shouldn't export the selectInternal methods

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest use 'drop' as the argument name instead since that is more R-like.

@adrian555
Copy link
Contributor

@felixcheung Yes, I agree. We should make this work for all dplyr-like functions.

Copy link
Member

Choose a reason for hiding this comment

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

these seem very expensive to have this per each call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, will have a cache for the environment per DataFrame.

@sun-rui
Copy link
Contributor Author

sun-rui commented Nov 24, 2015

I realize that this takes more effort than I expected as we'd better support specifying columns without $ for all DataFrame methods that take any argument of "Column" type. I will do more investigation.

@SparkQA
Copy link

SparkQA commented Nov 24, 2015

Test build #46600 has finished for PR 9835 at commit 17aa666.

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

@shivaram
Copy link
Contributor

@sun-rui Could we close this PR for now if its not active ? It'll just clear the PR queue.

@sun-rui
Copy link
Contributor Author

sun-rui commented Jan 20, 2016

Close it. Will send new PR later.

@sun-rui sun-rui closed this Jan 20, 2016
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.

5 participants