[BUGFIX] Use one inspector for the same engine/execution_engine #8534
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…data-is-not-caching
| def _get_sqlalchemy_column_metadata( | ||
| execution_engine: SqlAlchemyExecutionEngine, batch_data: SqlAlchemyBatchData | ||
| ): |
There was a problem hiding this comment.
Sorry for the late review.
@TrangPham are you sure this is meant to be the SqlAlchemyExecutionEngine and not just the native Sqlalchemy sqlachemy.engine,Engine type?
A type annotation here would have been helpful 😄
There was a problem hiding this comment.
Yes this is correct. It is being called in one place in this function which passes in the execution engine:
…data-is-not-caching
…data-is-not-caching
| @pytest.mark.sqlite | ||
| def test_table_column_type__sqlalchemy_happy_path(sa, capfd): |
There was a problem hiding this comment.
Curious : were you able to get this test passing locally?
There was a problem hiding this comment.
Yes this passes locally for me
…data-is-not-caching
…data-is-not-caching
…data-is-not-caching
|
Updated the tests so it would work. Seems like it passed for me locally, but within the github ci the sqlite file was being closed and capsys.readerrout().err was showing that the file was closed when trying to run empty SQL. This is for the happy path with used our metric resolution directly vs the other test which calls get_column without going through the metric resolution. |
| # Should have to re-inspect due to new batch | ||
| # But should use same inspector | ||
| assert mock_inspect.call_count == 1 |
…data-is-not-caching
TrangPham
deleted the
DX-60-clone-issue-5933-bug-snowflake-metadata-is-not-caching
branch
Previously, we were getting a new inspector each time that "table.column_types" metric was being computed. This means that SQLAlchemy would not see that information as cached as it is for a different inspector instance.
This changes adds _inspector to the ExecutionEngine and uses that same inspector later on (making it a singleton-ish).
Some additional refactoring can be done to move the functionality of "get_column_metadata" into the different engines and just ask the engine for that information. This would mean a refactor of the functions that "table_column_types.py" uses.
Closes: #5933
invoke lint(usesblack+ruff)For more information about contributing, see Contribute.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!