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

Decimal numbers are not correctly retrieved #16

Closed
simulot opened this issue Nov 5, 2020 · 20 comments
Closed

Decimal numbers are not correctly retrieved #16

simulot opened this issue Nov 5, 2020 · 20 comments

Comments

@simulot
Copy link
Contributor

simulot commented Nov 5, 2020

I have found that decimal numbers are retrieved as integer.

The following query

Select 2*1/3 from DUAL

returns 0 when expecting something like 0,6666666666666666666666666666666666666667

Here is the test code:

func main() {
	conn, err := sql.Open("oracle", "oracle://system:oracle@192.168.10.10/xe")
	OKorDie(err)
	defer conn.Close()

	stmt, err := conn.Prepare("Select 2*1/3 from DUAL")
	OKorDie(err)
	defer stmt.Close()

	rows, err := stmt.Query()
	OKorDie(err)
	defer rows.Close()

	for rows.Next() {
		var N sql.NullFloat64
		err = rows.Scan(&N)
		OKorDie(err)
		fmt.Println("N: ", N)
	}
}
@sijms
Copy link
Owner

sijms commented Nov 6, 2020

this issue is double sided issue
see this script:

 with inc (N) as (
	select 1 N from dual
	union all
	select N+1 N from inc where N <100
) 
select N, N as N2 from INC

raised in issue#11 I expect type int64

and this script:
select 2*1/3 from dual
you expect type float64

but actually the server deal the same in both situation it send precision = 0 and scale = 0xFF
which is something like decimal ==> float64

I make a mistake when solve issue#11 by making scale=0 when scale=0xFF to solve the
type problem.
but as you see the previous code is correct and both situation has type decimal ==> float64 thus I
return to the previous code scale = 0xFF

@simulot
Copy link
Contributor Author

simulot commented Nov 6, 2020

I was suspecting something like that. Using floats to represent integer leads to this.
I have updated the package and the "rounding issue" is back:

I have seen a problem with 123456.78 giving 123456.77999999998 with GO-ORA
I'm doing accounting report with data, and it would be nice to not having to cope with rounding in the client program.
When tracing the DecodeDouble function
bytes from data gives ret =12.345678. Note, we still have needed digits.

then ret = ret / powerTable[x][2]
x is 6 and powerTable[x][2] is 0.0001
ret takes 123456.77999999998
Then following is done: round(123456.77999999998* 10^255) / 10^255 which doesn't change the final result.

May bet the OCI library is doing differently for not messing the precision?

@sijms
Copy link
Owner

sijms commented Nov 6, 2020

I do a test to see where is the problem
I go to the function DecodeDouble() and add 2 code line

fmt.Println("input data: ", inputData)
at the function beginning

fmt.Println("final data: ", ret)
at the function end

if I run the sql code

with inc (N) as (
	select 1 N from dual
	union all
	select N+1 N from inc where N <100
) 
select N, N as N2 from INC		

the output for 61 and 62 is

input data:  [193 62]
final data:  61
input data:  [193 62]
final data:  61
input data:  [193 63]
final data:  62
input data:  [193 63]
final data:  62
input data:  [193 64]
final data:  63
input data:  [193 64]
final data:  63

but the output of the final result is:

60.99999999999999        60.99999999999999
62       62
62.99999999999999        62.99999999999999

this means that the problem is not in decodeDouble

if I change datatype from float64 to float32
output is normally

61      61
62      62
63      63

@sijms
Copy link
Owner

sijms commented Nov 6, 2020

I find the problem:
in file command.go inside function read line 459

base := math.Pow10(int(dataSet.Cols[x].Scale))
dataSet.currentRow[x] = math.Round(converters.DecodeDouble(temp)*base) / base

I change it to

base := math.Pow10(int(dataSet.Cols[x].Scale))
if dataSet.Cols[x].Scale < 0x80 {
	dataSet.currentRow[x] = math.Round(converters.DecodeDouble(temp)*base) / base
} else {
	dataSet.currentRow[x] = converters.DecodeDouble(temp)
}

suppose scale more than 0x80 is impractical

now the code work correctly either you use int64 or float64

@simulot
Copy link
Contributor Author

simulot commented Nov 7, 2020

It solve the 69 case,
But there are more of the same : 988 for example.

I have added Println in DecodeDouble.
input data: [194 10 89]
final data: 988.0000000000001
So, some round error appears in DecodeDouble.

Inside it , right after line 176 ret was exactly 9.88. Witch is good

I still think the problem is in multiple divisions by power of 10.

@sijms
Copy link
Owner

sijms commented Nov 7, 2020

yes you are correct may be this line is the source of problem
dataSet.currentRow[x] = math.Round(converters.DecodeDouble(temp)*base) / base

can you convert to:
dataSet.currentRow[x] = converters.DecodeDouble(temp)

and see if any problem happen with you

@simulot
Copy link
Contributor Author

simulot commented Nov 7, 2020

I did it, but it has no impact.
So I think the problem is inside DecodeDouble function.

@simulot
Copy link
Contributor Author

simulot commented Nov 7, 2020

Try -10, got -11.02...
Confirmed, there is a problem in DecodeDouble.

@sijms
Copy link
Owner

sijms commented Nov 7, 2020

please send me byte also to analyze like this:
input data: [194 10 89]

