Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add is not compatible with sqlite #447

Open
alpaylan opened this issue Dec 11, 2024 · 10 comments
Open

Add is not compatible with sqlite #447

alpaylan opened this issue Dec 11, 2024 · 10 comments
Labels
bug Something isn't working compat good first issue Good for newcomers help wanted Extra attention is needed

Comments

@alpaylan
Copy link
Contributor

In sqlite,

sqlite>  select 0.1 + 0.2;
0.3

Limbo is not compatible,

limbo> select 0.1 + 0.2;
0.30000000000000004

An short inspection led me to;

(OwnedValue::Float(lhs), OwnedValue::Float(rhs)) => {
    state.registers[dest] = OwnedValue::Float(lhs + rhs);
}

in core/vdbe/mod.rs/step function.

I first thought the problem was with printing, but sqlite seems to hold the actual result with no floating point errors, I was led to believe from the following interaction with sqlite.

sqlite> SELECT printf('%.100f', 0.1 + 0.2);
0.3000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

This also holds #446 back, because one of the tests is using the 0.1 + 0.2 in the replacement.

@jussisaurio jussisaurio added bug Something isn't working help wanted Extra attention is needed good first issue Good for newcomers compat labels Dec 11, 2024
@jussisaurio
Copy link
Collaborator

Thanks for the report. In the meantime, can you change your test to use another expression instead? Glad that your test discovered the issue though.

@alpaylan
Copy link
Contributor Author

Thanks! I'll do that now. AFAICT sqlite by default should exhibit the same behavior as Limbo, but also the CLI by default uses decimals, and that's the reason for the difference. Implementing decimals should solve the issue.

@vignesh-j-shetty
Copy link
Contributor

Is solution is round off it?

@alpaylan
Copy link
Contributor Author

Is solution is round off it?

Ah no, I think the solution is to wrap floating points with decimals at the operation sites, and unwrap at the end. This way, the core language still uses floating points, but the operations themselves are precise. I can implement a prototype today if that's the desired behavior.

@anhpnv
Copy link

anhpnv commented Dec 14, 2024

As i found, the limbo cli return with this line when we select with float cli/main.rs
The problem is enum Value we defined in core/type.rs file ,float type is f64 which cause when we use operator like this. I tested in my local machine some specific case:

limbo> select 0.1 + 0.2;
0.30000000000000004
limbo> select 0.12 + 0.22;
0.33999999999999997
limbo> select 0.1 + 0.7;
0.7999999999999999
limbo> 

To resolve this problem, instead of changing enum type, i'm only change the output return to f32 in this line Line315, and here is some of my result:

limbo> select 0.1 + 0.2;
0.3
```bash
limbo> select 0.12 + 0.22;
0.34
limbo> select 0.1 + 0.7;
0.8

@jussisaurio can you please assign me this issue ? I will push my pull request 😁

@alpaylan
Copy link
Contributor Author

That doesn't really fix the problem though, does it? If the output itself is a value that requires 64 bits to be expressed, this change would lose information.

@travenin
Copy link
Contributor

FYI in PR #460 I'm adding do_execsql_test_tolerance test case function, which takes a tolerance/delta in addition to expected value. It's a workaround to deal with floating point inaccuracy in CLI compatibility tests, as long as Limbo's floating point is not compatible with SQLite.

@dnylpz
Copy link

dnylpz commented Dec 16, 2024

just to confirm would a solution based on the sqlite decimal implementation be considered an appropriate solution?

if so, i'd like to work on this issue, seems like a good first issue to work on for me.

@alpaylan
Copy link
Contributor Author

I think we still need to understand how and when sqlite defaults to decimals. The sqlite spec itself does not default to them, but the CLI does. So there's some work required for achieving full compatibility. I tried implementing decimals myself, it's not hard to get a working version, although it might also be better to use rust_decimal instead of implementing one from scratch, at least at first.

@dnylpz
Copy link

dnylpz commented Dec 17, 2024

sounds good, i'll dedicate some time to figure out where and how the sqlite cli uses those decimals.

thanks for the guidance @alpaylan

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working compat good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants