-
Notifications
You must be signed in to change notification settings - Fork 138
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.
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
I#163 dataset drop #209
I#163 dataset drop #209
Conversation
Codecov Report
@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 95.71% 95.72% +0.01%
==========================================
Files 39 39
Lines 793 796 +3
Branches 11 13 +2
==========================================
+ Hits 759 762 +3
Misses 34 34
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -605,6 +606,29 @@ class TypedDataset[T] protected[frameless](val dataset: Dataset[T])(implicit val | |||
} | |||
} | |||
|
|||
def drop[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** Returns a new Dataset with a column dropped.
*
* {{{ maybe give an example? }}}
*
* apache/spark
*/
import org.scalacheck.Prop._ | ||
|
||
class DropTest extends TypedDatasetSuite { | ||
test("drop five columns") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could add a test dropping the only the first column, and another one dropping only the last column
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can make it more explicit by making separate tests but those two cases are already covered in this test by dropping 'a
which is the first column and then _4
etc which are the last ones
LGTM as well! Nice work @frosforever. Let us know when you add everything you want to add for the PR. Thank you! |
Thanks @imarios! I think this is done now. |
hmm, unclear to me why travis failed. it tests correctly locally. |
might be transient thing. restarting travis. |
Thanks for the PR! |
Connects to #163
Suffers from the same problem as #207