i am working on it and find the location of first problem

fmt.Println("modified calc: ", float64(9.88) / 0.01) // give 988.0000000000001

@simulot
Copy link
Contributor Author

simulot commented Nov 7, 2020

I have found an interesting article about how Oracle is storing numbers:
https://gotodba.com/2015/03/24/how-are-numbers-saved-in-oracle/
You can find a sql script to get internal representation of number

select N, vsize(N), dump(N) from (
select 0 N from dual union
select 1 N from dual union
select 69 N from dual union
select 1008 N from dual union
select 123456.78 N from dual union
select -1 N from dual union
select -1008 N from dual union
select -123456.78 N from dual 
)

This gives:

-123456,78	6	Typ=2 Len=6: 60,89,67,45,23,102
-1008	4	Typ=2 Len=4: 61,91,93,102
-1	3	Typ=2 Len=3: 62,100,102
0	1	Typ=2 Len=1: 128
1	2	Typ=2 Len=2: 193,2
69	2	Typ=2 Len=2: 193,70
1008	3	Typ=2 Len=3: 194,11,9
123456,78	5	Typ=2 Len=5: 195,13,35,57,79

Placed into a go program, the result is:

-123456.79019999997 Typ=2 Len=6: 60,89,67,45,23,102 '-123456.78'
-1009.0199999999999 Typ=2 Len=4: 61,91,93,102 '-1008'
-2.02 Typ=2 Len=3: 62,100,102 '-1'
0 Typ=2 Len=1: 128 '0'
1 Typ=2 Len=2: 193,2 '1'
69 Typ=2 Len=2: 193,70 '69'
1008 Typ=2 Len=3: 194,11,9 '1008'
123456.77999999998 Typ=2 Len=5: 195,13,35,57,79 '123456.78'

I'm also forking the project to add tests for this function, and try to help on resolution

@sijms
Copy link
Owner

sijms commented Nov 8, 2020

I am nearly find the problem and in the way to solve
see this line in function DecodeSign (line 109)

if len(input) <= 20 && input[len(input)-1] == 102 {
	input = input[:len(input)-1]
}

here I update the length of input slice and suppose this update will be alive after function end but this is not happen

now I am working on update all the code and rewrite DecodeDouble and test it

@sijms
Copy link
Owner

sijms commented Nov 8, 2020

I push the changes please test and inform me

@simulot
Copy link
Contributor Author

simulot commented Nov 8, 2020

I did a naïve implementation of the function to limit the use of floats at the minimal.

I'll create a push request with it and test code.

// DecodeDouble decode Oracle binary representation of numbers into float64
//
// Some documentation:
//	https://gotodba.com/2015/03/24/how-are-numbers-saved-in-oracle/
//  https://www.orafaq.com/wiki/Number

func DecodeDouble(inputData []byte) float64 {

	if len(inputData) == 0 {
		return math.NaN()
	}
	if inputData[0] == 0x80 {
		return 0
	}
	var (
		negative bool
		exponent int
		mantissa int64
	)

	negative = inputData[0]&0x80 == 0
	if negative {
		exponent = int(inputData[0]^0x7f) - 64
	} else {
		exponent = int(inputData[0]&0x7f) - 64
	}

	buf := inputData[1:]
	// When negative, strip the last byte if equal 0x66
	if negative && inputData[len(inputData)-1] == 0x66 {
		buf = inputData[1 : len(inputData)-1]
	}

	for _, digit100 := range buf {
		digit100--
		if negative {
			digit100 = 100 - digit100
		}
		mantissa *= 10
		mantissa += int64(digit100 / 10)
		mantissa *= 10
		mantissa += int64(digit100 % 10)
	}

	digits := 0
	temp64 := mantissa
	for temp64 > 0 {
		digits++
		temp64 /= 100
	}
	exponent = (exponent - digits) * 2
	if negative {
		mantissa = -mantissa
	}

	ret := float64(mantissa) * math.Pow10(exponent)
	return ret
}

@sijms
Copy link
Owner

sijms commented Nov 8, 2020

please test my change first

@simulot
Copy link
Contributor Author

simulot commented Nov 8, 2020

I read your message after having created the PR.
Sorry.

Your implementation works too.
Anyway, I think my implementation is straightforward and more idiomatic.

@sijms
Copy link
Owner

sijms commented Nov 8, 2020

ok I will get you code and comment my code
still my code need more testing

@simulot
Copy link
Contributor Author

simulot commented Nov 8, 2020

Thanks!

I suppose EncodeDouble should get your attention too.

@sijms
Copy link
Owner

sijms commented Nov 8, 2020

I need more test for all the driver to find all problem and fix

@sijms sijms closed this as completed in 061667e Nov 8, 2020
sijms added a commit that referenced this issue Nov 8, 2020
Fix #16 Rewrite of DecodeDouble to limit floating point errors
@sijms sijms reopened this Nov 8, 2020
@sijms
Copy link
Owner

sijms commented Nov 8, 2020

still this code give problem
select 2*1/3 from dual
result is
-3.3646807698472525e+18

@sijms
Copy link
Owner

sijms commented Nov 8, 2020

I update the code and problem solved

@sijms sijms closed this as completed Nov 8, 2020
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

No branches or pull requests

2 participants