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

Explicitly cast postgres function return values #1693

Merged
merged 1 commit into from
Nov 3, 2023
Merged

Explicitly cast postgres function return values #1693

merged 1 commit into from
Nov 3, 2023

Conversation

sluhn-harrisr
Copy link
Contributor

Two database functions fail in some versions of postgres, because their output is not explicitly cast to match the expected function return value type. This change simply does the explicit casting to match.

Type of PR

  • BUG
  • FEAT
  • MAINT
  • DOC

Is your Pull Request linked to an existing Issue or Pull Request?

Closes #1685

Give a brief description for the solution you have provided

The original issue was with the ave_months_between function that returned float8 values. I fixed that one and then the integer version failed in the same way, so i fixed that one too.

After doing that, I was able to successfully instantiate a PostgresLinker object, and I confirmed that the database functions were all created successfully

PR Checklist

  • Added documentation for changes
  • Added feature to example notebooks or tutorial (if appropriate)
  • Added tests (if appropriate)
  • Made changes based off the latest version of Splink
  • Run the linter

Note - i ran the linter and it did report some stuff, but nothing in the two places changed by this commit (of course, since it was such a simple change to some string values). so I considered those linter messages to be irrelevant here.

Two database functions fail in some versions of postgres, because their
output is not explicitly cast to match the expected function return
value type.  This change simply does the explicit casting to match.
Copy link
Member

@RobinL RobinL left a comment

Choose a reason for hiding this comment

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

Thanks very much - looks good to me. I'll let @ADBond just double check this before merging

Copy link
Contributor

@ADBond ADBond 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 this, looks good 👍

@ADBond
Copy link
Contributor

ADBond commented Nov 3, 2023

A word of warning though @sluhn-harrisr - from my testing it seems like there might be some other issues which could arise also using older versions of Postgres, which might be more closely linked to the inner workings of Splink. I haven't looked in great deal so not sure exactly which bits of functionality are effected, so you may well be fine.
If you do run into further issues let us know and we may be able to help

@sluhn-harrisr
Copy link
Contributor Author

Thank you both @ADBond and @RobinL. Aside from these two functions things have been going well so far; fingers crossed it's all good from here.

As for the mechanics of the PR - who does the merging into the master branch of the project? Is that me, now that it's reviewed and approved, or do you do that?

@ADBond ADBond merged commit d14aea8 into moj-analytical-services:master Nov 3, 2023
@ADBond
Copy link
Contributor

ADBond commented Nov 3, 2023

Sorry - got interrupted part-way through there, have now merged!

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.

DB type error when creating ave_months_between function in postgres 9.4
3 participants