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

Compare Java and Scala BigDecimal with tolerance #213

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nightscape
Copy link
Contributor

I noticed that for java.math.BigDecimal no tolerance comparison is done in DataFrameSuite.approxEquals and scala.math.BigDecimal is missing completely.
Is there a reason for that?
If there isn't I would add corresponding test cases along with this change.

@codecov-io
Copy link

codecov-io commented Nov 8, 2017

Codecov Report

Merging #213 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #213   +/-   ##
=======================================
  Coverage   86.36%   86.36%           
=======================================
  Files          46       46           
  Lines        1005     1005           
  Branches       89       86    -3     
=======================================
  Hits          868      868           
  Misses        116      116           
  Partials       21       21
Flag Coverage Δ
#python 85.87% <ø> (ø) ⬆️
#scala 70.76% <0%> (-0.27%) ⬇️
Impacted Files Coverage Δ
...holdenkarau/spark/testing/DataFrameSuiteBase.scala 0% <0%> (ø) ⬆️

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 c0f75d6...2e3531e. Read the comment docs.

@nightscape nightscape force-pushed the improve_approx_equals branch 4 times, most recently from 7dd1961 to 088792a Compare November 9, 2017 09:27
@nightscape
Copy link
Contributor Author

I added tests for the 2.0 branch, but didn't find an existing 1.3 tests where I could add the BigDecimal case.
Also the stuff that codacy complains about is because I used the existing code style in the 1.3 case.
Are you OK with leaving it as is?

@holdenk
Copy link
Owner

holdenk commented Nov 10, 2017

So if the relative tolerance for bigdecimal support only exists in 2+ we should only test it in 2+, but the style issues are fine.

@holdenk
Copy link
Owner

holdenk commented Nov 25, 2017

If you've got the time to move the new test code from 1.3 to 2+ that would be great, but if not let me know and I can do that part.

@holdenk
Copy link
Owner

holdenk commented Jan 11, 2018

Gentle follow up. If I don't here back I'll assume your busy and do the last fix :)

@nightscape
Copy link
Contributor Author

Hi @holdenk,
my second child was born a few days ago and I won't get to do much coding in the next time 😉
If you could take over the last fix that would be great!

Thanks a lot!

@holdenk
Copy link
Owner

holdenk commented Jan 18, 2018

Ok I'll take this on and try and fix it after Spark 2.3 goes out so we can have it with that release :)

@holdenk
Copy link
Owner

holdenk commented Jan 18, 2018

Also congratulations!!!!!!!!!!!!!!!!!!

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.

3 participants