Skip to content

Fix "double" handling with H2 #3273

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
nvoxland merged 3 commits into from
Sep 21, 2022
Merged

Fix "double" handling with H2 #3273

nvoxland merged 3 commits into from
Sep 21, 2022

Conversation

@MalloD12
Copy link
Collaborator

@MalloD12 MalloD12 commented Sep 13, 2022

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Fix double type column parsing when double type is specified with precision (i.e. double(25)).

Fixes #3099

Things to be aware of

  • Added logic to parse double(xy) as a "Double Precision" for H2 database.
  • Only impact on H2 database.

Things to worry about

  • Nothing.

Additional Context

  • Nothing.

@MalloD12 MalloD12 requested a review from nvoxland as a code owner September 13, 2022 20:45
@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2022 •
edited
Loading

Unit Test Results

  4 620 files  ±  0    4 620 suites  ±0   35m 32s ⏱️ -41s
  4 621 tests +  1    4 406 ✔️ +  1     215 💤 ±0  0 ❌ ±0 
54 624 runs  +12  49 604 ✔️ +12  5 020 💤 ±0  0 ❌ ±0 

Results for commit b4f5563. ± Comparison against base commit 31484f1.

♻️ This comment has been updated with latest results.

nvoxland
nvoxland approved these changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@nvoxland nvoxland left a comment

Choose a reason for hiding this comment

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

Looking at the docs, it looks like h2 1.x supported "double" and "double precision" as aliases for each other, but 2.x only supports "double precision. Neither version supports arguments, so I simplified the code down to just "always return double precision"

@MalloD12 MalloD12 requested a review from suryaaki2 September 20, 2022 13:21
@XDelphiGrl XDelphiGrl self-requested a review September 20, 2022 13:32
XDelphiGrl
XDelphiGrl approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@XDelphiGrl XDelphiGrl left a comment

Choose a reason for hiding this comment

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

Fix addresses a bug in Liquibase SQL generation for the DOUBLE PRECISION datatype.

  • New unit tests added to validate input of "double" and "double(20)"; both inputs are expect to return DOUBLE PRECISION.
    • H2 itself does not support providing precision, but Liquibase will handle such input as DOUBLE PRECISION.
  • No additional testing required.

APPROVED

@nvoxland nvoxland merged commit d9d9652 into master Sep 21, 2022
@nvoxland nvoxland changed the title Fix double with precision type error for H2 Fix "double" handling with H2 Sep 21, 2022
@nvoxland nvoxland deleted the fix-issue-3099 branch September 21, 2022 21:46
@nvoxland nvoxland added this to the 1NEXT milestone Sep 21, 2022
@TomBenjamins
Copy link

TomBenjamins commented Sep 23, 2022

Could you please apply this fix also to the BIGINT, INTEGER, BOOLEAN types?

#3099 (comment)

this bug is blocking for us to upgrade to h2 v2.

@bhuism
Copy link

bhuism commented Sep 23, 2022

+1

2 similar comments
@richardmLL
Copy link

richardmLL commented Sep 23, 2022

+1

@hilkojacobse
Copy link

hilkojacobse commented Sep 23, 2022

+1

@MalloD12
Copy link
Collaborator Author

MalloD12 commented Sep 23, 2022

@TomBenjamins We have already applied a similar fix for INT(xt). I wasn't aware this was also an issue for BOOLEAN and BIGINT. I'll discuss it today with the team and get back to you guys. Thanks for the feedback.

@MalloD12
Copy link
Collaborator Author

MalloD12 commented Sep 23, 2022

Hey guys, I just want to let you know I'll be creating a separate issue to address BIGINT and BOOLEAN types. For INTEGER, we already have an open issue for that. But I would also like to ask you if you guys can share some example (changelog/changeset) on how you are using these types.

Thanks,
Daniel.

@TomBenjamins
Copy link

TomBenjamins commented Sep 23, 2022

Hello Daniel,

for us the only blocking one is BIGINT, as that one is being used in nearly all our table creation scripts.
On mysql it still works fine, but on H2 v2 we get similar syntax exceptions as described in this issue and #3099
If you can provide me the new issue link I can add a liquibase example for at least the BIGINT issue there.

greetings,

Tom

@MalloD12
Copy link
Collaborator Author

MalloD12 commented Sep 23, 2022

Hey @TomBenjamins I have just created this issue, please have a look to it and feel free to edit/anything you guys consider relevant for me when working on it.

Thanks,
Daniel.

This was referenced Jul 22, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@nvoxland nvoxland nvoxland approved these changes

@XDelphiGrl XDelphiGrl XDelphiGrl approved these changes

@suryaaki2 suryaaki2 suryaaki2 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Liquibase Community
Archived in project

Milestone

v4.17.0

Development

Successfully merging this pull request may close these issues.

DOUBLE with precision doesn't work anymore with H2 database

10 participants

@MalloD12 @TomBenjamins @bhuism @richardmLL @hilkojacobse @nvoxland @XDelphiGrl @suryaaki2 @kataggart