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.

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

fix: 1261 pg custom query without creating view #1353

Merged
merged 12 commits into from
Dec 3, 2024
Merged

fix: 1261 pg custom query without creating view #1353

merged 12 commits into from
Dec 3, 2024

Conversation

sundar-mudupalli-work
Copy link
Collaborator

Hi,

The Postgres Custom Query validation used to create a temporary view. This fixes #1261, by finding the types from the custom query itself. Change only to one file that is specific for Postgres.

Sundar Mudupalli

@sundar-mudupalli-work sundar-mudupalli-work requested a review from a team as a code owner November 26, 2024 03:16
@sundar-mudupalli-work sundar-mudupalli-work linked an issue Nov 26, 2024 that may be closed by this pull request
Error validating Postgres using Custom Query "permission denied to create temporary tables" #1261
Closed
@sundar-mudupalli-work
Copy link
Collaborator Author

/gcbrun

@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 26, 2024
@sundar-mudupalli-work
Copy link
Collaborator Author

/gcbrun

@sundar-mudupalli-work
Copy link
Collaborator Author

/gcbrun

nj1973
nj1973 requested changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@nj1973 nj1973 left a comment

Choose a reason for hiding this comment

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

I wonder if we should replace the test_custom_query_validation_core_types test with one on dvt_pg_types to test with more data types?

third_party/ibis/ibis_postgres/client.py Show resolved Hide resolved
third_party/ibis/ibis_postgres/client.py Outdated Show resolved Hide resolved
helensilva14
helensilva14 reviewed Nov 27, 2024
View reviewed changes
Copy link
Collaborator

@helensilva14 helensilva14 left a comment

Choose a reason for hiding this comment

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

Definitely a complex challenge to solve, well done Sundar!

third_party/ibis/ibis_postgres/client.py Outdated Show resolved Hide resolved
third_party/ibis/ibis_postgres/client.py Outdated Show resolved Hide resolved
@sundar-mudupalli-work
Copy link
Collaborator Author

I wonder if we should replace the test_custom_query_validation_core_types test with one on dvt_pg_types to test with more data types?

The big difference is only in the time data types. Ibis support many more of the Postgres types - MAC Address, Polygon etc - we have not defined how to validate those. We may need to figure that out and include those types would be my suggestion.

Sundar Mudupalli

@sundar-mudupalli-work
Copy link
Collaborator Author

/gcbrun

@nj1973
Copy link
Contributor

nj1973 commented Nov 29, 2024

Ibis support many more of the Postgres types - MAC Address, Polygon etc - we have not defined how to validate those

Agreed, I wasn't suggesting we look into any new data types. It just crossed my mind that we could switch the test from one existing table to another already existing table. Anyway - let's leave it alone for now.

nj1973
nj1973 approved these changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@nj1973 nj1973 left a comment

Choose a reason for hiding this comment

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

LGTM

@nj1973
Copy link
Contributor

nj1973 commented Dec 2, 2024

/gcbrun

@sundar-mudupalli-work sundar-mudupalli-work merged commit f25fe80 into develop Dec 3, 2024
5 checks passed
@sundar-mudupalli-work sundar-mudupalli-work deleted the 1261-pg-custom-query-without-creating-view branch December 3, 2024 03:47
@release-please release-please bot mentioned this pull request Dec 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@helensilva14 helensilva14 helensilva14 approved these changes

@nj1973 nj1973 nj1973 approved these changes

@nehanene15 nehanene15 Awaiting requested review from nehanene15

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error validating Postgres using Custom Query "permission denied to create temporary tables"
3 participants
@sundar-mudupalli-work @nj1973 @helensilva14