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

Detection of Float query parameter types is problematic #6

Open
rko281 opened this issue Dec 18, 2019 · 2 comments
Open

Detection of Float query parameter types is problematic #6

rko281 opened this issue Dec 18, 2019 · 2 comments

Comments

@rko281
Copy link
Contributor

rko281 commented Dec 18, 2019

MySQLBindParameter>>detectParamType handles Floats (and Fractions) as follows:

((paramValue isKindOf: Float) or:  [paramValue isKindOf: Fraction]) 
		ifTrue: [	
			(paramValue asFloat at: 2) = 0 
				ifTrue: [^ MySQLTypes typeFLOAT]
				ifFalse: [^ MySQLTypes typeDOUBLE] ].

Firstly I'm not sure this is a valid test for floats versus doubles. For example, 4.25 is detected as typeFLOAT whereas 4.2 is detected as typeDOUBLE.

More generally, I think the approach of detecting the parameter type based on the value is incorrect. Test case (where c is a connected MySQLDriver):

c query: 'drop table if exists float_test'.
c query: 'create table float_test (f float)'.
r := c prepare: 'insert into float_test(f) values(?);'.
s := (MySQLDriverStatement onConnection: c) stmtId: r prepareOkay stmtHandlerId; cursoredFetch: false; yourself.
s addBinding: 4.2.
s execute.

This fails at the execute stage with an Out of Range error, since 4.2 is detected as typeDOUBLE, but is being assigned to a FLOAT parameter.

Fundamentally, I think it's flawed to try to deduce the parameter type based on the value being assigned; the type should come from the column definition for the parameter (I don't know if MySQL provides an easy way to do this however).

As a partial aside, running the above test with 4.25 (which is picked up as typeFLOAT) results in a returned value of 2.26562 - this looks to be due to an issue with MySQLBindParameter>>floatBytes which (I'm guessing) is due to a change in the internal representation of Floats in Pharo at some point in the past.

Running the test with a DOUBLE column also results in incorrect results even when using a typeDOUBLE value:

c query: 'drop table if exists double_test'.
c query: 'create table double_test (d double)'.
r := c prepare: 'insert into double_test(d) values(?);'.
s := (MySQLDriverStatement onConnection: c) stmtId: r prepareOkay stmtHandlerId; cursoredFetch: false; yourself.
s addBinding: 4.2.
s execute.
(c query: 'select * from double_test') rows first at: 1.   " '-9.255965342982487e61' "

This issue can be fixed by reversing the order in which the paramValue's bytes are considered in MySQLBindParameter>>doubleBytes:

doubleBytes
	| storable |
	ByteArray
		streamContents: [ :strm | 
			storable := paramValue asFloat at: 2.
			strm
				nextPut: (storable byteAt: 1);
				nextPut: (storable byteAt: 2);
				nextPut: (storable byteAt: 3);
				nextPut: (storable byteAt: 4).
			storable := paramValue asFloat at: 1.
			strm
				nextPut: (storable byteAt: 1);
				nextPut: (storable byteAt: 2);
				nextPut: (storable byteAt: 3);
				nextPut: (storable byteAt: 4).
			^ strm contents ]

All tests carried out with 64bit Pharo 7.0 on OSX, connecting to MySQL (actually MariaDB) on Raspberry Pi 4.

@pharo-todd
Copy link
Contributor

MySQL is interested in the types of the variables you are binding. It will convert to the appropriate type on the server if possible. If not possible to convert then mysql_stmt_bind_param() will return CR_UNSUPPORTED_PARAM_TYPE.

I found this information on accepted type conversions between C and MySQL types:
https://dev.mysql.com/doc/refman/8.0/en/c-api-prepared-statement-type-conversions.html

Some examples:

If you use MYSQL_TYPE_LONG with an int variable to pass an integer value to the server that is to be stored into a FLOAT column, MySQL converts the value to floating-point format before storing it.

If you fetch an SQL MEDIUMINT column value, but specify a buffer_type value of MYSQL_TYPE_LONGLONG and use a C variable of type long long int as the destination buffer, MySQL converts the MEDIUMINT value (which requires less than 8 bytes) for storage into the long long int (an 8-byte variable).

If you fetch a numeric column with a value of 255 into a char[4] character array and specify a buffer_type value of MYSQL_TYPE_STRING, the resulting value in the array is a 4-byte string '255\0'.

MySQL returns DECIMAL values as the string representation of the original server-side value, which is why the corresponding C type is char[]. For example, 12.345 is returned to the client as '12.345'. If you specify MYSQL_TYPE_NEWDECIMAL and bind a string buffer to the MYSQL_BIND structure, mysql_stmt_fetch() stores the value in the buffer as a string without conversion. If instead you specify a numeric variable and type code, mysql_stmt_fetch() converts the string-format DECIMAL value to numeric form.

For the MYSQL_TYPE_BIT type code, BIT values are returned into a string buffer, which is why the corresponding C type is char[]. The value represents a bit string that requires interpretation on the client side. To return the value as a type that is easier to deal with, you can cause the value to be cast to integer using either of the following types of expressions:

So I don't think we need to pull the column type so much as provide a conversion to an appropriate C type that can MySQL can work with.

@rko281
Copy link
Contributor Author

rko281 commented Dec 19, 2019

Thanks for the clarification; I was rather confused by the combination of the MySQLBindParameter>>detectParamType Float test and the byte-order issue in MySQLBindParameter>>doubleBytes:.

The issue appears to be fixable by implementing the change to byte-ordering in doubleBytes: (above) and removing the test for typeFLOAT in detectParam, i.e. always returning typeDOUBLE for Floats/Fractions.

With both these changes it's possible to successfully bind arbitrary Float values to FLOAT, DOUBLE, DECIMAL and INTEGER column parameters.

estebanlm added a commit that referenced this issue Mar 26, 2020
Fixes and Tests for Issues #5, #6 and #7
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