Skip to content

Fix binding negative ints #54

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 8, 2024
Merged

Fix binding negative ints #54

merged 1 commit into from
Nov 8, 2024

Conversation

cyberhuman
Copy link
Contributor

Fixes #31.
There is also a PR #48 which narrows down the integer to 32 bits instead, which is worse IMO because it will break working with BIGINT fields.

@ygrek
Copy link
Collaborator

ygrek commented Jul 3, 2024

i actually think that #48 behaviour makes sense, see the reasoning in 81e2a49
and bigint should be bound by the separate function (probably #43, didn't review), this PR cannot correctly handle all bigints anyway

@cyberhuman
Copy link
Contributor Author

I argue it should be long long for backwards compatibility. The change in #48 fixes negative values but breaks positive values >=2³²

@paurkedal
Copy link
Collaborator

paurkedal commented Jul 31, 2024 •
edited
Loading

My assessment is that #48 is fine, because long is the word length, and an OCaml integer is slightly narrower than the word length, so it would not overflow the long.

On the other hand we have no way to access the full range of the bigint SQL type, as far as I can tell, even on a 64 bit platform, so I think a int64 type would be a good addition (#43).

@paurkedal paurkedal mentioned this pull request Sep 19, 2024
Merged
@paurkedal
Copy link
Collaborator

I made the call and merged #48 instead, but thanks for the PR.

@paurkedal paurkedal closed this Sep 20, 2024
@tatchi tatchi mentioned this pull request Oct 29, 2024
Merged
@paurkedal paurkedal self-requested a review November 1, 2024 10:49
@paurkedal
Copy link
Collaborator

I'm re-opening this, due to #61. The problem with #48 is that MYSQL_TYPE_LONG maps to the C int type rather than the long type which corresponds to the OCaml int type according to the documentation (and my own tests). Since there is no MySQL type corresponding to the C long type, using MYSQL_TYPE_LONGLONG as proposed in this PR seems to be the right choice. Sorry for missing this point earlier.

Therefore I suggest we merge this.

@paurkedal paurkedal reopened this Nov 1, 2024
@paurkedal paurkedal requested a review from ygrek as a code owner November 1, 2024 11:39
@tatchi
Copy link
Contributor

tatchi commented Nov 5, 2024

Thanks for considering it. Also please let me know if you need help to rebase the branch on top of maser

@tatchi tatchi force-pushed the fix-negative-ints branch from 040dbf9 to 8a23403 Compare November 7, 2024 10:51
@tatchi
Copy link
Contributor

tatchi commented Nov 7, 2024

I have rebased this PR on top of master (+ fixed conflicts) as agreed with @cyberhuman in an offline discussion

paurkedal
paurkedal approved these changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@paurkedal paurkedal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the rebase; reaffirming my approval.

@paurkedal paurkedal merged commit 44c923c into ocaml-community:master Nov 8, 2024
paurkedal added a commit to paurkedal/opam-repository that referenced this pull request Nov 28, 2024
@paurkedal
[new release] mariadb (1.2.0)
cfc3b0e 
CHANGES:

  - Added `Stmt.start_txn` (ocaml-community/ocaml-mariadb#59 by Corentin Leruth).
  - Added `Res.insert_id` as binding for `mysql_stmt_insert_id` (ocaml-community/ocaml-mariadb#58 by
    Corentin Leruth).
  - Updated to support recent OCaml versions (ocaml-community/ocaml-mariadb#45 by @kit-ty-kate).
  - Fixed too-early retrieval of statement metadata (ocaml-community/ocaml-mariadb#41 by Albert Peschar).
  - Fixed decoding bug for the integer type (ocaml-community/ocaml-mariadb#54 by Raman Varabets, tested
    by ocaml-community/ocaml-mariadb#61 by Corentin Leruth).
  - Fixed a memory leaks related to result metadata (ocaml-community/ocaml-mariadb#39 by Albert Peschar).
  - The build system is now dune and dune-configurator (ocaml-community/ocaml-mariadb#52 by Petter A.
    Urkedal) and some of the examples have been converted to a test suite
    (ocaml-community/ocaml-mariadb#60 by Petter A. Urkedal).
  - The project has been transferred to ocaml-community with Petter A.
    Urkedal as the new maintainer.
@paurkedal paurkedal mentioned this pull request Nov 28, 2024
Merged
paurkedal added a commit to paurkedal/opam-repository that referenced this pull request Nov 28, 2024
@paurkedal
[new release] mariadb (1.2.0)
e139ee4 
CHANGES:

  - Added `Stmt.start_txn` (ocaml-community/ocaml-mariadb#59 by Corentin Leruth).
  - Added `Res.insert_id` as binding for `mysql_stmt_insert_id` (ocaml-community/ocaml-mariadb#58 by
    Corentin Leruth).
  - Updated to support recent OCaml versions (ocaml-community/ocaml-mariadb#45 by @kit-ty-kate).
  - Fixed too-early retrieval of statement metadata (ocaml-community/ocaml-mariadb#41 by Albert Peschar).
  - Fixed decoding bug for the integer type (ocaml-community/ocaml-mariadb#54 by Raman Varabets, tested
    by ocaml-community/ocaml-mariadb#61 by Corentin Leruth).
  - Fixed a memory leaks related to result metadata (ocaml-community/ocaml-mariadb#39 by Albert Peschar).
  - The build system is now dune and dune-configurator (ocaml-community/ocaml-mariadb#52 by Petter A.
    Urkedal) and some of the examples have been converted to a test suite
    (ocaml-community/ocaml-mariadb#60 by Petter A. Urkedal).
  - The project has been transferred to ocaml-community with Petter A.
    Urkedal as the new maintainer.
paurkedal added a commit to paurkedal/opam-repository that referenced this pull request Nov 29, 2024
@paurkedal
[new release] mariadb (1.2.0)
a21a7da 
CHANGES:

  - Added `Stmt.start_txn` (ocaml-community/ocaml-mariadb#59 by Corentin Leruth).
  - Added `Res.insert_id` as binding for `mysql_stmt_insert_id` (ocaml-community/ocaml-mariadb#58 by
    Corentin Leruth).
  - Updated to support recent OCaml versions (ocaml-community/ocaml-mariadb#45 by @kit-ty-kate).
  - Fixed too-early retrieval of statement metadata (ocaml-community/ocaml-mariadb#41 by Albert Peschar).
  - Fixed decoding bug for the integer type (ocaml-community/ocaml-mariadb#54 by Raman Varabets, tested
    by ocaml-community/ocaml-mariadb#61 by Corentin Leruth).
  - Fixed a memory leaks related to result metadata (ocaml-community/ocaml-mariadb#39 by Albert Peschar).
  - The build system is now dune and dune-configurator (ocaml-community/ocaml-mariadb#52 by Petter A.
    Urkedal) and some of the examples have been converted to a test suite
    (ocaml-community/ocaml-mariadb#60 by Petter A. Urkedal).
  - The project has been transferred to ocaml-community with Petter A.
    Urkedal as the new maintainer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ygrek ygrek ygrek approved these changes

@paurkedal paurkedal paurkedal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

int63 values are bound as int64, making them unsigned
4 participants
@cyberhuman @ygrek @paurkedal @tatchi