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

fix(c/driver/postgresql): add postgres type to cols created for numeric #1516

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

lupko
Copy link
Contributor

@lupko lupko commented Feb 5, 2024

  • introduce constant with key name
  • introduce separate private method to add field-level method

Fixes #1515

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

A unit test would be appreciated but I can understand if the setup is a little difficult

@lupko
Copy link
Contributor Author

lupko commented Feb 5, 2024

@lidavidm thanks for checking it out. good point re the tests - will add; I see there are bunch of tests in this area already so will extend. i'll get back to this early tomorrow.

- introduce constant with key name
- introduce separate private method to add field-level method
@lupko
Copy link
Contributor Author

lupko commented Feb 6, 2024

@lidavidm I have added tests (exercising numeric and array of numeric); the tests have revealed a little tweak was needed in AddPostgresTypeMetadata.

The fallback to using typename resolver because the typname is not always set. In end-to-end scenarios everything works well without the fallback because the type mapping is populated differently and includes type names.

I see some tests have failed in the CI run - checked it out and it does not look related to these changes (some Flight SQL cancellation stuff)

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks!

@lidavidm lidavidm merged commit 51abae1 into apache:main Feb 6, 2024
53 of 54 checks passed
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.

c/driver/postgresql: postgres type not included in string columns created for NUMERIC
2 participants