trinodb / trino-python-client

Python client for Trino
Apache License 2.0
309 stars 151 forks source link

[sqlalchemy] Ensure TrinoDialect.get_table_names only returns real tables #266

Closed john-bodley closed 1 year ago

john-bodley commented 1 year ago

Description

Per the SQLAlchemy documentation the get_table_names method is intended to return the real table names, i.e., without views. The PR updates the get_table_names to only return real table names whereas previously it returned real tables and views.

Note there were no prior integration tests for the TrinoDialect.get_view_names method so I added some to cover this logic and threw in a few extra for free which should cover all the various scenarios.

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required. ( ) Release notes are required, please propose a release note for me. (x) Release notes are required, with the following suggested text:

The TrinoDialect.get_table_names function logic has changed to adhere to the SQLAlchemy specification. Now only real table names are returned as opposed to both real table and view names.

* Return only tables in SQLAlchemy  `get_table_names`. Previously, it contained views. ({issue}`266`)
hashhar commented 1 year ago

Also commit message to say:

Ensure SQLAlchemy get_table_names only returns tables and not views

Per the [SQLAlchemy
documentation](https://docs.sqlalchemy.org/en/14/core/reflection.html#sqlalchemy.engine.reflection.Inspector.get_table_names)
the get_table_names method is intended to return only table names,
i.e., exclude views.

Also improve the performance of get_view_names by querying
INFORMATION_SCHEMA.TABLES with an appropriate type filter. Querying the
INFORMATION_SCHEMAS.VIEWS tables seems to be sub-performant for
determining which entities are views probably because it needs to fetch
view definitions.

(also if I were nitpicking I'd say it makes sense to have two separate commits - improve perf of view listing, fix get_table_names)

john-bodley commented 1 year ago

@hashhar I've addressed your comments. I addition to adding an integration test for this specific change I added additional tests which should cover the gamut of conditions for the TrinoDialect.get_table_names method.

john-bodley commented 1 year ago

@mdesmet thanks for the review. I've addressed your comments.