Conversation
… into DBQueryEvaluation
|
@hardikgu23 is this ready for review? Sorry if I missed it. |
… into DBQueryEvaluation
Hi @mahyareb , yes it is. |
… into DBQueryEvaluation
… into DBQueryEvaluation
… into DBQueryEvaluation
| return generate_ddl(rows, self.db_name) | ||
|
|
||
| def _execute(self, query: str) -> Tuple[Any, float]: | ||
| def _execute(self, query: str, rollback: bool = False, use_transaction: bool = True) -> Tuple[Any, float]: |
There was a problem hiding this comment.
it doesn't seem like we're doing anything with the transaction and rollback. Is that a TODO?
There was a problem hiding this comment.
yes, From discussion: plan is to have seperate execution path for dql, dml and ddl execution. So leaving this as it is so as to not further complicate the pr.
| if rollback: | ||
| transaction.rollback() | ||
| else: | ||
| for query in queries: |
There was a problem hiding this comment.
Do you still need "with connection.begin():" here?
There was a problem hiding this comment.
We do not need the use_transaction parameter anymore for mysql. So this will be changed in different pr.
| if rollback: | ||
| transaction.rollback() | ||
| else: | ||
| resultset = connection.execution_options(isolation_level="AUTOCOMMIT").execute(text(query)) |
There was a problem hiding this comment.
What are we using AUTOCOMMIT for here?
There was a problem hiding this comment.
Databases in postgres cannot be created within transaction block. Autocommit is being used here to execute queries in non-transactional mode.
From sqlalchemy documentation:
https://docs.sqlalchemy.org/en/20/orm/session_transaction.html#setting-transaction-isolation-levels-dbapi-autocommit.
| if error: | ||
| logging.error(f"Error while creating user: {error}") | ||
| return | ||
| setup_file = os.path.join(os.path.dirname(__file__), f"schema_details/bat/{database}/setup.yaml") |
There was a problem hiding this comment.
This right now just points to bat in the directory, could you get this from the config?
There was a problem hiding this comment.
Great suggestion. To not further complicate the pr, will create a seperate pr doing the same.
mahyareb
left a comment
mahyareb
left a comment
There was a problem hiding this comment.
Overall LGTM Hardik! Great work. Left some comments in there.
3 participants
Seperate execution for dql, dml and ddl queries
Add flter by evalID range, tags
Add retry in setup-teardown