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

Support mapping BIGINT to string in MySQL #15

Closed
yshrsmz opened this issue Feb 22, 2024 · 6 comments · Fixed by #16
Closed

Support mapping BIGINT to string in MySQL #15

yshrsmz opened this issue Feb 22, 2024 · 6 comments · Fixed by #16

Comments

@yshrsmz
Copy link
Contributor

yshrsmz commented Feb 22, 2024

Currently sqlc-gen-typescript maps MySQL's BIGINT type to number type, but the max value of number type is far smaller than that of BIGINT.

node-mysql2 supports big numbers by converting it to string, so sqlc should also support mapping bigint to string.

https://github.com/mysqljs/mysql#connection-options

Here's a related quote from the doc

supportBigNumbers: When dealing with big numbers (BIGINT and DECIMAL columns) in the database, you should enable this option (Default: false).
bigNumberStrings: Enabling both supportBigNumbers and bigNumberStrings forces big numbers (BIGINT and DECIMAL columns) to be always returned as JavaScript String objects (Default: false). Enabling supportBigNumbers but leaving bigNumberStrings disabled will return big numbers as String objects only when they cannot be accurately represented with [JavaScript Number objects] (https://tc39.es/ecma262/#sec-ecmascript-language-types-number-type) (which happens when they exceed the [-2^53, +2^53] range), otherwise they will be returned as Number objects. This option is ignored if supportBigNumbers is disabled.

I think we can simply navigate users to enable those two options to use BIGING support.

@yshrsmz
Copy link
Contributor Author

yshrsmz commented Feb 25, 2024

It looks like the PostgreSQL counterpart is already treated as a string.

@kyleconroy Any idea on how we handle this case?
I can send a PR to make this happen.

@kyleconroy kyleconroy changed the title support mapping BIGINT to string in MySQL Support mapping BIGINT to string in MySQL Mar 3, 2024
@kyleconroy
Copy link
Contributor

The default types map to the default return values for the mysql2 library. mysql2 returns floats for bigints by default, which I think is the wrong choice. Instead, we should allow users to configure this (and a few other options) based on what you can pass to createPool.

- schema: "authors/mysql/schema.sql"
  queries: "authors/mysql/query.sql"
  engine: "mysql"
  codegen:
  - plugin: ts
    out: node-mysql2/src/db
    options:
      runtime: node
      driver: mysql2
      mysql2:
        big_number_strings: true
        date_strings: true

Do you have time to update #16 with that functionality?

@yshrsmz
Copy link
Contributor Author

yshrsmz commented Mar 3, 2024

@kyleconroy sure, will do that.

But I have a few questions

Should we support enabling supportBigNumbers and bigNumberStrings independently? This makes things a bit harder.

I think it's yes. I'm going to conditionally change bigint return type to number, number | string or string.

IMO we should update columnType() method to conditionally change bitint return type, but how?

To achieve this(and more configurable feature in the future) we need to pass options to columnType(), but currently it does not accept options parameter.

Change method signature to columnType(c?: Column, options: Options)

And updates everywhere. this can be a hussle.

Change drivers/mysql2.ts's default export to a "driver factory".

This is a combination of the first one.
To minimize the impact, create a driver factory method.

export default function createMysql2Driver(options: Mysql2Options): Driver {
  return {
    columnType: (c?: Column) => columnType(c, options),
    preamble,
    execDecl,
    manyDecl,
    oneDecl,
  }
}

This way, what we need is updating mysql2-related code for now.

or any other?

@kyleconroy
Copy link
Contributor

Should we support enabling supportBigNumbers and bigNumberStrings independently? This makes things a bit harder.

That's a good point. I think it's a good idea since we can generate the correct type signature.

Change columnType?

Yeah, I really don't want to have to change that signature. We should probably move away from drivers as modules and instead have them as classes. I'll do that once #13 is merged so that we don't create too much churn.

@yshrsmz
Copy link
Contributor Author

yshrsmz commented Mar 3, 2024

We should probably move away from drivers as modules and instead have them as classes.

That sounds good to me.
I'm going to wait for that change, then.

@kyleconroy
Copy link
Contributor

Okay, the refactor is done. The classes don't have constructors yet.

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

Successfully merging a pull request may close this issue.

2 participants