Skip to content

Fix - Presto SQLAlchemy dialect did not implement get_view_names - #7190

Closed
rad-pat wants to merge 7 commits into
apache:masterfrom
rad-pat:fix-presto-dialect
Closed

rad-pat wants to merge 7 commits into
apache:masterfrom
rad-pat:fix-presto-dialect

Conversation

@rad-pat

@rad-pat rad-pat commented Sep 4, 2025 •

Copy link
Copy Markdown
Contributor

Why are the changes needed?

Presto SQLAlchemy dialect did not implement the get_view_names method and resulted in an exception when trying to inspect the schema. This was discovered in Superset repo whilst trying to update the pandas package which now makes a call to get_view_names.

How was this patch tested?

Very basic Python tests have been added here, but all the SQLAlchemy dialects in this repo would benefit from running the full SQLAlchemy dialect test suite instead of these bespoke tests.

@rad-pat
rad-pat marked this pull request as ready for review September 4, 2025 09:14
@rad-pat rad-pat changed the title [WIP] Fix - Presto SQLAlchemy dialect did not implement get_view_names Fix - Presto SQLAlchemy dialect did not implement get_view_names Sep 4, 2025
@pan3793

pan3793 commented Sep 5, 2025

Copy link
Copy Markdown
Member

It doesn't seem lucky, the change causes test failure. Could you take a look?

@github-actions github-actions Bot added the kind:infra license, community building, project builds, asf infra related, etc. label Sep 5, 2025
@rad-pat

rad-pat commented Sep 5, 2025

Copy link
Copy Markdown
Contributor Author

@pan3793, I have hopefully fixed it - I also extended the python range of tests to cover supported versions of Python, and bumped the package version in the hopes that a release could be made to PyPI when merged.

@rad-pat

rad-pat commented Sep 5, 2025

Copy link
Copy Markdown
Contributor Author

Looking at failures, new python versions failing because of old packages. The previous versions of python still failing for same reason, I thought I had fixed it, checking...

@codecov-commenter

codecov-commenter commented Sep 5, 2025 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (ea75fa8) to head (c2d06f7).
⚠️ Report is 17 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #7190    +/-   ##
=======================================
  Coverage    0.00%   0.00%            
=======================================
  Files         695     696     +1     
  Lines       43433   43544   +111     
  Branches     5887    5891     +4     
=======================================
- Misses      43433   43544   +111     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot removed the kind:infra license, community building, project builds, asf infra related, etc. label Sep 5, 2025
@rad-pat

rad-pat commented Sep 7, 2025

Copy link
Copy Markdown
Contributor Author

@pan3793 - definitely no broken Python tests now, I removed the addition of current Python versions as they fail because dependencies are not up to date with Python > 3.10. So, just the changes required and tests now working. 🤞 More work than I was hoping for.

Comment thread python/pyhive/__init__.py Outdated
Comment thread python/pyhive/sqlalchemy_presto.py Outdated
Comment thread python/pyhive/sqlalchemy_presto.py Outdated
Comment thread python/pyhive/sqlalchemy_presto.py Outdated
Comment thread python/pyhive/sqlalchemy_presto.py Outdated
@pan3793 pan3793 added this to the v1.11.0 milestone Sep 22, 2025
@pan3793 pan3793 closed this in 5771fdd Sep 22, 2025
@pan3793

pan3793 commented Sep 22, 2025

Copy link
Copy Markdown
Member

Thanks, merged to master

@pan3793 pan3793 reopened this Sep 22, 2025
@pan3793 pan3793 closed this in b992623 Sep 22, 2025
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.

3 participants