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

feat: Default to bigint = "integer" #7

Closed
wants to merge 1 commit into from
Closed

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Dec 16, 2023

Defaulting to integer64 does more harm than good in my opinion. Happy to discuss.

@krlmlr krlmlr requested a review from nbenn December 16, 2023 11:07
@nbenn
Copy link
Member

nbenn commented Dec 16, 2023

One issue is that the nanoarrow int conversion is not compatible with the "silent overflow" requirement, which I feel is as good thing, especially if this is to be the default setting. But I am too happy to discuss this. I don't think defaulting to int64 is great. I just felt it was the least-worst option.

@krlmlr
Copy link
Member Author

krlmlr commented Dec 16, 2023

What do you mean by "silent overflow requirement"?

@nbenn
Copy link
Member

nbenn commented Dec 16, 2023

What do you mean by "silent overflow requirement"?

With that I mean

In addition, DBI supports the bigint argument that governs how 64-bit integer data is returned. The following values are supported:

  • "integer": always return as integer, silently overflow
  • "numeric": always return as numeric, silently round
  • ...

which I believe is explicitly enforced in connect_bigint_integer.

@krlmlr
Copy link
Member Author

krlmlr commented Dec 16, 2023

We could invent a new "integer-strict" setting here?

@nbenn
Copy link
Member

nbenn commented Dec 18, 2023

@krlmlr yes, if some form of bigint = "integer" should be default, we'd need to at least warn the user when they try to pull data that does not fit. From my side this is ready to go. Warnings in case of overflow are the default anyways. I can always muffle the warnings for DBI compliance-sake. Would you want to look into adding this "strict" int mode to DBI/DBItest?

@krlmlr
Copy link
Member Author

krlmlr commented Dec 18, 2023

A proper error seems like the better way.

Happy to review a proposal for an "integer-strict" mode in adbi, to see which checks now fail, before introducing them to DBI/DBItest.

@nbenn nbenn closed this Jan 26, 2024
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 this pull request may close these issues.

2 participants