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 support for several types #69

Merged
merged 11 commits into from
May 31, 2019
Merged

Add support for several types #69

merged 11 commits into from
May 31, 2019

Conversation

AdrianRaFo
Copy link
Contributor

@AdrianRaFo AdrianRaFo commented May 18, 2019

Add more instances to encode and decode the following Arrow types:

  • Option
  • Either
  • Tuple2
  • Tuple3

Also, support for kotlin types:

  • List
  • Map
  • Pair
  • Triple
  • All nullables

and finally, support for Java types:

  • BigDecimal
  • BigInteger
  • UUID

I have tested all these new types with complex models on the sample module

Note: If you note that an important type is missing and want to see it on the first release please let me know to add it here

@47degdev
Copy link
Contributor

47degdev commented May 18, 2019

Hood benchmark comparison:
✔️ Decoding (Threshold: 257.7455896237748)

Benchmark Value
master_benchmark 10916.990504635269
helios_benchmark 11130.893134319518

⚠️ Decodingfromraw (Threshold: 250.0)

Benchmark Value
master_benchmark 7232.359819202301
helios_benchmark 7093.866595814756

✔️ Parsing (Threshold: 250.0)

Benchmark Value
master_benchmark 22898.94171983691
helios_benchmark 22990.565607334767

Copy link
Contributor

@pakoito pakoito left a comment

Choose a reason for hiding this comment

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

Float point comparison is brrroken. May be broken on Arrow too, we need to check.

@nomisRev
Copy link
Contributor

nomisRev commented May 18, 2019

@pakoito I thought I ported this from Circe but I just checked and the Eq instance looks quite different there. The only issue istoBiggerDecimal, which we don't have in Kotlin.

implicit final val eqJsonNumber: Eq[JsonNumber] = Eq.instance {
    case (JsonLong(x), JsonLong(y))             => x == y
    case (JsonDouble(x), JsonDouble(y))         => java.lang.Double.compare(x, y) == 0
    case (JsonFloat(x), JsonFloat(y))           => java.lang.Float.compare(x, y) == 0
    case (JsonBigDecimal(x), JsonBigDecimal(y)) => x.compareTo(y) == 0
    case (a, b)                                 => a.toBiggerDecimal == b.toBiggerDecimal
}

PS: In Arrow it shouldn't be broken since we only have Eq for Kotlin's Number types which we can just redirect to the underlying equals.

@AdrianRaFo
Copy link
Contributor Author

I'm going to move the float comparison issue to this #70

@AdrianRaFo
Copy link
Contributor Author

I still need to fix the Map codec before merge

@AdrianRaFo AdrianRaFo added the work in progress This issue or pull request is not ready to be merged label May 24, 2019
@AdrianRaFo AdrianRaFo changed the title Add more instances Add support for several types May 24, 2019
@AdrianRaFo AdrianRaFo removed the work in progress This issue or pull request is not ready to be merged label May 28, 2019
@AdrianRaFo
Copy link
Contributor Author

This is ready for review now @nomisRev @pakoito @fedefernandez

Copy link
Contributor

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

@raulraja @pakoito @AdrianRaFo

Thoughts on my remark about the public API of JsNumber?

Copy link
Contributor

@fedefernandez fedefernandez left a comment

Choose a reason for hiding this comment

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

Left a minor comment, besides that looks good

Copy link
Contributor

@fedefernandez fedefernandez left a comment

Choose a reason for hiding this comment

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

👍

@AdrianRaFo AdrianRaFo merged commit d00dc2f into master May 31, 2019
@AdrianRaFo AdrianRaFo deleted the arf-add-more-instances branch May 31, 2019 15:46
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