Big Decimal Support #720

Activity

1. Jannis

   self-assigned this

   on Jan 29, 2019
2. Jannis

   added

   area/graphql

   area/runtime

   area/subgraphs

   area/store

   new feature

   on Jan 29, 2019
3. Jannis

   added this to the February 2019 milestone on Jan 29, 2019
4. Jannis

   added

   to do

   ready

   and removed

   to do

   on Jan 29, 2019
5. Jannis

   assigned

   leoyvens

   and unassigned

   Jannis

   on Feb 4, 2019
6. Jannis

   added

   to do

   and removed

   ready

   on Feb 4, 2019
7. leoyvens commented on Feb 13, 2019

   

   leoyvens

   on Feb 13, 2019

   Contributor

   More actions

   I agree that if you divide two BigInts you should get an exact result. Using f64 internally is a bad option since it will lose precison in intermediary operations, since this usually represents monetary values precision must be controled. So we should represent the number as a fraction.

   An open question is how do we represent this in graphql? Some options:

   • Return a fraction string and let frontend get as much precision as it wants.
   • Approximate to an f64 so we can use the native graphql type. This gets us around 15 digits of decimal precision.
   • Aproximate to a decimal string so we can get more precision than an f64, but how much is enough? 100 decimal places? Make it user configurable?
8. 15 remaining items

   Load more

9. leoyvens commented on Feb 27, 2019

   

   leoyvens

   on Feb 27, 2019

   Contributor

   More actions

   Oh wow we support floats in the store (I was assuming we didn't). In light of this whole discussion, I suppose we should only have one type for non-integer numbers, and we should drop support for f32 and support only a new fixed-point BigDecimal.
10. davekay100 commented on Feb 27, 2019

    

    davekay100

    on Feb 27, 2019

    ContributorAuthor

    More actions

    We only refer to float here in graph-ts index.ts:

    export enum ValueKind {
      STRING = 0,
      INT = 1,
      FLOAT = 2,
      BOOL = 3,
      ARRAY = 4,
      NULL = 5,
      BYTES = 6,
      BIGINT = 7,
    }
    

    But we never write any functions to handle setting or getting Float. Since no one is dependant on f32 right now, I agree with what you are saying leo, that we should stick with just BigDecimal
11. Jannis

    changed the title [-]Add big decimal support[/-] [+]Big Decimal Support[/+] on Mar 4, 2019
12. leoyvens commented on Mar 11, 2019

    

    leoyvens

    on Mar 11, 2019

    Contributor

    More actions

    Implementation plan:

    In graph-node, add a BigDecimal scalar graphql type, with all the filters that BigInt has. Coerce it from input strings, ints or floats. Add a store::Value::BigDecimal variant, with a bigdecimal as the backing implementation. Store it in the entity json as a string, casting to ::numeric for filtering, same as we do for BigInt.

    In graph-ts, add a BigDecimal class with a BigInt field for the digits and an i64 for the scale (decimal expoent). Have a from_big_int constructor and the math operators (exported from the node). Change BigInt division from returning a BigInt to returning a BigDecimal.

    In graph-cli, add BigDecimal to validation and to schema codegen.
13. Jannis commented on Mar 11, 2019

    

    Jannis

    on Mar 11, 2019

    Contributor

    More actions

    @leodasvacas The plan looks good to me! 👍
14. leoyvens commented on Mar 15, 2019

    

    leoyvens

    on Mar 15, 2019

    Last edited by leoyvens

    Contributor

    More actions

    A better organized plan:

    Requirements

    • Have a fixed-point decimal type.
    • Integrate that type fully with our graph-cli tool, our graph-ts library and our GraphQL schema.

    Proposed solution

    • Call the type BigDecimal.
    • Have parity with BigInt in graphql, with the added ability of coercing from floats.
    • Use the bigdecimal in the Rust implementation.
    • Store it as a string in the entity json, casting to postgres numeric when necessary.
    • In graph-ts, represent it as a class with a BigInt field for the digits and an i64 for the scale (decimal expoent), with a from_big_int constructor, a from_float constructor and a parse function. Have same operations as BigInt. Change BigInt division from returning a BigInt to returning a BigDecimal.

    Tasks

    • Graph Node: Add BigDecimal scalar graphql type, with appropriate filters and coercions.
    • Graph Node: Add BigDecimal store value type, and implement its operations in the postgres store.
    • Graph CLI: Add BigDecimal to validation and schema codegen.
    • Graph TS: Implement the BigDecimal class and its operations.
    • Graph Node: Implement runtime support for the BigDecimal AS class.
15. Jannis commented on Mar 18, 2019

    

    Jannis

    on Mar 18, 2019

    Last edited by Jannis

    Contributor

    More actions

    I only have two comments about the plan (which looks great):

    1. In graph-ts, the methods would be fromBigInt(n: BigInt) and fromFloat(n: f64) instead of from_big_int and from_float.

    2. Changing the behavior of div// for BigInt might break existing subgraphs. I also think there should be a BigInt division method that returns a BigInt. Think 5 / 4 = 1 vs. 5 / 4.0 = 1.25. Can we check if we can overload the / operator so that we can do e.g.

       let n: BigInt = BigInt.fromI32(5) / BigInt.fromI32(4) // => 1 (as BigInt)
       let f: BigDecimal = BigInt.fromI32(5) / BigDecimal.fromBigInt(BigInt.fromI32(4)) // => 1.25 (as BigDecimal)

       That would be ideal. If we can't do that, it's maybe better to keep div as it is and add a new method to not break subgraphs.
16. Jannis commented on Mar 18, 2019

    

    Jannis

    on Mar 18, 2019

    Contributor

    More actions

    I touched base with the AS team regarding operator overloading. It's not currently possible.
17. leoyvens commented on Mar 18, 2019

    

    leoyvens

    on Mar 18, 2019

    Last edited by leoyvens

    Contributor

    More actions

    Thanks for the comments! Different languages take different approaches with division of integers. Since JS/TS will return a float, it seems reasonable to follow the convention. We should offer a asBigInt() to truncate the BigDecimal.
18. leoyvens

    added

    in progress

    pending review

    and removed

    to do

    in progress

    on Apr 3, 2019
19. leoyvens

    closed this as completedin #841on Apr 4, 2019
20. ghost removed

    pending review

    on Apr 4, 2019