fix(go/adbc/driver/snowflake): handling of integer values sent for NUMBER columns #1267
Conversation
|
|
CurtHagenlocher
changed the title
| query := "SELECT CAST('" + numberString + fmt.Sprintf("' AS NUMBER(%d, %d)) AS RESULT", precision, scale) | ||
| decimalNumber, err := decimal128.FromString(numberString, int32(precision), int32(scale)) | ||
| suite.NoError(err) | ||
| number := int64(decimalNumber.LowBits()) |
There was a problem hiding this comment.
I don't think this is great behavior for the driver, but it is the current behavior.
There was a problem hiding this comment.
Can you add a comment here as to why the resulting value is equal to the low bits?
There was a problem hiding this comment.
Do you mind filing an issue to update this? It sounds like we must error here
| // We can't do a cast directly into the destination type because the numbers we get from Snowflake | ||
| // are scaled integers. So not only would the cast produce the wrong value, it also risks producing | ||
| // an error of precisions which e.g. can't hold every int64. To work around these problems, we instead | ||
| // cast into a decimal type of a precision and scale which we know will hold all values and won't | ||
| // require scaling, We then substitute the type on this array with the actual return type. |
There was a problem hiding this comment.
I hate this so much, but it makes sense that we have to do this because of how snowflake works.
| // For precisions of 16, 17 and 18, a conversion from int64 to float64 fails with an error | ||
| // So for these precisions, we instead convert first to a decimal128 and then to a float64. |
There was a problem hiding this comment.
What's the error? Should those precisions instead work and we should push a fix upstream to the Arrow lib?
There was a problem hiding this comment.
The error message is "invalid: integer value 99999999999999999 not in range: -9007199254740992 to 9007199254740992". I do think that it makes sense to allow a lossy conversion in Arrow from int64 to float64 and that would avoid the need for this special case. This may require some design work in Arrow -- for instance, having the Divide kernel take a CastOptions or adding AllowFloatTruncate to ArithmeticOptions.
|
BTW Curt: please use "fixes" or "closes" in the commit message so that GitHub picks up on it, "addresses" doesn't appear to be in its keyword list |
|
@zeroshade or @lidavidm - is anything else needed to merge this? |
| query := "SELECT CAST('" + numberString + fmt.Sprintf("' AS NUMBER(%d, %d)) AS RESULT", precision, scale) | ||
| decimalNumber, err := decimal128.FromString(numberString, int32(precision), int32(scale)) | ||
| suite.NoError(err) | ||
| number := int64(decimalNumber.LowBits()) |
There was a problem hiding this comment.
Do you mind filing an issue to update this? It sounds like we must error here
lidavidm
changed the title
…MBER columns (apache#1267) Fixes apache#1242.
Fixes #1242.