feat: DuckDB connection as a state management instance #32
Conversation
| except Exception as e: | ||
| logger.info( | ||
| "Healthcheck failed for DuckDB" | ||
| "Initializing new connection object. Details: ", |
There was a problem hiding this comment.
This comment makes it seems like we try to initialize a new connection if the health check failed. That doesn't seem to be the case here right?
| self.connection.load_extension("substrait") | ||
| self.connection.install_extension("tpch") | ||
| self.connection.load_extension("tpch") | ||
| self.connection.execute(query=schema_lineitem) |
There was a problem hiding this comment.
Creating the table doesn't seem like it belongs in the connect method? maybe have a separate one just for the table creation?
| ) | ||
| finally: | ||
| return status | ||
| def check(self): |
There was a problem hiding this comment.
It might make more sense to have the separate method for table creation and doing the health check as part of that? And then store that health status in a class variable? What do you think?
EpsilonPrime
requested review from
EpsilonPrime
and removed request for
westonpace
|
@sanjibansg How do you want to proceed with this PR? |
|
closing this, since this is a requirement for the custom schema functionality, and has been implemented in the respective PR #51 |
This PR modifies the way we use the DuckDB instance for executing queries and parsing SQL queries to Substrait plans. We were using global variables, which aren't recommended, instead, it would be better to use a state management instance.