trino-python-client icon indicating copy to clipboard operation
trino-python-client copied to clipboard

[sqlalchemy] Ensure TrinoDialect.get_table_names only returns real tables

Open john-bodley opened this issue 2 years ago • 2 comments

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.

* Fix some things. ({issue}`issuenumber`)

john-bodley avatar Oct 13 '22 18:10 john-bodley

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)

hashhar avatar Oct 13 '22 18:10 hashhar

@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 avatar Oct 14 '22 01:10 john-bodley

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

john-bodley avatar Oct 20 '22 18:10 john-bodley