-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(postgres): add support for stored procedures in postgres. #14102
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM Thanks for the contrib
I left some minor and mostly cosmetic comments
metadata-ingestion/src/datahub/ingestion/source/sql/postgres.py
Outdated
Show resolved
Hide resolved
metadata-ingestion/src/datahub/ingestion/source/sql/postgres.py
Outdated
Show resolved
Hide resolved
default=True, | ||
description="Include ingest of stored procedures. Requires access to the 'sys' schema.", | ||
) | ||
include_stored_procedures_code: bool = Field( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this flag being used, if that's the case, let's remove it
Co-authored-by: Sergio Gómez Villamor <sgomezvillamor@gmail.com>
Co-authored-by: Sergio Gómez Villamor <sgomezvillamor@gmail.com>
…ld and edited description of the include_stored_procedures field
metadata-ingestion/src/datahub/ingestion/source/sql/postgres.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I just left a final code suggestion with a tiny fix for a string interpolation that is causing the tests to fail.
As soon as tests pass, this can be merged
Thanks for the contrib!
Co-authored-by: Sergio Gómez Villamor <sgomezvillamor@gmail.com>
Yeah, no problem, thanks for the help/advice. |
…ub-project#14102) Co-authored-by: root <root@Aarush-PC.localdomain> Co-authored-by: Sergio Gómez Villamor <sgomezvillamor@gmail.com>
Related to issue 13541.
Golden files for postgres integration have been updated to account for the change and the added support for stored procedures.