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 monotonically increasing id and inputFileName functions #229

Merged
merged 2 commits into from
Jan 23, 2018

Conversation

frosforever
Copy link
Contributor

Connects to #164

@codecov-io
Copy link

codecov-io commented Dec 21, 2017

Codecov Report

Merging #229 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   96.15%   96.16%   +<.01%     
==========================================
  Files          52       52              
  Lines         937      939       +2     
  Branches       15       14       -1     
==========================================
+ Hits          901      903       +2     
  Misses         36       36
Impacted Files Coverage Δ
...ataset/src/main/scala/frameless/TypedDataset.scala 93.89% <ø> (ø) ⬆️
...la/frameless/functions/NonAggregateFunctions.scala 100% <100%> (ø) ⬆️

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 30e77f5...9d47027. Read the comment docs.

def prop[A : TypedEncoder](xs: List[X1[A]])(implicit x2en: Encoder[X2[A, Long]]) = {
val ds = TypedDataset.create(xs)

val result = ds.withColumn[X2[A, Long]](monotonicallyIncreasingId[X1[A]]())
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work if we omitted X1[A] and just type monotonicallyIncreasingId()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, and it's terribly frustrating. Being that monotonicallyIncreasingId doesn't take a TypedColumn[T, A] like so many other functions do, there's no way that I know of to infer the T needed for the methods on TypedDataset[T] that require the Ts to align, e.g. select, withColumn etc.

Any ideas on that? Very open to suggestions here!

Copy link
Contributor

Choose a reason for hiding this comment

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

if you help the compiler by specifically adding .apply it can infer the type. I run into this few other times. For example, ds.withColumn[X2[A,Long]].apply(monotonicallyIncreasingId()) should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

The "problem" here is the existence of two overloaded methods withColumn. I am thinking maybe is a good idea to rename the withColumn that does replacement of existing column as withColumnReplaced. I tried it and with that the expression in question works without having to help the compiler with .apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I prefer renaming to withColumnReplaced over having to use types everywhere.

@@ -462,6 +461,72 @@ class NonAggregateFunctionsTests extends TypedDatasetSuite {
check(forAll(prop[Int] _))
}

test("inputFileName") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't even knew this function exists 👍 . I know that adding them to terget means they should be cleaned up on sbt clean, but I think it would be better if we just delete the temp files on exit. What do you think?

Copy link
Contributor Author

@frosforever frosforever Dec 22, 2017

Choose a reason for hiding this comment

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

Agreed! I'll try to get on this sometime in the next few days. Holidays etc getting in the way. Done

@OlivierBlanvillain
Copy link
Contributor

@imarios could you do another review pass on this PR?

@imarios imarios merged commit 33a39c6 into typelevel:master Jan 23, 2018
@frosforever frosforever deleted the i#164-missing-functions branch January 23, 2018 16:38
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