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

Test and specify bigint argument to dbConnect() #133

Closed
jimhester opened this issue Apr 11, 2017 · 12 comments
Closed

Test and specify bigint argument to dbConnect() #133

jimhester opened this issue Apr 11, 2017 · 12 comments

Comments

@jimhester
Copy link
Contributor

Basically MySQL always returns a BIGINT (64 bit integer) if you have scalar arithmetic in your query.

E.g. the following and friends, which is used heavily in DBItest.

dbGetQuery(con, "SELECT 1")

So if I return these values as integer64 objects in R DBItest has numerous failures because it is expecting regular 32 bit R integers in this case, so you get error like the following.

dbFetch(res)[[1]] not equal to 1.
Attributes: < Modes: list, NULL >
Attributes: < Lengths: 1, 0 >
Attributes: < names for target but not for current >
Attributes: < current is not list-like >
target is integer64, current is numeric

I think in order to support this we would need a tweak that relaxed the comparison function to compare based on the numeric values rather than object identity.

@krlmlr
Copy link
Member

krlmlr commented Apr 11, 2017

Are you using MySQL via the odbc package?

@jimhester
Copy link
Contributor Author

jimhester commented Apr 12, 2017 via email

@krlmlr
Copy link
Member

krlmlr commented Apr 12, 2017

I don't think this is a mistake in the specs, but we might be more up front and give nicer test output.

The general idea is that backends should return a proper R data type if the data fits, in the case of integers it's an integer and not bit64::integer64. I have solved the problem of "on-demand coercion" in RSQLite, and I'll extract the relevant parts for reuse in other packages soon, but not earlier than May.

@jimhester
Copy link
Contributor Author

Changing the returned type based on the values is a dangerous idea. So if a given table is updated on the database with a larger number the column goes from being an integer to a integer64? If the driver returns a stable data type it should be used.

jimhester added a commit to jimhester/DBItest that referenced this issue Apr 12, 2017
@jimhester
Copy link
Contributor Author

FWIW I can work around this by adding comparison methods for integer64 objects and relaxing a few of the tests master...jimhester:bit64.

@krlmlr
Copy link
Member

krlmlr commented Apr 12, 2017

I suspect that always returning integer64 is against the expectations of many DBI users. They might just expect a number most of the times.

Does the data type returned by the driver depend on the width/size of the column?

@jimhester
Copy link
Contributor Author

It doesn't always return integer64, it only returns integer64 if you use a scalar number in your query. SELECT column + 1 FROM table would be a 64 bit integer, but SELECT column from table would be a 32 bit integer if column is of SQL type INTEGER. It should be noted the PostgreSQL implementation does not do this, and returns 32 bit integers in both cases. This is compounded by the fact that MySQL does not support casting to 32 bit integers using CAST(), which would be the easy solution to ensure 32 bit integers in the tests.

Alternatively the tests could be written against proper tables with explicitly defined column types, which would also avoid the implicit typing used in this case.

@krlmlr
Copy link
Member

krlmlr commented Apr 12, 2017

This behavior is surprising at the very least, but I guess we have to live with that. I'll look into this, thanks for your input.

@krlmlr
Copy link
Member

krlmlr commented Apr 21, 2017

I think this is another reason to return numerics for 64-bit-data, with the original data as an integer64 object in an attribute. See r-dbi/bigrquery#94 (comment) for an example.

@krlmlr
Copy link
Member

krlmlr commented Oct 4, 2017

Following up the discussion in r-dbi/RMariaDB@bcd4216#commitcomment-24368964: There's now a bigint argument to odbc::dbConnect(), accepting one of c("integer64", "integer", "numeric", "character"). Seems a good idea to add this to the spec. Open questions:

  • What to do if the data doesn't fit the data type? NA with warning? NA with a "problems" attribute?
  • @hannesmuehleisen asked if the choices are part of the spec (I think so) and if "integer64" refers to a particular implementation (probably we don't need that, just test the lossless behavior like we do currently)

@jimhester
Copy link
Contributor Author

Right now we don't do anything with data that doesn't fit the datatype, it just silently overflows. They way the ODBC API functions work there isn't an efficient way to test for overflow, the onus is on the user to be sure the values won't overflow before using a non-default mapping.

@krlmlr
Copy link
Member

krlmlr commented Oct 31, 2017

@hannesmuehleisen asked in r-dbi/RMariaDB@bcd4216#commitcomment-24368964:

If part of the spec, would the choices listed above be also part of the specification? And does integer64 refer to the class of the same name in the bit64 package or more to a generic class?

If part of the spec, we should be using the same options across backends: as numeric, as integer, or as integer64. The former two are lossyless, the latter would refer to any class that can be coerced losslessly to character and also to numeric/integer if size permits. So, in theory your backend should be able to use character and get away with it.

@krlmlr krlmlr changed the title MySQL returns 64 bit integer for all scalar inputs Test and specify "bigint" argument to dbConnect() Dec 18, 2017
@krlmlr krlmlr changed the title Test and specify "bigint" argument to dbConnect() Test and specify bigint argument to dbConnect() Dec 18, 2017
@krlmlr krlmlr added this to To Do in krlmlr Dec 18, 2017
@krlmlr krlmlr closed this as completed in d669af5 Apr 25, 2018
krlmlr automation moved this from To Do to Done Apr 25, 2018
krlmlr added a commit to r-dbi/RMariaDB that referenced this issue Apr 25, 2018
- Add support for `bigint` argument to `dbConnect()`, supported values are `"integer64"`, `"integer"`, `"numeric"` and `"character"`. Large integers are returned as values of that type (r-dbi/DBItest#133).
krlmlr added a commit to r-dbi/RSQLite that referenced this issue Apr 25, 2018
- Add support for `bigint` argument to `dbConnect()`, supported values are `"integer64"`, `"integer"`, `"numeric"` and `"character"`. Large integers are returned as values of that type (r-dbi/DBItest#133).
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
krlmlr
  
Done
Development

No branches or pull requests

2 participants