Advice for adding rust_decimal support

[bheylin]

I'm looking to use Rhai for scripting in a financial application and I'm starting to research how I can add rust_decimal support to the runtime.

Best case scenario for me is that all decimal literals are stored as a Decimal type. In my case I want to disable floating point support (which is already possible with features gates) and enable Decimal support.

Does anyone have advice on the various approaches I can take?

[schungx]

You'll need to:

1) Add a new variant to Dynamic. You can copy floating point code. Beware that Dynamic is deliberately designed to be small, so if your decimal type is larger than 32 bits, put a Box around it unless you really care about calculation performance. You may need to measure whether Boxing makes code faster or having Dynamic be larger to skip the Boxing.

2) Add a feature gate. That gate can also turn on no_float automatically.

3) Register the relevant functions for your new numeric type. For most purposes you can copy the floating point code.

The easiest way to do it is to simply search for no_float and duplicate everything there where you'll have your decimal data type.

[bheylin]

Thanks @schungx

This is what I garnered from the source,but it's good to have that confirmed. I'll take a look at the task in more detail later today. I'm sure I'll be back with more questions, but this is enough to get me started ;)

[schungx]

Alternatively, if you don't really need ultra-high execution speed, Rhai already works with any Rust type. So it already works with your decimal type, except that it'll be kept as a trait object. That's one extra redirection from a Boxed decimal type.

All you need to do is to register the appropriate math functions like addition, subtraction etc. You may consider making a package that you can simply load. You'll need to do this anyway even if you build in your decimal type into Dynamic.

The only thing missing will be that you cannot parse a floating-point literal directly into a decimal value without going through some clumsy conversion function, as floating-point literals currently parse to f64. If you do not work exclusively with decimal numbers, then that's probably a small price to pay.

[bheylin]

Speed is indeed not important for my use cases. Correctness and clarity is more important. The scripts will be written by non-technical people. So I favor all floating-point literals being converted to Decimal as there is no need for mixed float and decimal representation in any use case I have.

[schungx]

Ah. That makes sense. In that case, the route you described above is the correct one.

Also, you'll want to rethink your need (or lack thereof) for speed. Numerical and financial calculations are seldom not CPU-bound (because calculations are usually all they do). So essentially, you may find yourself later needing speed.

[bheylin]

The speed performance constraints are being defined atm. I'm building a prototype that can be used to validate how Rhai performs with the complexity of the scripts we expect. So yes, speed is not paramount in the use cases, but of course it has a bound it needs to fit within. That bound is just unknown atm.

[schungx]

BTW did you get this working?

[bheylin]

Hey @schungx,

I have it mostly working from what I can see. The decimal type is behind a feature gate, and is being parsed and converted to Decimal type with a DecimalWrapper and all that jazz.

I've defined a set of basic arithmetic operators, but I still need to overload the + operator for string concatenation.

I've written a test where a decimal literal is use and added, but this test is failing due to a ErrorMismatchOutputType.

Error: ErrorMismatchOutputType("rust_decimal::decimal::Decimal", "Decimal", 0:0)

That's next on the list to figure out. By the looks of things, It's almost there. Thanks for following up ;)

[schungx]

Great! Beware: you only need a DecimalWrapper if the decimal type does not implement Hash. Otherwise, you can just put it inside Dynamic as a variant. The FloatWrapper is just there to implement Hash for f64.

Error: ErrorMismatchOutputType("rust_decimal::decimal::Decimal", "Decimal", 0:0)

Have you implemented From<Decimal> for Dynamic and Dynamic::try_cast for Decimal etc.?

Also I assume you're hijacking the floating-point parsing for Decimal, right?

[bheylin]

I was indeed missing the Dynamic::try_cast for Decimal.

rust_decimal::Decimal doesn't implement hash, so a wrapper is needed.

And yep I'm hijacking the floating-point parser for now. In the long run, this would be a decent use case for adding typed literals to the language:

let a = 0.333_f64;
let b = 0.333_d;

But my plan for now is to disable float support if decimal support is enabled.

[schungx]

Yes I think this is the smartest way to go right now.

There is an outstanding task for adding numerics with other types, which hasn't been implemented yet. This is because, other than INT (which is either i32 or i64), all other number types will be stored as trait objects. That's very inefficient, and the speed penalty wipes out the benefits of using a smaller data type. So this is a pattern that should not be encouraged. We really need to consider execution speed on all language designs...

[bheylin]

My bad Decimal actually does implement Hash. I'll remove the DecimalWrapper in that case.

Having only_decimal support satisfies my use cases, so I'm not in a rush to see typed literals implemented and given the performance trade offs, I'm not sure if it's ever necessary to implement them.

I'm going to do a pass over the Decimal implementation today, removing the DecimalWrapper and adding an only_decimal feature gate that adds feature = "no_float" if cfg(not(feature = "no_decimal")).

@schungx are you interested in having these changes merged into the main repo?

[schungx]

@schungx are you interested in having these changes merged into the main repo?

Most definitely!

I'd suggest calling the feature simply decimal because only_decimal implies that other types like i64 are also not supported. I don't think that's your implementation right now... you're simply supplanting floats.

decimal can require no_float so that should take care of removing floats support.

BTW, don't forget to implement the few functions for arrays and maps, so you can add decimals to them. That's in addition to printing.

Basic arithmetic, you can probably do it in fn_call.rs as built-in operators.

Other math functions... especially the float ones (e.g. sin, cos) you can decide whether to support them for decimal or not...

