Breakout connection from athena DB init #328
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
| # the profile may not be required, provided the above three AWS env vars | ||
| # are set. If both are present, the env vars take precedence |
There was a problem hiding this comment.
This comment is lightly out of date now, since it refers to "above three AWS env vars" and it got moved around
| @@ -102,8 +105,11 @@ def col_pyarrow_types_from_sql(self, columns: list[tuple]) -> list: | |||
| output.append((column[0], pyarrow.int64())) | |||
| case "double": | |||
| output.append((column[0], pyarrow.float64())) | |||
| # This is future proofing - we don't see this type currently. | |||
There was a problem hiding this comment.
I thought we did see it in one edge case - I don't think a normal production flow, but it might have been when dealing with flat tables? Or I'm making it up.
| @abc.abstractmethod | ||
| def connect(self): | ||
| """Initiates connection configuration of the database""" |
There was a problem hiding this comment.
If we wanted to be Extremely Python ™️, we would probably write this abstract class as a context manager, so you could do:
with AthenaDatabase(...) as db:
# use db
To automatically get connect/close support (and make sure that callers don't forget one of the calls, even when an exception happens).
But not necessary right now and can often be a little annoying to reorganize code to support that style (but does have some benefits - mainly the exception handling).
| if "prepare" not in args.keys(): | ||
| backend.connect() | ||
| elif not args["prepare"]: | ||
| backend.connect() |
There was a problem hiding this comment.
nit: if not args.get("prepare"):
The automatic db connection is causing some issues for the packaging use case (where we want to generate sql based on db type, but aren't actually using a connection), so this creates an explicit connection function that is called seperately from the database object init. Connection logic is then moved from the individual db init functions to the connect function.
Checklist
docs/needs to be updatedgenerate-mdcorestudy fields that not in US Core, update our list of those incore-study-details.mdmanifest.toml