Enhance BigQueryHandler with table filtering capabilities#23
Conversation
- Introduced `include_tables` and `exclude_tables` parameters in connection arguments for selective table access. - Implemented methods to parse and validate table lists, and retrieve filtered tables based on connection settings. - Updated `get_tables` and metadata retrieval methods to respect filtering configurations. - Added comprehensive tests to ensure filtering functionality works as intended, including backward compatibility checks.
There was a problem hiding this comment.
Pull request overview
This PR adds table filtering capabilities to the BigQueryHandler, allowing users to specify which tables to include or exclude when connecting to a BigQuery dataset. The implementation includes connection parameters for filtering, caching mechanisms, and comprehensive test coverage.
Key changes:
- Added
include_tablesandexclude_tablesconnection parameters for filtering accessible tables - Implemented caching of filtered table lists with invalidation on disconnect
- Updated all metadata methods to respect connection-time and query-time filtering
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| connection_args.py | Added include_tables and exclude_tables connection parameters |
| bigquery_handler.py | Implemented table filtering logic in handler with caching and validation |
| test_bigquery_handler.py | Added comprehensive test suite for filtering functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self._filtered_tables is not None: | ||
| return self._filtered_tables |
There was a problem hiding this comment.
[nitpick] The cache check treats an empty list as a valid cached value that should be returned. However, the cache is initialized to None on line 35. If filtering results in an empty list (e.g., all tables excluded), the cache will store [] on line 219, 236, etc. On subsequent calls, line 172 will return the cached empty list, but the condition if self._filtered_tables is not None on line 172 evaluates to True for an empty list, which is correct. However, the initial check should differentiate between 'not yet computed' (None) and 'computed as empty' ([]). Currently the logic is correct, but consider using a sentinel value or checking hasattr(self, '_filtered_tables_computed') to make the caching logic more explicit and maintainable.
| tables = res.data_frame['table_name'].tolist() | ||
| assert "TEST" not in tables | ||
| # Should still have other tables | ||
| assert len(tables) >= 0 |
There was a problem hiding this comment.
This assertion is always true since len() never returns a negative value. Replace with a more meaningful assertion, such as assert len(tables) > 0 if you expect at least one table, or remove it if the test is only verifying TEST is not present.
| assert len(tables) >= 0 | |
| assert len(tables) > 0 |
No description provided.