[bheylin]

Calling it decimal fits better with the usage indeed. I've called it no_decimal for now but phrasing it in the positive makes more sense.

I've added basic arithmetic functions through the arithmetic.rs file. And I've added the available advanced math functions to math_basic.rs.

What's the difference between the operators defined in fn_call.rs and what looks like duplicated operators in arithmetic.rs?

    // fn_call.rs
    #[cfg(not(feature = "no_float"))]
    if args_type == TypeId::of::<FLOAT>() {
        let x = x.clone().cast::<FLOAT>();
        let y = y.clone().cast::<FLOAT>();

        match op {
            "+" => return Ok(Some((x + y).into())),
            "-" => return Ok(Some((x - y).into())),
            "*" => return Ok(Some((x * y).into())),
            "/" => return Ok(Some((x / y).into())),
            "%" => return Ok(Some((x % y).into())),
            "~" => return pow_f_f(x, y).map(Into::into).map(Some),
            "==" => return Ok(Some((x == y).into())),
            "!=" => return Ok(Some((x != y).into())),
            ">" => return Ok(Some((x > y).into())),
            ">=" => return Ok(Some((x >= y).into())),
            "<" => return Ok(Some((x < y).into())),
            "<=" => return Ok(Some((x <= y).into())),
            _ => (),
        }
    }

    // arithmetic.rs
    // Basic arithmetic for floating-point - no need to check
    if cfg!(not(feature = "no_float")) {
        reg_op!(lib, "+", add_u, f32);
        reg_op!(lib, "-", sub_u, f32);
        reg_op!(lib, "*", mul_u, f32);
        reg_op!(lib, "/", div_u, f32);
        reg_sign!(lib, "sign", f32, f32);
        reg_sign!(lib, "sign", f64, f64);
    }

[schungx]

The built-in paths in fn_call.rs are short-cuts. They run much faster, but are only for selected data types.

Putting them in fn_call.rs is the right thing to do. Packages go through some bookkeeping overheads and run slower than the short-circuit. For example, x += y evaluates to x = x + y (which is two operations) in arithmetic.rs but it is a simple Rust statement in fn_call.rs that's much faster.

[schungx]

    // arithmetic.rs
    // Basic arithmetic for floating-point - no need to check
    if cfg!(not(feature = "no_float")) {
        reg_op!(lib, "+", add_u, f32);
        reg_op!(lib, "-", sub_u, f32);
        reg_op!(lib, "*", mul_u, f32);
        reg_op!(lib, "/", div_u, f32);
        reg_sign!(lib, "sign", f32, f32);
        reg_sign!(lib, "sign", f64, f64);
    }

You're right, this is unnecessary. Floating-point math is always checked. So the first four are unnecessary. It is a mistake to include them in arithmetic.rs because it prevent short-circuiting the calculation.

Thanks for spotting this!

Essentially, there should be no duplication between fn_call.rs and arithmetic.rs.

[bheylin]

Ok understood, I'll move the Decimal operators over to fn_call.rs ;)

[bheylin]

Also...

// logic.rs
    #[cfg(not(feature = "no_float"))]
    {
        reg_op!(lib, "<", lt, f32);
        reg_op!(lib, "<=", lte, f32);
        reg_op!(lib, ">", gt, f32);
        reg_op!(lib, ">=", gte, f32);
        reg_op!(lib, "==", eq, f32);
        reg_op!(lib, "!=", ne, f32);
    }

[schungx]

Wait... that's for f32. Can't remove those. And the operators too. My mistake.

fn_call.rs only handles f64 which is FLOAT. It does not handle f32.

[bheylin]

Ah, ok so they have a purpose

[bheylin]

A purpose given that there's two float types. But for Decimal I can just define basic arithmetic operators in fn_call.rs and the advanced operators in arithmetic.rs and math_basic.rs.

[bheylin]

I've added a decent set of test for the Decimal type and added the functions and tests to enable Decimal array support.

    #[cfg(not(feature = "no_decimal"))]
    {
        reg_op!(lib, "push", push, Decimal);
        reg_pad!(lib, "pad", pad, Decimal);
        reg_tri!(lib, "insert", ins, Decimal);
    }

I don't see any specific functions for maps. I'm guessing that maps are implemented as arrays?

[schungx]

I don't see any specific functions for maps. I'm guessing that maps are implemented as arrays?

Nope. Maps are implemented as HashMap. I checked and you're right - no need for specific functions.

[bheylin]

All good. I'll start moving no_decimal over to decimal and add a make no_float a requirement of decimal.

[schungx]

You may also start on a PR which you mark NOT YET FOR MERGE - then we can take a quick peek :-D

[bheylin]

Will do. This work in happening on company time. So I'll check that they are good with a public submission. I don't expect any problems though.

[bheylin]

@schungx Hey, I have the go ahead from work to make a pull request. I'll mark it with NOT YET FOR MERGE as you suggested. That's incoming today.

[schungx]

Hi, is the Decimal PR going to be up any time soon? No real pressure on this, just that I'm about ready to release 0.19.0 and wondering whether I should include Decimal.

[schungx]

@ObsceneGiraffe I have some free time today and so I implemented support for Decimal.

PR https://github.com/rhaiscript/rhai/pull/351

Please check it out and let me know how it works. If you do no_float together with decimal, the Decimal type replaces the f64 floating point type.

[schungx]

Closing this issue for now. Feel free to ping me if you find any prob.