DM-37744: Make daf_butler compatible with sqlalchemy 2 #788
Conversation
There are two issues fixed in this commit: - SA now handles UUID internally for postgresql and returns UUID. - Transaction handling needs more care as there is no autocommit. - str(URL) replaces password with starts.
andy-slac
force-pushed
the
tickets/DM-37744
branch
from
46bb8bf
to
2c47f31
Compare
Codecov ReportBase: 85.48% // Head: 85.49% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #788 +/- ##
==========================================
+ Coverage 85.48% 85.49% +0.01%
==========================================
Files 265 265
Lines 35043 35047 +4
Branches 7367 7370 +3
==========================================
+ Hits 29957 29964 +7
+ Misses 3770 3768 -2
+ Partials 1316 1315 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -1763,7 +1774,9 @@ def query( | |||
| connection = self._engine.connect() | |||
| else: | |||
| connection = self._session_connection | |||
| result = connection.execute(sql, *args, **kwargs) | |||
| # TODO: SelectBase is not good for execute(), but it used everywhere, | |||
| # e.g. in daf_relation. We should switch to Executable at some point. | |||
There was a problem hiding this comment.
Can you make a ticket for this if daf_relation is using some old syntax? cc/ @TallJimbo
There was a problem hiding this comment.
I made DM-37971 for @TallJimbo, but you can reassign it to me if you don't have time to work on that.
| if value is None: | ||
| return value | ||
| elif isinstance(value, uuid.UUID): | ||
| # sqlalchemy 2 converts to UUID internally |
There was a problem hiding this comment.
Does that mean in sqlalchemy 1.4 we can't do this?
There was a problem hiding this comment.
In 1.4 strings are returned for UUIDs for both Postgres and sqlite, strings are still handled in else clause below.
| @@ -197,7 +200,7 @@ def _convertExclusionConstraintSpec( | |||
| metadata: sqlalchemy.MetaData, | |||
| ) -> sqlalchemy.schema.Constraint: | |||
| # Docstring inherited. | |||
| args = [] | |||
| args: list[tuple[sqlalchemy.schema.Column, str]] = [] | |||
There was a problem hiding this comment.
Are you surprised this method never gets called?
There was a problem hiding this comment.
I'm surprised. I'd expect this to be called whenever we register a calibration dataset type with a new set of dimensions.
There was a problem hiding this comment.
Grepping through the code it looks like that it is only called when TableSpec.exclusion is not empty, and that could only happen when timespan representation class hasExclusionConstraint() returns True. I found only one implementation of hasExclusionConstraint() which always returns False.
There was a problem hiding this comment.
Oh, interesting. The PostgreSQL-specific timespan representation should have hasExclusionConstraint() return True, as that's one of the things that btree_gist brings, and would avoid a table lock (IIRC) in certify (which I thought we were already avoiding for PostgreSQL).
But I'm not sure if we can just fix that without a migration now that we've made the mistake.
| @@ -57,7 +58,9 @@ | |||
| from .._exceptions import ConflictingDefinitionError | |||
|
|
|||
|
|
|||
| def _checkExistingTableDefinition(name: str, spec: ddl.TableSpec, inspection: List[Dict[str, Any]]) -> None: | |||
| # TODO: method is called with list[ReflectedColumn] in SA 2, and | |||
There was a problem hiding this comment.
So when we get SA2 we can make the type annotation explicit about that list contents?
There was a problem hiding this comment.
Right, we should clean it up when we decide to drop 1.4 support.
Lots of changes everywhere to make it run with sqlalchemy 2. Many mypy fixes. I enabled sqlalchemy 2 in requirements, so GA should build with that.
Checklist
doc/changes