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

Use "numeric" (without parameters) data type in PostgreSQL #1906

Merged
merged 5 commits into from Jul 1, 2022

Conversation

LonwoLonwo
Copy link
Contributor

@LonwoLonwo LonwoLonwo commented Jun 15, 2021

when input data type has char parameters in the data type (like NUMBER(*,0) in Oracle)

catch NumberFormatException

Steps:

  1. Create a table with NUMBER(*,0) type column in Oracle schema.
CREATE TABLE TEST_ASTERISK (
COLUMN1 NUMBER(*,0)
);
  1. Try to compare this Oracle schema with any PostgreSQL schema.
    Actual result: catch NumberFormatException

Hello from DBeaver.

…t data type has char parameters in data type (like NUMBER(*,0) in Oracle)
Copy link
Contributor

@molivasdat molivasdat left a comment

Choose a reason for hiding this comment

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

This change would allow values such as NUMBER(*) and NUMBER(1..0) etc. and still try to use numeric as the value. Yes, it will fix your use case specifically but needs to account for other invalid non number formats.

@kevin-atx kevin-atx added the SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions label Jun 15, 2022
@github-actions
Copy link

github-actions bot commented Jun 15, 2022

Unit Test Results

  4 560 files  +   391    4 560 suites  +391   30m 37s ⏱️ + 3m 27s
  4 527 tests +       5    4 309 ✔️ +     77     218 💤  -   26  0 ±0 
53 604 runs  +4 522  48 592 ✔️ +4 124  5 012 💤 +444  0 ±0 

Results for commit cd01227. ± Comparison against base commit e8c6019.

♻️ This comment has been updated with latest results.

@nvoxland nvoxland changed the base branch from master to 1_9 June 24, 2022 21:15
@nvoxland nvoxland changed the base branch from 1_9 to master June 24, 2022 21:15
@nvoxland nvoxland added SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions and removed SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Jun 24, 2022
@nvoxland
Copy link
Contributor

The piece that was keeping me from reviewing it sooner was thinking on what sort of tests we'd like for this. The fix itself is very isolated but also sort of expansive since it treats any number parse error as that we should ignore the arguments. While numeric(*, 0) is not valid for postgresql, there is may be other values for that first argument which is not a number but is valid and we don't want to loose that. Or, even though the postgresql docs say it has to be a positive integer, future versions and/or existing variants may have other options. Like enterprisedb may actually support numeric(*, 0).

Really, the only reason we're trying to parse that first argument is because there times we get a larger than 1000 value from metadata and so we want to remove it in that case. But in general we try to respect the arguments users pass along, under the assumption they understand what is possible in their database better than we do.

I ended up adding a new unit test around the toDatabaseType() and also updating your change to preserve the arguments if it gets arguments it doesn't understand.

That still works well with your oracle vs. postgresql comparison logic, correct @LonwoLonwo ?

@kataggart kataggart added this to the NEXT milestone Jun 29, 2022
@LonwoLonwo
Copy link
Contributor Author

Thanks @nvoxland

Now it all looks fantastic, and it's working for me.

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 improves handling of precision for Postgres numeric datatypes.
    • Change limited to Postgres (and by extension, EDB)
  • A nice new set of tests added to validate behavior with different number types.

APPROVED

Passing Functional Tests
Passing Test Harness Execution

@nvoxland nvoxland merged commit e1379f3 into liquibase:master Jul 1, 2022
Conditioning++ automation moved this from To Do to Done Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autocandidate DBOracle DBPostgres enabler ImpactLow IntegrationAny SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions Severity4 sprint2022-28 TypeBug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

8 participants