Skip to content
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

add when column method #220

Merged
merged 1 commit into from
Dec 8, 2017

Conversation

frosforever
Copy link
Contributor

@frosforever frosforever commented Dec 5, 2017

connects to #164

Differs from the spark implementation in that it requires an otherwise and will not default to null.

@codecov-io
Copy link

codecov-io commented Dec 5, 2017

Codecov Report

Merging #220 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
- Coverage    96.1%   95.87%   -0.23%     
==========================================
  Files          52       42      -10     
  Lines         924      849      -75     
  Branches       12       13       +1     
==========================================
- Hits          888      814      -74     
+ Misses         36       35       -1
Impacted Files Coverage Δ
...la/frameless/functions/NonAggregateFunctions.scala 100% <100%> (ø) ⬆️
...c/main/scala/frameless/TypedDatasetForwarded.scala 50% <0%> (-4.17%) ⬇️
...ataset/src/main/scala/frameless/TypedDataset.scala 93.7% <0%> (-0.2%) ⬇️
dataset/src/main/scala/frameless/TypedColumn.scala 100% <0%> (ø) ⬆️
...src/main/scala/frameless/ml/TypedTransformer.scala
...la/frameless/ml/feature/TypedVectorAssembler.scala
...cala/frameless/ml/feature/TypedStringIndexer.scala
...la/frameless/ml/internals/UnaryInputsChecker.scala
...scala/frameless/ml/internals/SelectorByValue.scala
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f249172...60f0df0. Read the comment docs.

Copy link
Contributor

@imarios imarios left a comment

Choose a reason for hiding this comment

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

LGTM!

object when {
/** Non-Aggregate function: Evaluates a list of conditions and returns one of multiple
* possible result expressions. If none match, otherwise is returned
*
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add an example here?

@frosforever
Copy link
Contributor Author

@imarios yet again thanks for your review! Comments addressed.

@imarios
Copy link
Contributor

imarios commented Dec 6, 2017

Hey @frosforever

So I played a bit with the code and came up with what I believe is a simplified implementation.

   ... rest of NonAggregateFunctions
   /** Non-Aggregate function: Evaluates a list of conditions and returns one of multiple
    * possible result expressions. If none match, otherwise is returned
    * {{{
    *   when(ds('boolField), ds('a))
    *     .when(ds('otherBoolField), lit(123))
    *     .otherwise(ds('b))
    * }}}
    * apache/spark
    */
  def when[T, A](condition: TypedColumn[T, Boolean], value: TypedColumn[T, A]): When[T, A] =
    new When[T, A](untyped.when(condition.untyped, value.untyped))
}

private[functions] class When[T, A](untypedC: Column) {
  def when(condition: TypedColumn[T, Boolean], value: TypedColumn[T, A]): When[T, A] = new When[T, A](
    untypedC.when(condition.untyped, value.untyped)
  )

  def otherwise(value: TypedColumn[T, A]): TypedColumn[T, A] =
    new TypedColumn[T, A](untypedC.otherwise(value.untyped))(value.uencoder)
}

What do you think?

@frosforever
Copy link
Contributor Author

@imarios Ha! that's nearly identical to a version I had earlier! Swapped it out for the one I pushed up for some reason that I can't seem to recall anymore...
Mine had an additional constructor instead

@OlivierBlanvillain
Copy link
Contributor

The #218 merge created some conflicts 😕

@imarios
Copy link
Contributor

imarios commented Dec 8, 2017

@frosforever can you resolve conflicts rebase/squash? I will merge as soon as everything clears from CI.

@frosforever
Copy link
Contributor Author

Thanks for the reviews @imarios and @OlivierBlanvillain! Conflicts resolved

@OlivierBlanvillain OlivierBlanvillain merged commit 73eada3 into typelevel:master Dec 8, 2017
@OlivierBlanvillain
Copy link
Contributor

@frosforever Thanks for the PR! 👍

@frosforever frosforever deleted the i#164-when branch December 8, 2017 23:10
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.

4 participants